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

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<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()
+		DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512)
-				.setBlockSize(512)
 				.setBlockLimit(1 << 20));
 		cache = DfsBlockCache.getInstance();
 	}
+
+	private void asyncRun(ExecutorService pool, Callable<?> call) {
+		pool.execute(() -> {
+			try {
+				call.call();
+			} catch (Exception e) {
+				// Ignore.
+			}
+		});
+	}
 }

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 @@ private PackIndex idx(DfsReader ctx) throws IOException {
 
 		Repository.getGlobalListenerList()
 				.dispatch(new BeforeDfsPackIndexLoadedEvent(this));
-
+		try {
-		synchronized (initLock) {
+			DfsStreamKey idxKey = desc.getStreamKey(INDEX);
-			if (index != null) {
+			AtomicBoolean cacheHit = new AtomicBoolean(true);
-				return index;
+			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();
-			try {
+			if (index == null && idx != null) {
-				DfsStreamKey idxKey = desc.getStreamKey(INDEX);
+				index = idx;
-				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;
 			}
+			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) {
+		DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX);
-			if (bitmapIndex != null) {
+		AtomicBoolean cacheHit = new AtomicBoolean(true);
-				return bitmapIndex;
+		DfsBlockCache.Ref<PackBitmapIndex> idxref = cache
-			}
+				.getOrLoadRef(bitmapKey, REF_POSITION, () -> {
-
+					cacheHit.set(false);
-			PackIndex idx = idx(ctx);
+					return loadBitmapIndex(ctx, bitmapKey);
-			PackReverseIndex revidx = getReverseIdx(ctx);
+				});
-			DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX);
+		if (cacheHit.get()) {
-			AtomicBoolean cacheHit = new AtomicBoolean(true);
+			ctx.stats.bitmapCacheHit++;
-			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;
 		}
+		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) {
+		PackIndex idx = idx(ctx);
-			if (reverseIndex != null) {
+		DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex(
-				return reverseIndex;
+				desc.getStreamKey(INDEX));
-			}
+		AtomicBoolean cacheHit = new AtomicBoolean(true);
-
+		DfsBlockCache.Ref<PackReverseIndex> revref = cache.getOrLoadRef(revKey,
-			PackIndex idx = idx(ctx);
+				REF_POSITION, () -> {
-			DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex(
+					cacheHit.set(false);
-					desc.getStreamKey(INDEX));
+					return loadReverseIdx(ctx, revKey, idx);
-			AtomicBoolean cacheHit = new AtomicBoolean(true);
+				});
-			DfsBlockCache.Ref<PackReverseIndex> revref = cache.getOrLoadRef(
+		if (cacheHit.get()) {
-					revKey,
+			ctx.stats.ridxCacheHit++;
-					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;
 		}
+		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<PackReverseIndex> loadReverseIdx(
 				revidx);
 	}
 
-	private DfsBlockCache.Ref<PackBitmapIndex> loadBitmapIndex(
+	private DfsBlockCache.Ref<PackBitmapIndex> loadBitmapIndex(DfsReader ctx,
-			DfsReader ctx,
+			DfsStreamKey bitmapKey) throws IOException {
-			DfsStreamKey bitmapKey,
-			PackIndex idx,
-			PackReverseIndex revidx) 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<PackBitmapIndex> 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;

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(
+	public static PackBitmapIndex open(File idxFile, PackIndex packIndex,
-			File idxFile, PackIndex packIndex, PackReverseIndex reverseIndex)
+			PackReverseIndex reverseIndex)
 			throws IOException {
-		try (SilentFileInputStream fd = new SilentFileInputStream(
+		try (SilentFileInputStream fd = new SilentFileInputStream(idxFile)) {
-				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(
+	public static PackBitmapIndex read(InputStream fd, PackIndex packIndex,
-			InputStream fd, PackIndex packIndex, PackReverseIndex reverseIndex)
+			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 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 <T>
+	 *            the return type which is expected from {@link #get()}
+	 */
+	@FunctionalInterface
+	public interface SupplierWithIOException<T> {
+		/**
+		 * @return result
+		 * @throws IOException
+		 */
+		T get() throws IOException;
+	}
 }

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<StoredBitmap> bitmaps;
 
-	PackBitmapIndexV1(final InputStream fd, PackIndex packIndex,
+	PackBitmapIndexV1(final InputStream fd,
-			PackReverseIndex reverseIndex) throws IOException {
+			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
+		// Read full bitmap from storage first.
-		// bitmap. The object id is an int32 of the nth position sorted by name.
+		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);
+			IdxPositionBitmap xorIdxPositionBitmap = null;
-			StoredBitmap xorBitmap = null;
 			if (xorOffset > 0) {
 				int index = (i - xorOffset);
-				xorBitmap = recentBitmaps[index % recentBitmaps.length];
+				xorIdxPositionBitmap = recentBitmaps[index
-				if (xorBitmap == null)
+						% recentBitmaps.length];
+				if (xorIdxPositionBitmap == null) {
 					throw new IOException(MessageFormat.format(
 							JGitText.get().invalidId,
 							String.valueOf(xorOffset)));
+				}
 			}
-
+			IdxPositionBitmap idxPositionBitmap = new IdxPositionBitmap(
-			StoredBitmap sb = new StoredBitmap(
+					nthObjectId, xorIdxPositionBitmap, bitmap, flags);
-					objectId, bitmap, xorBitmap, flags);
+			idxPositionBitmapList.add(idxPositionBitmap);
-			bitmaps.add(sb);
+			recentBitmaps[i % recentBitmaps.length] = idxPositionBitmap;
-			recentBitmaps[i % recentBitmaps.length] = sb;
 		}
+
+		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;
+		}
+	}
 }