DfsPackFile: Abstract the bitmap loading to support other backends

Current code reads the bitmap index from the pack extension and loads
all bitmaps into memory, with its IO and memory cost. We could
consider to store the bitmaps on e.g. a database and load them on
demand.

Abstract the loading of the PackBitmapIndex in an interface that can
be implemented with other backends.

Change-Id: Ib5f64d05954708ea5325feea7088a8df229b36a5

2 files changed, 168 insertions, 26 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackFileTest.java
index 77e5b7cb14..44694acc8d 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackFileTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackFileTest.java
@@ -14,6 +14,8 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_MIN_BYTES_OBJ_SIZE
 import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_PACK_SECTION;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayOutputStream;
@@ -24,14 +26,18 @@ import java.util.zip.Deflater;
 
 import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
 import org.eclipse.jgit.internal.storage.dfs.DfsReader.PackLoadListener;
+import org.eclipse.jgit.internal.storage.file.PackBitmapIndex;
 import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.pack.PackOutputStream;
 import org.eclipse.jgit.internal.storage.pack.PackWriter;
 import org.eclipse.jgit.junit.JGitTestUtil;
 import org.eclipse.jgit.junit.TestRng;
+import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -123,6 +129,41 @@ public class DfsPackFileTest {
 	}
 
 	@Test
+	public void testGetBitmapIndex() throws IOException {
+		bypassCache = false;
+		clearCache = true;
+		ObjectId objectId = setupPack(512, 800);
+
+		// Add a ref for GC
+		BatchRefUpdate batchRefUpdate = db.getRefDatabase().newBatchUpdate();
+		batchRefUpdate.addCommand(new ReceiveCommand(ObjectId.zeroId(),
+				objectId, "refs/heads/master"));
+		try (RevWalk rw = new RevWalk(db)) {
+			batchRefUpdate.execute(rw, NullProgressMonitor.INSTANCE);
+		}
+		DfsGarbageCollector gc = new DfsGarbageCollector(db);
+		gc.pack(NullProgressMonitor.INSTANCE);
+
+		DfsReader reader = db.getObjectDatabase().newReader();
+		PackBitmapIndex bitmapIndex = db.getObjectDatabase().getPacks()[0]
+				.getBitmapIndex(reader);
+		assertNotNull(bitmapIndex);
+		assertEquals(1, bitmapIndex.getObjectCount());
+	}
+
+	@Test
+	public void testGetBitmapIndex_noBitmaps() throws IOException {
+		bypassCache = false;
+		clearCache = true;
+		setupPack(512, 800);
+
+		DfsReader reader = db.getObjectDatabase().newReader();
+		PackBitmapIndex bitmapIndex = db.getObjectDatabase().getPacks()[0]
+				.getBitmapIndex(reader);
+		assertNull(bitmapIndex);
+	}
+
+	@Test
 	public void testLoadObjectSizeIndex_noIndex() throws IOException {
 		bypassCache = false;
 		clearCache = true;
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 e0f0e13553..42b1d235bf 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
@@ -65,8 +65,11 @@ import org.eclipse.jgit.util.LongList;
  */
 public final class DfsPackFile extends BlockBasedFile {
 	private static final int REC_SIZE = Constants.OBJECT_ID_LENGTH + 8;
+
 	private static final long REF_POSITION = 0;
 
+	private static final PackBitmapIndexLoader DEFAULT_BITMAP_LOADER = new StreamPackBitmapIndexLoader();
+
 	/** Index mapping {@link ObjectId} to position within the pack stream. */
 	private volatile PackIndex index;
 
@@ -79,6 +82,8 @@ public final class DfsPackFile extends BlockBasedFile {
 	/** Index of compressed commit graph mapping entire object graph. */
 	private volatile CommitGraph commitGraph;
 
+	private final PackBitmapIndexLoader bitmapLoader;
+
 	/** Index by size */
 	private boolean objectSizeIndexLoadAttempted;
 
@@ -105,6 +110,10 @@ public final class DfsPackFile extends BlockBasedFile {
 	 *            description of the pack within the DFS.
 	 */
 	DfsPackFile(DfsBlockCache cache, DfsPackDescription desc) {
+		this(cache, desc, DEFAULT_BITMAP_LOADER);
+	}
+
+	DfsPackFile(DfsBlockCache cache, DfsPackDescription desc, PackBitmapIndexLoader bitmapLoader) {
 		super(cache, desc, PACK);
 
 		int bs = desc.getBlockSize(PACK);
@@ -114,6 +123,8 @@ public final class DfsPackFile extends BlockBasedFile {
 
 		long sz = desc.getFileSize(PACK);
 		length = sz > 0 ? sz : -1;
+
+		this.bitmapLoader = bitmapLoader;
 	}
 
 	/**
@@ -206,7 +217,7 @@ public final class DfsPackFile extends BlockBasedFile {
 	 *             the bitmap index is not available, or is corrupt.
 	 */
 	public PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException {
-		if (invalid || isGarbage() || !desc.hasFileExt(BITMAP_INDEX)) {
+		if (invalid || isGarbage() || !bitmapLoader.hasBitmaps(desc)) {
 			return null;
 		}
 
@@ -214,6 +225,15 @@ public final class DfsPackFile extends BlockBasedFile {
 			return bitmapIndex;
 		}
 
+		if (!bitmapLoader.keepInDfs(desc)) {
+			PackBitmapIndexLoader.LoadResult result = bitmapLoader
+					.loadPackBitmapIndex(ctx, this);
+			if (bitmapIndex == null && result.bitmapIndex != null) {
+				bitmapIndex = result.bitmapIndex;
+			}
+			return bitmapIndex;
+		}
+
 		DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX);
 		AtomicBoolean cacheHit = new AtomicBoolean(true);
 		DfsBlockCache.Ref<PackBitmapIndex> idxref = cache
@@ -1252,31 +1272,11 @@ public final class DfsPackFile extends BlockBasedFile {
 	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)) {
-			long size;
-			PackBitmapIndex bmidx;
-			try {
-				bmidx = PackBitmapIndex.read(alignTo8kBlocks(rc),
-						() -> idx(ctx), () -> getReverseIdx(ctx),
-						ctx.getOptions().shouldLoadRevIndexInParallel());
-			} finally {
-				size = rc.position();
-				ctx.stats.readBitmapIdxBytes += size;
-				ctx.stats.readBitmapIdxMicros += elapsedMicros(start);
-			}
-			bitmapIndex = bmidx;
-			return new DfsBlockCache.Ref<>(
-					bitmapKey, REF_POSITION, size, bmidx);
-		} catch (EOFException e) {
-			throw new IOException(MessageFormat.format(
-					DfsText.get().shortReadOfIndex,
-					desc.getFileName(BITMAP_INDEX)), e);
-		} catch (IOException e) {
-			throw new IOException(MessageFormat.format(
-					DfsText.get().cannotReadIndex,
-					desc.getFileName(BITMAP_INDEX)), e);
-		}
+		PackBitmapIndexLoader.LoadResult result = bitmapLoader
+				.loadPackBitmapIndex(ctx, this);
+		bitmapIndex = result.bitmapIndex;
+		return new DfsBlockCache.Ref<>(bitmapKey, REF_POSITION,
+				result.bytesRead, result.bitmapIndex);
 	}
 
 	private DfsBlockCache.Ref<CommitGraph> loadCommitGraph(DfsReader ctx,
@@ -1317,4 +1317,105 @@ public final class DfsPackFile extends BlockBasedFile {
 		}
 		return new BufferedInputStream(in, bs);
 	}
+
+	/**
+	 * Loads the PackBitmapIndex associated with this packfile
+	 */
+	public interface PackBitmapIndexLoader {
+		/**
+		 * Does this pack has bitmaps associated?
+		 *
+		 * @param desc
+		 *            the pack
+		 * @return true if the pack has bitmaps
+		 */
+		boolean hasBitmaps(DfsPackDescription desc);
+
+		/**
+		 * If the returned instance must be kept in DFS cache
+		 *
+		 * It should be true when the instance is expensive to load and can be
+		 * reused.
+		 *
+		 * @param desc
+		 *            the pack
+		 * @return true if the returned bitmap index should be kept in DFS
+		 */
+		boolean keepInDfs(DfsPackDescription desc);
+
+		/**
+		 * Returns a PackBitmapIndex for this pack, if the pack has bitmaps
+		 * associated.
+		 *
+		 * @param ctx
+		 *            the reader
+		 * @param pack
+		 *            the pack
+		 * @return the pack bitmap index and bytes size (when applicable)
+		 * @throws IOException
+		 *             error accessing storage
+		 */
+		LoadResult loadPackBitmapIndex(DfsReader ctx, DfsPackFile pack)
+				throws IOException;
+
+		/**
+		 * The instance of the pack bitmap index and the amount of bytes loaded.
+		 *
+		 * The bytes can be 0, if the implementation doesn't do any initial
+		 * loading.
+		 */
+		class LoadResult {
+			final PackBitmapIndex bitmapIndex;
+
+			final long bytesRead;
+
+			LoadResult(PackBitmapIndex packBitmapIndex, long bytesRead) {
+				this.bitmapIndex = packBitmapIndex;
+				this.bytesRead = bytesRead;
+			}
+		}
+	}
+
+	/**
+	 * Load the packbitmapindex from the BITMAP_INDEX pack extension
+	 */
+	private static final class StreamPackBitmapIndexLoader implements PackBitmapIndexLoader {
+		@Override
+		public boolean hasBitmaps(DfsPackDescription desc) {
+			return desc.hasFileExt(BITMAP_INDEX);
+		}
+
+		@Override
+		public boolean keepInDfs(DfsPackDescription desc) {
+			return true;
+		}
+
+		@Override
+		public LoadResult loadPackBitmapIndex(DfsReader ctx, DfsPackFile pack)
+				throws IOException {
+			DfsPackDescription desc = pack.getPackDescription();
+			try (ReadableChannel rc = ctx.db.openFile(desc, BITMAP_INDEX)) {
+				long size;
+				PackBitmapIndex bmidx;
+				try {
+					bmidx = PackBitmapIndex.read(alignTo8kBlocks(rc),
+							() -> pack.idx(ctx), () -> pack.getReverseIdx(ctx),
+							ctx.getOptions().shouldLoadRevIndexInParallel());
+				} finally {
+					size = rc.position();
+				}
+				return new LoadResult(bmidx, size);
+			} catch (EOFException e) {
+				throw new IOException(
+						MessageFormat.format(DfsText.get().shortReadOfIndex,
+								desc.getFileName(BITMAP_INDEX)),
+						e);
+			} catch (IOException e) {
+				throw new IOException(
+						MessageFormat.format(DfsText.get().cannotReadIndex,
+								desc.getFileName(BITMAP_INDEX)),
+						e);
+			}
+		}
+	}
 }