From 049749558e340e1c34abd9b7bf08696c6fd99880 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 21 Feb 2024 15:06:38 -0800 Subject: 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 --- .../jgit/internal/storage/dfs/DfsPackFileTest.java | 41 ++++++ .../jgit/internal/storage/dfs/DfsPackFile.java | 153 +++++++++++++++++---- 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; @@ -122,6 +128,41 @@ public class DfsPackFileTest { assertEquals(800, pack.getIndexedObjectSize(reader, blobId)); } + @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; 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 idxref = cache @@ -1252,31 +1272,11 @@ public final class DfsPackFile extends BlockBasedFile { 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)) { - 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 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); + } + } + } } -- cgit v1.2.3