From 3b960ae72daa76d0d8f85c8a0d74575af0c48b24 Mon Sep 17 00:00:00 2001 From: Alina Djamankulova Date: Wed, 13 Oct 2021 13:16:41 -0700 Subject: [PATCH] DFS block cache: fix lock issue and support parallel index loading This change is a fix to http://git.eclipse.org/r/c/jgit/jgit/+/183562 that was reverted in http://git.eclipse.org/r/c/jgit/jgit/+/184978 due to deadlocks. Separate locks in DfsBlockFile are removed to rely on getting value from DfsBlockCache with region locking in place. With this change bitmap index creation is not blocked on index and reverse index full initialization in DfsPackFile. Now bitmap index and index could be read from storage in parallel in separate threads. A unit test is added for parallel index loading. Signed-off-by: Alina Djamankulova Change-Id: Ic6d9c5a4a254628636aa98a5008447a27a003f69 --- .../storage/dfs/DfsBlockCacheTest.java | 57 ++++++- .../internal/storage/dfs/DfsPackFile.java | 157 ++++++++---------- .../storage/file/PackBitmapIndex.java | 60 ++++++- .../storage/file/PackBitmapIndexV1.java | 97 ++++++++--- 4 files changed, 250 insertions(+), 121 deletions(-) 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; + } + } }