Avoid reading packed-refs concurrently sometimes
If the packed-refs file has not changed since a thread has started
reading and parsing it, do not bother to re-read and re-parse it from
another thread as this will just create extra load and likely slow down
the current reading/parsing thread only to in theory return the same
result. Of course, jgit uses timestamps to identify that a file has not
changed since the current thread has started refreshing it, without
rereading it, so this optimization only comes into play if some form of
trusting the packed-refs stats is enabled, otherwise the behavior before
and after should be similar.
This duplicate parsing does not synchronize the refresh, and it still
allows for more then one thread to read/parse the packed-refs file in
parallel, but only when the file has changed since the current refresh
started. This change does however synchronize creating a
PackedRefsRefresher which for just long enough to ensure that only one
is created when only one is needed. This change should result in better
performance all while maintaining the same data consistency with respect
to requests as before.
Change-Id: Ic486e0975a67c6ea493493083411255953fba72e
Signed-off-by: Martin Fick <mfick@nvidia.com>
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 321584b..1b8fd34 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
@@ -48,6 +48,8 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.ReentrantLock;
@@ -145,6 +147,9 @@ public class RefDirectory extends RefDatabase {
/** Immutable sorted list of packed references. */
final AtomicReference<PackedRefList> packedRefs = new AtomicReference<>();
+ private final AtomicReference<PackedRefsRefresher> packedRefsRefresher =
+ new AtomicReference<>();
+
/**
* Lock for coordinating operations within a single process that may contend
* on the {@code packed-refs} file.
@@ -943,8 +948,40 @@ else if (0 <= (idx = packed.find(dst.getName())))
}
PackedRefList getPackedRefs() throws IOException {
- final PackedRefList curList = packedRefs.get();
+ PackedRefList curList = packedRefs.get();
+ if (!curList.shouldRefresh()) {
+ return curList;
+ }
+ return getPackedRefsRefresher(curList).getOrThrowIOException();
+ }
+ private PackedRefsRefresher getPackedRefsRefresher(PackedRefList curList)
+ throws IOException {
+ PackedRefsRefresher refresher = packedRefsRefresher.get();
+ if (refresher != null && !refresher.shouldRefresh()) {
+ 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.
+ 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.
+ return refresher;
+ }
+ }
+ refresher = createPackedRefsRefresherAsLatest(curList);
+ }
+ return runAndClear(refresher);
+ }
+
+ private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOException {
switch (trustPackedRefsStat) {
case NEVER:
break;
@@ -957,23 +994,34 @@ PackedRefList getPackedRefs() throws IOException {
}
//$FALL-THROUGH$
case ALWAYS:
- if (!curList.snapshot.isModified(packedRefsFile)) {
- return curList;
+ if (!snapshot.isModified(packedRefsFile)) {
+ return false;
}
break;
case UNSET:
- if (trustFolderStat
- && !curList.snapshot.isModified(packedRefsFile)) {
- return curList;
+ if (trustFolderStat && !snapshot.isModified(packedRefsFile)) {
+ return false;
}
break;
}
-
- return refreshPackedRefs(curList);
+ return true;
}
PackedRefList refreshPackedRefs() throws IOException {
- return refreshPackedRefs(packedRefs.get());
+ 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)
@@ -997,7 +1045,7 @@ private PackedRefList readPackedRefs() throws IOException {
new DigestInputStream(
new FileInputStream(f), digest),
UTF_8))) {
- return new PackedRefList(parsePackedRefs(br),
+ return new NonEmptyPackedRefList(parsePackedRefs(br),
snapshot,
ObjectId.fromRaw(digest.digest()));
}
@@ -1103,7 +1151,7 @@ protected void writeFile(String name, byte[] content)
throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name));
byte[] digest = Constants.newMessageDigest().digest(content);
- PackedRefList newPackedList = new PackedRefList(
+ PackedRefList newPackedList = new NonEmptyPackedRefList(
refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest));
packedRefs.compareAndSet(oldPackedList, newPackedList);
if (changed) {
@@ -1452,21 +1500,57 @@ static void sleep(long ms) throws InterruptedIOException {
}
static class PackedRefList extends RefList<Ref> {
-
- private final FileSnapshot snapshot;
-
private final ObjectId id;
- private PackedRefList(RefList<Ref> src, FileSnapshot s, ObjectId i) {
+ PackedRefList() {
+ this(RefList.emptyList(), ObjectId.zeroId());
+ }
+
+ protected PackedRefList(RefList<Ref> src, ObjectId id) {
super(src);
- snapshot = s;
- id = i;
+ this.id = id;
+ }
+
+ public boolean shouldRefresh() throws IOException {
+ return true;
}
}
- private static final PackedRefList NO_PACKED_REFS = new PackedRefList(
- RefList.emptyList(), FileSnapshot.MISSING_FILE,
- ObjectId.zeroId());
+ private static final PackedRefList NO_PACKED_REFS = new PackedRefList();
+
+ private class NonEmptyPackedRefList extends PackedRefList {
+ private final FileSnapshot snapshot;
+
+ private NonEmptyPackedRefList(RefList<Ref> src, FileSnapshot s, ObjectId id) {
+ super(src, id);
+ snapshot = s;
+ }
+
+ @Override
+ public boolean shouldRefresh() throws IOException {
+ return shouldRefreshPackedRefs(snapshot);
+ }
+ }
+
+ private class PackedRefsRefresher extends FutureTask<PackedRefList> {
+ private final FileSnapshot snapshot = FileSnapshot.save(packedRefsFile);
+
+ public PackedRefsRefresher(PackedRefList curList) {
+ super(() -> refreshPackedRefs(curList));
+ }
+
+ public PackedRefList getOrThrowIOException() throws IOException {
+ try {
+ return get();
+ } catch (ExecutionException | InterruptedException e) {
+ throw new IOException(e);
+ }
+ }
+
+ public boolean shouldRefresh() throws IOException {
+ return shouldRefreshPackedRefs(snapshot);
+ }
+ }
private static LooseSymbolicRef newSymbolicRef(FileSnapshot snapshot,
String name, String target) {
