DfsPackFile: Do not set primery index local ref from cache callback

DfsPackFile assumes the indices are in pack streams, but we would like
to consider other formats and storage. Currently, the local ref in the
DfsPackFile to the index is set in the cache loading callback, which
prevents abstracting the loading.

Reorganize the code so: the loadPackIndex function just parses the bytes
returning a reference and the caller sets the loaded index in the local
ref and DfsBlockCache.

We will follow this pattern with other indices in follow-up
changes. Note that although DfsPackFile is used only in one thread,
the loading in DfsBlockCache can happen from multiple threads
concurrently and we want to keep only one ref around.

Change-Id: Ie99de6c80784225484c0f99c51caa44c6a372c45

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
index 5cc2a57abab7282c11dabec20c30c5b11e62c8d3..3e4d4d300ca1cae215e862a5c2640c129d3808e7 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
@@ -29,6 +29,7 @@ import java.nio.channels.Channels;
 import java.text.MessageFormat;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
@@ -196,18 +197,24 @@ public final class DfsPackFile extends BlockBasedFile {
                                .dispatch(new BeforeDfsPackIndexLoadedEvent(this));
                try {
                        DfsStreamKey idxKey = desc.getStreamKey(INDEX);
-                       AtomicBoolean cacheHit = new AtomicBoolean(true);
-                       DfsBlockCache.Ref<PackIndex> idxref = cache.getOrLoadRef(idxKey,
+                       // Keep the value parsed in the loader, in case the Ref<> is
+                       // nullified in ClockBlockCacheTable#reserveSpace
+                       // before we read its value.
+                       AtomicReference<PackIndex> loadedRef = new AtomicReference<>(null);
+                       DfsBlockCache.Ref<PackIndex> cachedRef = cache.getOrLoadRef(idxKey,
                                        REF_POSITION, () -> {
-                                               cacheHit.set(false);
-                                               return loadPackIndex(ctx, idxKey);
+                                               RefWithSize<PackIndex> idx = loadPackIndex(ctx);
+                                               loadedRef.set(idx.ref);
+                                               return new DfsBlockCache.Ref<>(idxKey, REF_POSITION,
+                                                               idx.size, idx.ref);
                                        });
-                       if (cacheHit.get()) {
+                       if (loadedRef.get() == null) {
                                ctx.stats.idxCacheHit++;
                        }
-                       PackIndex idx = idxref.get();
-                       if (index == null && idx != null) {
-                               index = idx;
+                       index = cachedRef.get() != null ? cachedRef.get() : loadedRef.get();
+                       if (index == null) {
+                               throw new IOException(
+                                               "Couldn't get a reference to the primary index"); //$NON-NLS-1$
                        }
                        ctx.emitIndexLoad(desc, INDEX, index);
                        return index;
@@ -1210,20 +1217,15 @@ public final class DfsPackFile extends BlockBasedFile {
                }
        }
 
-       private DfsBlockCache.Ref<PackIndex> loadPackIndex(
-                       DfsReader ctx, DfsStreamKey idxKey) throws IOException {
+       private RefWithSize<PackIndex> loadPackIndex(DfsReader ctx)
+                       throws IOException {
                try {
                        ctx.stats.readIdx++;
                        long start = System.nanoTime();
                        try (ReadableChannel rc = ctx.db.openFile(desc, INDEX)) {
                                PackIndex idx = PackIndex.read(alignTo8kBlocks(rc));
                                ctx.stats.readIdxBytes += rc.position();
-                               index = idx;
-                               return new DfsBlockCache.Ref<>(
-                                               idxKey,
-                                               REF_POSITION,
-                                               idx.getObjectCount() * REC_SIZE,
-                                               idx);
+                               return new RefWithSize<>(idx, idx.getObjectCount() * REC_SIZE);
                        } finally {
                                ctx.stats.readIdxMicros += elapsedMicros(start);
                        }
@@ -1449,4 +1451,13 @@ public final class DfsPackFile extends BlockBasedFile {
                        }
                }
        }
+
+       private static final class RefWithSize<V> {
+               final V ref;
+               final long size;
+               RefWithSize(V ref, long size) {
+                       this.ref = ref;
+                       this.size = size;
+               }
+       }
 }