diff options
author | Alina Djamankulova <adjama@google.com> | 2021-10-13 13:16:41 -0700 |
---|---|---|
committer | Alina Djamankulova <adjama@google.com> | 2021-10-19 15:01:59 -0700 |
commit | 3b960ae72daa76d0d8f85c8a0d74575af0c48b24 (patch) | |
tree | 2bc216f9f436b14060bca0276db748ccdd4d066b | |
parent | c0436a3a0ab197b4e9639c0f2d94a386dc91d12a (diff) | |
download | jgit-3b960ae72daa76d0d8f85c8a0d74575af0c48b24.tar.gz jgit-3b960ae72daa76d0d8f85c8a0d74575af0c48b24.zip |
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 <adjama@google.com>
Change-Id: Ic6d9c5a4a254628636aa98a5008447a27a003f69
4 files changed, 248 insertions, 119 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 4f300bcd8d..549e1f4696 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.List; 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 class DfsBlockCacheTest { 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<InMemoryRepository> 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 9be3df3b17..b5bf03fcbb 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}. - * <p> - * 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 @@ public final class DfsPackFile extends BlockBasedFile { 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<PackIndex> 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<PackIndex> 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 @@ public final class DfsPackFile extends BlockBasedFile { 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 @@ public final class DfsPackFile extends BlockBasedFile { 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<PackBitmapIndex> 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<PackBitmapIndex> 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 @@ public final class DfsPackFile extends BlockBasedFile { 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<PackReverseIndex> 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<PackReverseIndex> 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 @@ public final class DfsPackFile extends BlockBasedFile { 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 @@ public final class DfsPackFile extends BlockBasedFile { revidx); } - private DfsBlockCache.Ref<PackBitmapIndex> loadBitmapIndex( - DfsReader ctx, - DfsStreamKey bitmapKey, - PackIndex idx, - PackReverseIndex revidx) throws IOException { + private DfsBlockCache.Ref<PackBitmapIndex> 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 @@ public final class DfsPackFile extends BlockBasedFile { 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 beb51dc2eb..8401f0718a 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 abstract class PackBitmapIndex { * @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. + * <p> + * 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<PackIndex> packIndexSupplier, + SupplierWithIOException<PackReverseIndex> 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 abstract class PackBitmapIndex { * @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 class PackBitmapIndex { * @return the number of bitmaps in this bitmap index. */ public abstract int getBitmapCount(); + + /** + * Supplier that propagates IOException. + * + * @param <T> + * the return type which is expected from {@link #get()} + */ + @FunctionalInterface + public interface SupplierWithIOException<T> { + /** + * @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 b7d241f3f8..6846e3bcad 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.DataInput; 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<StoredBitmap> bitmaps; - PackBitmapIndexV1(final InputStream fd, PackIndex packIndex, - PackReverseIndex reverseIndex) throws IOException { + PackBitmapIndexV1(final InputStream fd, + SupplierWithIOException<PackIndex> packIndexSupplier, + SupplierWithIOException<PackReverseIndex> 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<StoredBitmap>()); - 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<IdxPositionBitmap> 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))); + } } + IdxPositionBitmap idxPositionBitmap = new IdxPositionBitmap( + nthObjectId, xorIdxPositionBitmap, bitmap, flags); + idxPositionBitmapList.add(idxPositionBitmap); + recentBitmaps[i % recentBitmaps.length] = idxPositionBitmap; + } - StoredBitmap sb = new StoredBitmap( - objectId, bitmap, xorBitmap, flags); + 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); - recentBitmaps[i % recentBitmaps.length] = sb; } + + this.reverseIndex = reverseIndexSupplier.get(); } /** {@inheritDoc} */ @@ -214,4 +239,34 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { 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; + } + } } |