From 5dae4a8351287526b323547f942da72b9e00f456 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Wed, 12 Feb 2025 19:07:42 -0800 Subject: Pack: no longer set invalid in openFail() The intention of the 'invalidate' argument in openFail() is to invalidate the Pack in certain situations. However, after moving doOpen() to a lock instead of using synchronized, the invalidation approach could also incorrectly mark an already invalid Pack valid, which was never the intention since previously invalid would only ever get set to false if it already was false. Fix this by never setting invalid in openFail(), instead set invalid explicitly before calling openFail when needed. This makes the intent clearer, and aligns better with all the existing comments already trying to explain the boolean (and some of them become obvious enough now that the comment is deleted or shortened). This is also likely faster than adding a conditional in openFail() to make 'invalidate' work properly. Change-Id: Ie6182103ee2994724cb5cb0b64030fedba84b637 Signed-off-by: Martin Fick --- .../eclipse/jgit/internal/storage/file/Pack.java | 30 ++++++++++++---------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 1a7b5de1d7..122d800582 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -694,7 +694,7 @@ public class Pack implements Iterable { private void doOpen() throws IOException { if (invalid) { - openFail(true, invalidatingCause); + openFail(invalidatingCause); throw new PackInvalidException(packFile, invalidatingCause); } try { @@ -705,39 +705,41 @@ public class Pack implements Iterable { } } catch (InterruptedIOException e) { // don't invalidate the pack, we are interrupted from another thread - openFail(false, e); + openFail(e); throw e; } catch (FileNotFoundException fn) { - // don't invalidate the pack if opening an existing file failed - // since it may be related to a temporary lack of resources (e.g. - // max open files) - openFail(!packFile.exists(), fn); + if (!packFile.exists()) { + // Failure to open an existing file may be related to a temporary lack of resources + // (e.g. max open files) + invalid = true; + } + openFail(fn); throw fn; } catch (EOFException | AccessDeniedException | NoSuchFileException | CorruptObjectException | NoPackSignatureException | PackMismatchException | UnpackException | UnsupportedPackIndexVersionException | UnsupportedPackVersionException pe) { - // exceptions signaling permanent problems with a pack - openFail(true, pe); + invalid = true; // exceptions signaling permanent problems with a pack + openFail(pe); throw pe; } catch (IOException ioe) { - // mark this packfile as invalid when NFS stale file handle error - // occur - openFail(FileUtils.isStaleFileHandleInCausalChain(ioe), ioe); + if (FileUtils.isStaleFileHandleInCausalChain(ioe)) { + invalid = true; + } + openFail(ioe); throw ioe; } catch (RuntimeException ge) { // generic exceptions could be transient so we should not mark the // pack invalid to avoid false MissingObjectExceptions - openFail(false, ge); + openFail(ge); throw ge; } } - private void openFail(boolean invalidate, Exception cause) { + private void openFail(Exception cause) { activeWindows = 0; activeCopyRawData = 0; - invalid = invalidate; invalidatingCause = cause; doClose(); } -- cgit v1.2.3 From fab9b3d961393cd2f0e9e994b0c2827d3f3ac4ee Mon Sep 17 00:00:00 2001 From: Antonio Barone Date: Thu, 13 Feb 2025 13:21:04 +0100 Subject: Fix calculation of pack files and objects since bitmap Fix a logic issue where pack files and objects created since the most recent bitmap were incorrectly counted, ignoring their modification time. Since pack files are processed in order from most recent to oldest, we can reliably stop counting as soon as we encounter the first bitmap. By definition, all subsequent pack files are older and should not be included in the count. This ensures accurate repository statistics and prevents overcounting. Bug: jgit-140 Change-Id: I99d85fb70bc7eb42a8d24c74a1fdb8e03334099e --- .../storage/file/GcSinceBitmapStatisticsTest.java | 31 +++++++++++++++++++--- .../org/eclipse/jgit/internal/storage/file/GC.java | 10 ++++--- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcSinceBitmapStatisticsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcSinceBitmapStatisticsTest.java index 3cd766c4e9..af52e2cb85 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcSinceBitmapStatisticsTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcSinceBitmapStatisticsTest.java @@ -20,6 +20,7 @@ import java.util.stream.StreamSupport; import org.eclipse.jgit.internal.storage.file.GC.RepoStatistics; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.storage.pack.PackConfig; import org.junit.Test; public class GcSinceBitmapStatisticsTest extends GcTestCase { @@ -51,11 +52,19 @@ public class GcSinceBitmapStatisticsTest extends GcTestCase { } @Test - public void testShouldReportNoPacksDirectlyAfterGc() throws Exception { - // given + public void testShouldReportNoPacksFilesSinceBitmapWhenPackfilesAreOlderThanBitmapFile() + throws Exception { addCommit(null); - gc.gc().get(); + configureGC(/* buildBitmap */ false).gc().get(); + assertEquals(1L, gc.getStatistics().numberOfPackFiles); + assertEquals(0L, repositoryBitmapFiles()); + assertEquals(1L, gc.getStatistics().numberOfPackFilesSinceBitmap); + + addCommit(null); + configureGC(/* buildBitmap */ true).gc().get(); + assertEquals(1L, repositoryBitmapFiles()); + assertEquals(2L, gc.getStatistics().numberOfPackFiles); assertEquals(0L, gc.getStatistics().numberOfPackFilesSinceBitmap); } @@ -79,8 +88,11 @@ public class GcSinceBitmapStatisticsTest extends GcTestCase { // progress & pack addCommit(parent); - tr.packAndPrune(); + assertEquals(1L, gc.getStatistics().numberOfPackFiles); + assertEquals(0L, gc.getStatistics().numberOfPackFilesSinceBitmap); + tr.packAndPrune(); + assertEquals(2L, gc.getStatistics().numberOfPackFiles); assertEquals(1L, gc.getStatistics().numberOfPackFilesSinceBitmap); } @@ -90,13 +102,17 @@ public class GcSinceBitmapStatisticsTest extends GcTestCase { // commit & gc RevCommit parent = addCommit(null); gc.gc().get(); + assertEquals(0L, gc.getStatistics().numberOfLooseObjects); assertEquals(0L, gc.getStatistics().numberOfObjectsSinceBitmap); // progress & pack addCommit(parent); + assertEquals(1L, gc.getStatistics().numberOfLooseObjects); assertEquals(1L, gc.getStatistics().numberOfObjectsSinceBitmap); tr.packAndPrune(); + assertEquals(0L, gc.getStatistics().numberOfLooseObjects); + // Number of objects contained in the newly created PackFile assertEquals(3L, gc.getStatistics().numberOfObjectsSinceBitmap); } @@ -164,4 +180,11 @@ public class GcSinceBitmapStatisticsTest extends GcTestCase { } }).sum(); } + + private GC configureGC(boolean buildBitmap) { + PackConfig pc = new PackConfig(repo.getObjectDatabase().getConfig()); + pc.setBuildBitmaps(buildBitmap); + gc.setPackConfig(pc); + return gc; + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index b33afed131..3cc43ebb04 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -1579,7 +1579,7 @@ public class GC { public RepoStatistics getStatistics() throws IOException { RepoStatistics ret = new RepoStatistics(); Collection packs = repo.getObjectDatabase().getPacks(); - long latestBitmapTime = Long.MIN_VALUE; + long latestBitmapTime = 0L; for (Pack p : packs) { long packedObjects = p.getIndex().getObjectCount(); ret.numberOfPackedObjects += packedObjects; @@ -1587,9 +1587,11 @@ public class GC { ret.sizeOfPackedObjects += p.getPackFile().length(); if (p.getBitmapIndex() != null) { ret.numberOfBitmaps += p.getBitmapIndex().getBitmapCount(); - latestBitmapTime = p.getFileSnapshot().lastModifiedInstant() - .toEpochMilli(); - } else { + if (latestBitmapTime == 0L) { + latestBitmapTime = p.getFileSnapshot().lastModifiedInstant().toEpochMilli(); + } + } + else if (latestBitmapTime == 0L) { ret.numberOfPackFilesSinceBitmap++; ret.numberOfObjectsSinceBitmap += packedObjects; } -- cgit v1.2.3 From e328d203f20b8cd0a9b55678bfe3678ffd5d8179 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Thu, 13 Feb 2025 15:38:14 -0800 Subject: Do not load bitmap indexes during directory scans Previously, if a bitmap index had not been loaded yet, it would get loaded during a directory scan. Loading a bitmap file can be expensive and there is no immediate need to do so during a scan. Fix this by simply setting bitmap index file names on the Packs during directory scans so that bitmaps can be lazily loaded at some later point if they are needed. This change has the side affect of no longer marking a Pack valid if it is currently invalid simply because a bitmap file has been found, as there is no valid reason to do so and this can incorrectly mark a Pack without an index, or with other issues valid. Since the initial lack of a bitmap file, or an invalid one, or the deletion of one, would not result in the Pack being marked invalid, there is no need to overturn the invalid flag when a new bitmap file is found. Change-Id: I056acc09e7ae6a0982acd81b552d524190ebb4be Signed-off-by: Martin Fick --- .../resources/org/eclipse/jgit/internal/JGitText.properties | 2 -- .../src/org/eclipse/jgit/internal/JGitText.java | 2 -- .../src/org/eclipse/jgit/internal/storage/file/Pack.java | 13 ++----------- .../eclipse/jgit/internal/storage/file/PackDirectory.java | 9 +++------ 4 files changed, 5 insertions(+), 21 deletions(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 19c90086aa..c54c811b57 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -64,8 +64,6 @@ binaryHunkDecodeError=Binary hunk, line {0}: invalid input binaryHunkInvalidLength=Binary hunk, line {0}: input corrupt; expected length byte, got 0x{1} binaryHunkLineTooShort=Binary hunk, line {0}: input ended prematurely binaryHunkMissingNewline=Binary hunk, line {0}: input line not terminated by newline -bitmapAccessErrorForPackfile=Error whilst trying to access bitmap file for {} -bitmapFailedToGet=Failed to get bitmap index file {} bitmapMissingObject=Bitmap at {0} is missing {1}. bitmapsMustBePrepared=Bitmaps must be prepared before they may be written. bitmapUseNoopNoListener=Use NOOP instance for no listener diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 700b54a7a6..b7175508ee 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -94,8 +94,6 @@ public class JGitText extends TranslationBundle { /***/ public String binaryHunkInvalidLength; /***/ public String binaryHunkLineTooShort; /***/ public String binaryHunkMissingNewline; - /***/ public String bitmapAccessErrorForPackfile; - /***/ public String bitmapFailedToGet; /***/ public String bitmapMissingObject; /***/ public String bitmapsMustBePrepared; /***/ public String bitmapUseNoopNoListener; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 122d800582..5813d39e9a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -116,7 +116,7 @@ public class Pack implements Iterable { private volatile Exception invalidatingCause; @Nullable - private PackFile bitmapIdxFile; + private volatile PackFile bitmapIdxFile; private AtomicInteger transientErrorCount = new AtomicInteger(); @@ -1213,17 +1213,8 @@ public class Pack implements Iterable { return null; } - synchronized void refreshBitmapIndex(PackFile bitmapIndexFile) { - this.bitmapIdx = Optionally.empty(); - this.invalid = false; + void setBitmapIndexFile(PackFile bitmapIndexFile) { this.bitmapIdxFile = bitmapIndexFile; - try { - getBitmapIndex(); - } catch (IOException e) { - LOG.warn(JGitText.get().bitmapFailedToGet, bitmapIdxFile, e); - this.bitmapIdx = Optionally.empty(); - this.bitmapIdxFile = null; - } } private synchronized PackReverseIndex getReverseIdx() throws IOException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java index e31126f027..5cc7be4f98 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java @@ -459,12 +459,9 @@ class PackDirectory { && !oldPack.getFileSnapshot().isModified(packFile)) { forReuse.remove(packFile.getName()); list.add(oldPack); - try { - if(oldPack.getBitmapIndex() == null) { - oldPack.refreshBitmapIndex(packFilesByExt.get(BITMAP_INDEX)); - } - } catch (IOException e) { - LOG.warn(JGitText.get().bitmapAccessErrorForPackfile, oldPack.getPackName(), e); + PackFile bitMaps = packFilesByExt.get(BITMAP_INDEX); + if (bitMaps != null) { + oldPack.setBitmapIndexFile(bitMaps); } continue; } -- cgit v1.2.3