Do not always refresh packed-refs during ref updates
Replace the refreshPackedRefs() method with a new getLockedPackedRefs()
method taking a LockFile as a parameter to better clarify the real
intention of this method and to better differentiate when it should be
called versus getPackedRefs(). This differentiation is important because
SnapshottingRefDirectory overrides getPackedRefs() to cache refs
throughout a whole request without ever verifying if they have changed
during normal read paths. However, during ref update paths,
SnapshottingRefDirectory needs to still verify if the packed-refs have
changed, and having two methods ensures that for a
SnapshottingRefDirectory the two methods will be different. The LockFile
argument is not really required to create a correct
getLockedPackedRefs(), but it does help ensure the method is only used
in ref update paths.
Unlike the replaced refreshPackedRefs() method, but similar to the
getPackedRefs() method, the new getLockedPackedRefs() method also
benefits from caching if the packed-refs file has not changed making
ref updates much faster! This takes a push down from almost 30s to
around 13s, on NFS using after_open semantics and a repository with 4M+
refs. With the same setup, this change also improves ten parallel
pushes from 60s/push to around 40s/push (so both latency and throughput
are improved). This does add some extra risk in that the jgit "trust"
settings are now also applied during ref update paths where as
previously a re-read was always done in these paths. Depending on your
point of view, this change can also be seen as a bug fix, not just a
performance improvement.
Change-Id: I8e9636c37802a249c9f5555748d69045beeea608
Signed-off-by: Martin Fick <mfick@nvidia.com>
diff --git a/Documentation/config-options.md b/Documentation/config-options.md
index 78930e6..b30b958 100644
--- a/Documentation/config-options.md
+++ b/Documentation/config-options.md
@@ -55,7 +55,7 @@
| `core.supportsAtomicFileCreation` | `true` | ⃞ | Whether the filesystem supports atomic file creation. |
| `core.symlinks` | Auto detect if filesystem supports symlinks| ✅ | If false, symbolic links are checked out as small plain files that contain the link text. |
| `core.trustFolderStat` | `true` | ⃞ | Whether to trust the pack folder's, packed-refs file's and loose-objects folder's file attributes (Java equivalent of stat command on *nix). When looking for pack files, if `false` JGit will always scan the `.git/objects/pack` folder and if set to `true` it assumes that pack files are unchanged if the file attributes of the pack folder are unchanged. When getting the list of packed refs, if `false` JGit will always read the packed-refs file and if set to `true` it uses the file attributes of the packed-refs file and will only read it if a file attribute has changed. When looking for loose objects, if `false` and if a loose object is not found, JGit will open and close a stream to `.git/objects` folder (which can refresh its directory listing, at least on some NFS clients) and retry looking for that loose object. Setting this option to `false` can help to workaround caching issues on NFS, but reduces performance. |
-| `core.trustPackedRefsStat` | `unset` | ⃞ | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the packed-refs file. If `never` JGit will ignore the file attributes of the packed-refs file and always read it. If `always` JGit will trust the file attributes of the packed-refs file and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, except that the packed-refs file is opened and closed before its file attributes are considered. An open/close of the packed-refs file is known to refresh its file attributes, at least on some NFS clients. If `unset`, JGit will use the behavior described in `trustFolderStat`. |
+| `core.trustPackedRefsStat` | `unset` | ⃞ | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the packed-refs file. If `never` JGit will ignore the file attributes of the packed-refs file and always read it. If `always` JGit will trust the file attributes of the packed-refs file and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, except that the packed-refs file is opened and closed before its file attributes are considered. An open/close of the packed-refs file is known to refresh its file attributes, at least on some NFS clients. If `unset`, JGit will use the behavior described in `trustFolderStat`. Note: since 6.10.2, this setting applies during both ref reads and ref updates, but previously only applied during reads.|
| `core.trustLooseRefStat` | `always` | ⃞ | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the loose ref. If `always` JGit will trust the file attributes of the loose ref and its parent directories. `after_open` behaves similar to `always`, except that all parent directories of the loose ref up to the repository root are opened and closed before its file attributes are considered. An open/close of these parent directories is known to refresh the file attributes, at least on some NFS clients. |
| `core.worktree` | Root directory of the working tree if it is not the parent directory of the `.git` directory | ✅ | The path to the root of the working tree. |
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
index 5584f13..4b5566e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
@@ -172,7 +172,7 @@ public void execute(RevWalk walk, ProgressMonitor monitor,
}
packedRefsLock = refdb.lockPackedRefsOrThrow();
- PackedRefList oldPackedList = refdb.refreshPackedRefs();
+ PackedRefList oldPackedList = refdb.getLockedPackedRefs(packedRefsLock);
RefList<Ref> newRefs = applyUpdates(walk, oldPackedList, pending);
if (newRefs == null) {
return;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
index 1b8fd34..52deac8 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -697,7 +697,7 @@ void delete(RefDirectoryUpdate update) throws IOException {
PackedRefList packed = getPackedRefs();
if (packed.contains(name)) {
// Force update our packed-refs snapshot before writing
- packed = refreshPackedRefs();
+ packed = getLockedPackedRefs(lck);
int idx = packed.find(name);
if (0 <= idx) {
commitPackedRefs(lck, packed.remove(idx), packed, true);
@@ -765,7 +765,7 @@ private void pack(Collection<String> refs,
try {
LockFile lck = lockPackedRefsOrThrow();
try {
- PackedRefList oldPacked = refreshPackedRefs();
+ PackedRefList oldPacked = getLockedPackedRefs(lck);
RefList<Ref> newPacked = oldPacked;
// Iterate over all refs to be packed
@@ -948,6 +948,15 @@ else if (0 <= (idx = packed.find(dst.getName())))
}
PackedRefList getPackedRefs() throws IOException {
+ return refreshPackedRefsIfNeeded();
+ }
+
+ PackedRefList getLockedPackedRefs(LockFile packedRefsFileLock) throws IOException {
+ packedRefsFileLock.requireLock();
+ return refreshPackedRefsIfNeeded();
+ }
+
+ PackedRefList refreshPackedRefsIfNeeded() throws IOException {
PackedRefList curList = packedRefs.get();
if (!curList.shouldRefresh()) {
return curList;
@@ -962,23 +971,29 @@ private PackedRefsRefresher getPackedRefsRefresher(PackedRefList curList)
return refresher;
}
// This synchronized is NOT needed for correctness. Instead it is used
- // as a throttling mechanism to ensure that only one "read" thread does
- // the work to refresh the file. In order to avoid stalling writes which
- // must already be serialized and tend to be a bottleneck,
- // the refreshPackedRefs() need not be synchronized.
+ // as a mechanism to try to avoid parallel reads of the same file content
+ // since repeating work, even in parallel, hurts performance.
+ // Unfortunately, this approach can still lead to some unnecessary re-reads
+ // during the "racy" window of the snapshot timestamp.
synchronized (this) {
if (packedRefsRefresher.get() != refresher) {
refresher = packedRefsRefresher.get();
if (refresher != null) {
- // Refresher now guaranteed to have been created after the
- // current thread entered getPackedRefsRefresher(), even if
- // it's currently out of date.
+ // Refresher now guaranteed to have not started refreshing until
+ // after the current thread entered getPackedRefsRefresher(),
+ // even if it's currently out of date. And if the packed-refs
+ // lock is held before calling this method, then it is also
+ // guaranteed to not be out-of date even during the "racy"
+ // window of the snapshot timestamp.
return refresher;
}
}
- refresher = createPackedRefsRefresherAsLatest(curList);
+ refresher = new PackedRefsRefresher(curList);
+ packedRefsRefresher.set(refresher);
}
- return runAndClear(refresher);
+ refresher.run();
+ packedRefsRefresher.compareAndSet(refresher, null);
+ return refresher;
}
private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOException {
@@ -1007,23 +1022,6 @@ private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOExceptio
return true;
}
- PackedRefList refreshPackedRefs() throws IOException {
- return runAndClear(createPackedRefsRefresherAsLatest(packedRefs.get()))
- .getOrThrowIOException();
- }
-
- private PackedRefsRefresher createPackedRefsRefresherAsLatest(PackedRefList curList) {
- PackedRefsRefresher refresher = new PackedRefsRefresher(curList);
- packedRefsRefresher.set(refresher);
- return refresher;
- }
-
- private PackedRefsRefresher runAndClear(PackedRefsRefresher refresher) {
- refresher.run();
- packedRefsRefresher.compareAndSet(refresher, null);
- return refresher;
- }
-
private PackedRefList refreshPackedRefs(PackedRefList curList)
throws IOException {
final PackedRefList newList = readPackedRefs();
