diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java index 4f300bcd8..549e1f469 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java @@ -21,11 +21,17 @@ import java.util.Map; import java.util.stream.LongStream; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import org.eclipse.jgit.internal.storage.pack.PackExt; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.junit.TestRng; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -144,10 +150,57 @@ public void hasCacheHotMap() throws Exception { assertEquals(0, cache.getEvictions()[PackExt.INDEX.getPosition()]); } + @SuppressWarnings("resource") + @Test + public void noConcurrencySerializedReads() throws Exception { + DfsRepositoryDescription repo = new DfsRepositoryDescription("test"); + InMemoryRepository r1 = new InMemoryRepository(repo); + TestRepository repository = new TestRepository<>( + r1); + RevCommit commit = repository.branch("/refs/ref1").commit() + .add("blob1", "blob1").create(); + repository.branch("/refs/ref2").commit().add("blob2", "blob2") + .parent(commit).create(); + + new DfsGarbageCollector(r1).pack(null); + // Reset cache with concurrency Level at 1 i.e. no concurrency. + DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512) + .setBlockLimit(1 << 20).setConcurrencyLevel(1)); + cache = DfsBlockCache.getInstance(); + + DfsReader reader = (DfsReader) r1.newObjectReader(); + ExecutorService pool = Executors.newFixedThreadPool(10); + for (DfsPackFile pack : r1.getObjectDatabase().getPacks()) { + // Only load non-garbage pack with bitmap. + if (pack.isGarbage()) { + continue; + } + asyncRun(pool, () -> pack.getBitmapIndex(reader)); + asyncRun(pool, () -> pack.getPackIndex(reader)); + asyncRun(pool, () -> pack.getBitmapIndex(reader)); + } + + pool.shutdown(); + pool.awaitTermination(500, TimeUnit.MILLISECONDS); + assertTrue("Threads did not complete, likely due to a deadlock.", + pool.isTerminated()); + assertEquals(1, cache.getMissCount()[PackExt.BITMAP_INDEX.ordinal()]); + assertEquals(1, cache.getMissCount()[PackExt.INDEX.ordinal()]); + } + private void resetCache() { - DfsBlockCache.reconfigure(new DfsBlockCacheConfig() - .setBlockSize(512) + DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512) .setBlockLimit(1 << 20)); cache = DfsBlockCache.getInstance(); } + + private void asyncRun(ExecutorService pool, Callable call) { + pool.execute(() -> { + try { + call.call(); + } catch (Exception e) { + // Ignore. + } + }); + } } 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 9be3df3b1..b5bf03fcb 100644 --- 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 @@ -59,13 +59,6 @@ public final class DfsPackFile extends BlockBasedFile { private static final int REC_SIZE = Constants.OBJECT_ID_LENGTH + 8; private static final long REF_POSITION = 0; - /** - * Lock for initialization of {@link #index} and {@link #corruptObjects}. - *

- * This lock ensures only one thread can perform the initialization work. - */ - private final Object initLock = new Object(); - /** Index mapping {@link ObjectId} to position within the pack stream. */ private volatile PackIndex index; @@ -84,6 +77,9 @@ public final class DfsPackFile extends BlockBasedFile { */ private volatile LongList corruptObjects; + /** Lock for {@link #corruptObjects}. */ + private final Object corruptObjectsLock = new Object(); + /** * Construct a reader for an existing, packfile. * @@ -155,35 +151,26 @@ private PackIndex idx(DfsReader ctx) throws IOException { Repository.getGlobalListenerList() .dispatch(new BeforeDfsPackIndexLoadedEvent(this)); - - synchronized (initLock) { - if (index != null) { - return index; + try { + DfsStreamKey idxKey = desc.getStreamKey(INDEX); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref idxref = cache.getOrLoadRef(idxKey, + REF_POSITION, () -> { + cacheHit.set(false); + return loadPackIndex(ctx, idxKey); + }); + if (cacheHit.get()) { + ctx.stats.idxCacheHit++; } - - try { - DfsStreamKey idxKey = desc.getStreamKey(INDEX); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref idxref = cache.getOrLoadRef( - idxKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadPackIndex(ctx, idxKey); - }); - if (cacheHit.get()) { - ctx.stats.idxCacheHit++; - } - PackIndex idx = idxref.get(); - if (index == null && idx != null) { - index = idx; - } - return index; - } catch (IOException e) { - invalid = true; - invalidatingCause = e; - throw e; + PackIndex idx = idxref.get(); + if (index == null && idx != null) { + index = idx; } + return index; + } catch (IOException e) { + invalid = true; + invalidatingCause = e; + throw e; } } @@ -191,7 +178,17 @@ final boolean isGarbage() { return desc.getPackSource() == UNREACHABLE_GARBAGE; } - PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { + /** + * Get the BitmapIndex for this PackFile. + * + * @param ctx + * reader context to support reading from the backing store if + * the index is not already loaded in memory. + * @return the BitmapIndex. + * @throws java.io.IOException + * the bitmap index is not available, or is corrupt. + */ + public PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { if (invalid || isGarbage() || !desc.hasFileExt(BITMAP_INDEX)) { return null; } @@ -200,31 +197,21 @@ PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { return bitmapIndex; } - synchronized (initLock) { - if (bitmapIndex != null) { - return bitmapIndex; - } - - PackIndex idx = idx(ctx); - PackReverseIndex revidx = getReverseIdx(ctx); - DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref idxref = cache.getOrLoadRef( - bitmapKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadBitmapIndex(ctx, bitmapKey, idx, revidx); - }); - if (cacheHit.get()) { - ctx.stats.bitmapCacheHit++; - } - PackBitmapIndex bmidx = idxref.get(); - if (bitmapIndex == null && bmidx != null) { - bitmapIndex = bmidx; - } - return bitmapIndex; + DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref idxref = cache + .getOrLoadRef(bitmapKey, REF_POSITION, () -> { + cacheHit.set(false); + return loadBitmapIndex(ctx, bitmapKey); + }); + if (cacheHit.get()) { + ctx.stats.bitmapCacheHit++; } + PackBitmapIndex bmidx = idxref.get(); + if (bitmapIndex == null && bmidx != null) { + bitmapIndex = bmidx; + } + return bitmapIndex; } PackReverseIndex getReverseIdx(DfsReader ctx) throws IOException { @@ -232,31 +219,23 @@ PackReverseIndex getReverseIdx(DfsReader ctx) throws IOException { return reverseIndex; } - synchronized (initLock) { - if (reverseIndex != null) { - return reverseIndex; - } - - PackIndex idx = idx(ctx); - DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex( - desc.getStreamKey(INDEX)); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref revref = cache.getOrLoadRef( - revKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadReverseIdx(ctx, revKey, idx); - }); - if (cacheHit.get()) { - ctx.stats.ridxCacheHit++; - } - PackReverseIndex revidx = revref.get(); - if (reverseIndex == null && revidx != null) { - reverseIndex = revidx; - } - return reverseIndex; + PackIndex idx = idx(ctx); + DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex( + desc.getStreamKey(INDEX)); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref revref = cache.getOrLoadRef(revKey, + REF_POSITION, () -> { + cacheHit.set(false); + return loadReverseIdx(ctx, revKey, idx); + }); + if (cacheHit.get()) { + ctx.stats.ridxCacheHit++; } + PackReverseIndex revidx = revref.get(); + if (reverseIndex == null && revidx != null) { + reverseIndex = revidx; + } + return reverseIndex; } /** @@ -1003,7 +982,7 @@ boolean isCorrupt(long offset) { private void setCorrupt(long offset) { LongList list = corruptObjects; if (list == null) { - synchronized (initLock) { + synchronized (corruptObjectsLock) { list = corruptObjects; if (list == null) { list = new LongList(); @@ -1066,11 +1045,8 @@ private DfsBlockCache.Ref loadReverseIdx( revidx); } - private DfsBlockCache.Ref loadBitmapIndex( - DfsReader ctx, - DfsStreamKey bitmapKey, - PackIndex idx, - PackReverseIndex revidx) throws IOException { + private DfsBlockCache.Ref loadBitmapIndex(DfsReader ctx, + DfsStreamKey bitmapKey) throws IOException { ctx.stats.readBitmap++; long start = System.nanoTime(); try (ReadableChannel rc = ctx.db.openFile(desc, BITMAP_INDEX)) { @@ -1086,7 +1062,8 @@ private DfsBlockCache.Ref loadBitmapIndex( bs = wantSize; } in = new BufferedInputStream(in, bs); - bmidx = PackBitmapIndex.read(in, idx, revidx); + bmidx = PackBitmapIndex.read(in, () -> idx(ctx), + () -> getReverseIdx(ctx)); } finally { size = rc.position(); ctx.stats.readBitmapIdxBytes += size; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java index beb51dc2e..8401f0718 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java @@ -57,11 +57,10 @@ public abstract class PackBitmapIndex { * @throws CorruptObjectException * the stream does not contain a valid pack bitmap index. */ - public static PackBitmapIndex open( - File idxFile, PackIndex packIndex, PackReverseIndex reverseIndex) + public static PackBitmapIndex open(File idxFile, PackIndex packIndex, + PackReverseIndex reverseIndex) throws IOException { - try (SilentFileInputStream fd = new SilentFileInputStream( - idxFile)) { + try (SilentFileInputStream fd = new SilentFileInputStream(idxFile)) { try { return read(fd, packIndex, reverseIndex); } catch (IOException ioe) { @@ -94,10 +93,39 @@ public static PackBitmapIndex open( * @throws CorruptObjectException * the stream does not contain a valid pack bitmap index. */ - public static PackBitmapIndex read( - InputStream fd, PackIndex packIndex, PackReverseIndex reverseIndex) + public static PackBitmapIndex read(InputStream fd, PackIndex packIndex, + PackReverseIndex reverseIndex) throws IOException { + return new PackBitmapIndexV1(fd, () -> packIndex, () -> reverseIndex); + } + + /** + * Read an existing pack bitmap index file from a buffered stream. + *

+ * The format of the file will be automatically detected and a proper access + * implementation for that format will be constructed and returned to the + * caller. The file may or may not be held open by the returned instance. + * + * @param fd + * stream to read the bitmap index file from. The stream must be + * buffered as some small IOs are performed against the stream. + * The caller is responsible for closing the stream. + * @param packIndexSupplier + * the supplier for pack index for the corresponding pack file. + * @param reverseIndexSupplier + * the supplier for pack reverse index for the corresponding pack + * file. + * @return a copy of the index in-memory. + * @throws java.io.IOException + * the stream cannot be read. + * @throws CorruptObjectException + * the stream does not contain a valid pack bitmap index. + */ + public static PackBitmapIndex read(InputStream fd, + SupplierWithIOException packIndexSupplier, + SupplierWithIOException reverseIndexSupplier) throws IOException { - return new PackBitmapIndexV1(fd, packIndex, reverseIndex); + return new PackBitmapIndexV1(fd, packIndexSupplier, + reverseIndexSupplier); } /** Footer checksum applied on the bottom of the pack file. */ @@ -121,7 +149,8 @@ public static PackBitmapIndex read( * @throws java.lang.IllegalArgumentException * when the item is not found. */ - public abstract ObjectId getObject(int position) throws IllegalArgumentException; + public abstract ObjectId getObject(int position) + throws IllegalArgumentException; /** * Returns a bitmap containing positions for objects that have the given Git @@ -161,4 +190,19 @@ public abstract EWAHCompressedBitmap ofObjectType( * @return the number of bitmaps in this bitmap index. */ public abstract int getBitmapCount(); + + /** + * Supplier that propagates IOException. + * + * @param + * the return type which is expected from {@link #get()} + */ + @FunctionalInterface + public interface SupplierWithIOException { + /** + * @return result + * @throws IOException + */ + T get() throws IOException; + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java index b7d241f3f..6846e3bca 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java @@ -14,8 +14,11 @@ import java.io.IOException; import java.io.InputStream; import java.text.MessageFormat; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; @@ -46,11 +49,13 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { private final ObjectIdOwnerMap bitmaps; - PackBitmapIndexV1(final InputStream fd, PackIndex packIndex, - PackReverseIndex reverseIndex) throws IOException { + PackBitmapIndexV1(final InputStream fd, + SupplierWithIOException packIndexSupplier, + SupplierWithIOException reverseIndexSupplier) + throws IOException { + // An entry is object id, xor offset, flag byte, and a length encoded + // bitmap. The object id is an int32 of the nth position sorted by name. super(new ObjectIdOwnerMap()); - this.packIndex = packIndex; - this.reverseIndex = reverseIndex; this.bitmaps = getBitmaps(); final byte[] scratch = new byte[32]; @@ -97,10 +102,10 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { this.blobs = readBitmap(dataInput); this.tags = readBitmap(dataInput); - // An entry is object id, xor offset, flag byte, and a length encoded - // bitmap. The object id is an int32 of the nth position sorted by name. + // Read full bitmap from storage first. + List idxPositionBitmapList = new ArrayList<>(); // The xor offset is a single byte offset back in the list of entries. - StoredBitmap[] recentBitmaps = new StoredBitmap[MAX_XOR_OFFSET]; + IdxPositionBitmap[] recentBitmaps = new IdxPositionBitmap[MAX_XOR_OFFSET]; for (int i = 0; i < (int) numEntries; i++) { IO.readFully(fd, scratch, 0, 6); int nthObjectId = NB.decodeInt32(scratch, 0); @@ -108,38 +113,58 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { int flags = scratch[5]; EWAHCompressedBitmap bitmap = readBitmap(dataInput); - if (nthObjectId < 0) + if (nthObjectId < 0) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(nthObjectId))); - if (xorOffset < 0) + } + if (xorOffset < 0) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(xorOffset))); - if (xorOffset > MAX_XOR_OFFSET) + } + if (xorOffset > MAX_XOR_OFFSET) { throw new IOException(MessageFormat.format( JGitText.get().expectedLessThanGot, String.valueOf(MAX_XOR_OFFSET), String.valueOf(xorOffset))); - if (xorOffset > i) + } + if (xorOffset > i) { throw new IOException(MessageFormat.format( JGitText.get().expectedLessThanGot, String.valueOf(i), String.valueOf(xorOffset))); - - ObjectId objectId = packIndex.getObjectId(nthObjectId); - StoredBitmap xorBitmap = null; + } + IdxPositionBitmap xorIdxPositionBitmap = null; if (xorOffset > 0) { int index = (i - xorOffset); - xorBitmap = recentBitmaps[index % recentBitmaps.length]; - if (xorBitmap == null) + xorIdxPositionBitmap = recentBitmaps[index + % recentBitmaps.length]; + if (xorIdxPositionBitmap == null) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(xorOffset))); + } } - - StoredBitmap sb = new StoredBitmap( - objectId, bitmap, xorBitmap, flags); - bitmaps.add(sb); - recentBitmaps[i % recentBitmaps.length] = sb; + IdxPositionBitmap idxPositionBitmap = new IdxPositionBitmap( + nthObjectId, xorIdxPositionBitmap, bitmap, flags); + idxPositionBitmapList.add(idxPositionBitmap); + recentBitmaps[i % recentBitmaps.length] = idxPositionBitmap; } + + this.packIndex = packIndexSupplier.get(); + for (int i = 0; i < idxPositionBitmapList.size(); ++i) { + IdxPositionBitmap idxPositionBitmap = idxPositionBitmapList.get(i); + ObjectId objectId = packIndex + .getObjectId(idxPositionBitmap.nthObjectId); + StoredBitmap sb = new StoredBitmap(objectId, + idxPositionBitmap.bitmap, + idxPositionBitmap.getXorStoredBitmap(), + idxPositionBitmap.flags); + // Save the StoredBitmap for a possible future XorStoredBitmap + // reference. + idxPositionBitmap.sb = sb; + bitmaps.add(sb); + } + + this.reverseIndex = reverseIndexSupplier.get(); } /** {@inheritDoc} */ @@ -214,4 +239,34 @@ private static EWAHCompressedBitmap readBitmap(DataInput dataInput) bitmap.deserialize(dataInput); return bitmap; } + + /** + * Temporary holder of object position in pack index and other metadata for + * {@code StoredBitmap}. + */ + private static final class IdxPositionBitmap { + int nthObjectId; + + IdxPositionBitmap xorIdxPositionBitmap; + + EWAHCompressedBitmap bitmap; + + int flags; + + StoredBitmap sb; + + IdxPositionBitmap(int nthObjectId, + @Nullable IdxPositionBitmap xorIdxPositionBitmap, + EWAHCompressedBitmap bitmap, int flags) { + this.nthObjectId = nthObjectId; + this.xorIdxPositionBitmap = xorIdxPositionBitmap; + this.bitmap = bitmap; + this.flags = flags; + } + + StoredBitmap getXorStoredBitmap() { + return xorIdxPositionBitmap == null ? null + : xorIdxPositionBitmap.sb; + } + } }