diff options
7 files changed, 202 insertions, 56 deletions
diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug/ShowPackDelta.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug/ShowPackDelta.java index 74e322ff7f..80d3503851 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug/ShowPackDelta.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug/ShowPackDelta.java @@ -71,8 +71,9 @@ class ShowPackDelta extends TextBuiltin { PackWriter pw = new PackWriter(new PackConfig(), reader) { @Override - public void select(ObjectToPack otp, StoredObjectRepresentation next) { + public boolean select(ObjectToPack otp, StoredObjectRepresentation next) { otp.select(next); + return true; } }; diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BasePackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BasePackWriterTest.java index 92d7465376..cd73c6ae83 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BasePackWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BasePackWriterTest.java @@ -19,7 +19,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -702,13 +702,29 @@ public class BasePackWriterTest extends SampleDataRepositoryTestCase { } @Test - public void testTotalPackFilesScanWhenSearchForReuseTimeoutNotSet() + public void testTotalPackFilesScanWhenSearchForReuseTimeoutNotSetTrue() throws Exception { + totalPackFilesScanWhenSearchForReuseTimeoutNotSet(true); + } + + @Test + public void testTotalPackFilesScanWhenSearchForReuseTimeoutNotSetFalse() + throws Exception { + totalPackFilesScanWhenSearchForReuseTimeoutNotSet(false); + } + + public void totalPackFilesScanWhenSearchForReuseTimeoutNotSet(boolean doReturn) throws Exception { FileRepository fileRepository = setUpRepoWithMultiplePackfiles(); + int numberOfPackFiles = (int) new GC(fileRepository).getStatistics().numberOfPackFiles; + int objectsInMultiplePacks = 2; + int objectsInOnePacks = 1; + int expectedSelectCalls = objectsInMultiplePacks * (doReturn ? numberOfPackFiles : 1) + + objectsInOnePacks; + PackWriter mockedPackWriter = Mockito .spy(new PackWriter(config, fileRepository.newObjectReader())); - doNothing().when(mockedPackWriter).select(any(), any()); + doReturn(doReturn).when(mockedPackWriter).select(any(), any()); try (FileOutputStream packOS = new FileOutputStream( getPackFileToWrite(fileRepository, mockedPackWriter))) { @@ -716,27 +732,37 @@ public class BasePackWriterTest extends SampleDataRepositoryTestCase { NullProgressMonitor.INSTANCE, packOS); } - long numberOfPackFiles = new GC(fileRepository) - .getStatistics().numberOfPackFiles; - int expectedSelectCalls = - // Objects contained in multiple packfiles * number of packfiles - 2 * (int) numberOfPackFiles + - // Objects in single packfile - 1; verify(mockedPackWriter, times(expectedSelectCalls)).select(any(), any()); } @Test - public void testTotalPackFilesScanWhenSkippingSearchForReuseTimeoutCheck() + public void testTotalPackFilesScanWhenSkippingSearchForReuseTimeoutCheckTrue() throws Exception { + totalPackFilesScanWhenSkippingSearchForReuseTimeoutCheck(true); + } + + @Test + public void testTotalPackFilesScanWhenSkippingSearchForReuseTimeoutCheckFalse() + throws Exception { + totalPackFilesScanWhenSkippingSearchForReuseTimeoutCheck(false); + } + + public void totalPackFilesScanWhenSkippingSearchForReuseTimeoutCheck( + boolean doReturn) throws Exception { FileRepository fileRepository = setUpRepoWithMultiplePackfiles(); + int numberOfPackFiles = (int) new GC(fileRepository).getStatistics().numberOfPackFiles; + int objectsInMultiplePacks = 2; + int objectsInOnePacks = 1; + int expectedSelectCalls = objectsInMultiplePacks * (doReturn ? numberOfPackFiles : 1) + + objectsInOnePacks; + PackConfig packConfig = new PackConfig(); packConfig.setSearchForReuseTimeout(Duration.ofSeconds(-1)); PackWriter mockedPackWriter = Mockito.spy( new PackWriter(packConfig, fileRepository.newObjectReader())); - doNothing().when(mockedPackWriter).select(any(), any()); + doReturn(doReturn).when(mockedPackWriter).select(any(), any()); try (FileOutputStream packOS = new FileOutputStream( getPackFileToWrite(fileRepository, mockedPackWriter))) { @@ -744,28 +770,31 @@ public class BasePackWriterTest extends SampleDataRepositoryTestCase { NullProgressMonitor.INSTANCE, packOS); } - long numberOfPackFiles = new GC(fileRepository) - .getStatistics().numberOfPackFiles; - int expectedSelectCalls = - // Objects contained in multiple packfiles * number of packfiles - 2 * (int) numberOfPackFiles + - // Objects contained in single packfile - 1; verify(mockedPackWriter, times(expectedSelectCalls)).select(any(), any()); } @Test - public void testPartialPackFilesScanWhenDoingSearchForReuseTimeoutCheck() + public void partialPackFilesScanWhenDoingSearchForReuseTimeoutCheck() throws Exception { + int objectsInMultiplePacks = 2; + int objectsInOnePacks = 1; + int expectedSelectCalls = objectsInMultiplePacks + objectsInOnePacks; + testPartialPackFilesScanWhenDoingSearchForReuseTimeoutCheck(true, expectedSelectCalls); + testPartialPackFilesScanWhenDoingSearchForReuseTimeoutCheck(false, expectedSelectCalls); + } + + public void testPartialPackFilesScanWhenDoingSearchForReuseTimeoutCheck( + boolean doReturn, int expectedSelectCalls) throws Exception { FileRepository fileRepository = setUpRepoWithMultiplePackfiles(); + PackConfig packConfig = new PackConfig(); packConfig.setSearchForReuseTimeout(Duration.ofSeconds(-1)); PackWriter mockedPackWriter = Mockito.spy( new PackWriter(packConfig, fileRepository.newObjectReader())); mockedPackWriter.enableSearchForReuseTimeout(); - doNothing().when(mockedPackWriter).select(any(), any()); + doReturn(doReturn).when(mockedPackWriter).select(any(), any()); try (FileOutputStream packOS = new FileOutputStream( getPackFileToWrite(fileRepository, mockedPackWriter))) { @@ -773,7 +802,6 @@ public class BasePackWriterTest extends SampleDataRepositoryTestCase { NullProgressMonitor.INSTANCE, packOS); } - int expectedSelectCalls = 3; // Objects in packfiles verify(mockedPackWriter, times(expectedSelectCalls)).select(any(), any()); } diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 877a488a73..eeed75f64e 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -1,16 +1,25 @@ <?xml version="1.0" encoding="UTF-8" standalone="no"?> <component id="org.eclipse.jgit" version="2"> - <resource path="src/org/eclipse/jgit/lib/RefDatabase.java" type="org.eclipse.jgit.lib.RefDatabase"> - <filter id="336695337"> + <resource path="META-INF/MANIFEST.MF"> + <filter id="923795461"> <message_arguments> - <message_argument value="org.eclipse.jgit.lib.RefDatabase"/> - <message_argument value="getReflogReader(Ref)"/> + <message_argument value="7.2.2"/> + <message_argument value="7.1.0"/> + </message_arguments> + </filter> + <filter id="934281281"> + <message_arguments> + <message_argument value="org.eclipse.jgit.lib"/> + <message_argument value="8.1.0"/> + <message_argument value="7.1.0"/> </message_arguments> </filter> + </resource> + <resource path="src/org/eclipse/jgit/lib/RefDatabase.java" type="org.eclipse.jgit.lib.RefDatabase"> <filter id="336695337"> <message_arguments> <message_argument value="org.eclipse.jgit.lib.RefDatabase"/> - <message_argument value="getReflogReader(String)"/> + <message_argument value="getReflogReader(Ref)"/> </message_arguments> </filter> </resource> @@ -30,12 +39,6 @@ <filter id="403804204"> <message_arguments> <message_argument value="org.eclipse.jgit.lib.TypedConfigGetter"/> - <message_argument value="getIntInRange(Config, String, String, String, Integer, Integer, Integer)"/> - </message_arguments> - </filter> - <filter id="403804204"> - <message_arguments> - <message_argument value="org.eclipse.jgit.lib.TypedConfigGetter"/> <message_argument value="getIntInRange(Config, String, String, String, int, int, Integer)"/> </message_arguments> </filter> @@ -60,4 +63,12 @@ </message_arguments> </filter> </resource> + <resource path="src/org/eclipse/jgit/util/Iterators.java" type="org.eclipse.jgit.util.Iterators"> + <filter id="1109393411"> + <message_arguments> + <message_argument value="6.10.2"/> + <message_argument value="org.eclipse.jgit.util.Iterators"/> + </message_arguments> + </filter> + </resource> </component> diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index dc25bfde40..af1671d442 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -446,12 +446,12 @@ public class ObjectDirectory extends FileObjectDatabase { AnyObjectId id) throws IOException { if (loose.hasCached(id)) { long len = loose.getSize(curs, id); - if (0 <= len) { + if (len >= 0) { return len; } } long len = getPackedSizeFromSelfOrAlternate(curs, id, null); - if (0 <= len) { + if (len >= 0) { return len; } return getLooseSizeFromSelfOrAlternate(curs, id, null); @@ -461,14 +461,14 @@ public class ObjectDirectory extends FileObjectDatabase { AnyObjectId id, Set<AlternateHandle.Id> skips) throws PackMismatchException { long len = packed.getSize(curs, id); - if (0 <= len) { + if (len >= 0) { return len; } skips = addMe(skips); for (AlternateHandle alt : myAlternates()) { if (!skips.contains(alt.getId())) { len = alt.db.getPackedSizeFromSelfOrAlternate(curs, id, skips); - if (0 <= len) { + if (len >= 0) { return len; } } @@ -479,14 +479,14 @@ public class ObjectDirectory extends FileObjectDatabase { private long getLooseSizeFromSelfOrAlternate(WindowCursor curs, AnyObjectId id, Set<AlternateHandle.Id> skips) throws IOException { long len = loose.getSize(curs, id); - if (0 <= len) { + if (len >= 0) { return len; } skips = addMe(skips); for (AlternateHandle alt : myAlternates()) { if (!skips.contains(alt.getId())) { len = alt.db.getLooseSizeFromSelfOrAlternate(curs, id, skips); - if (0 <= len) { + if (len >= 0) { return len; } } 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 f50c17eafa..544961b936 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 @@ -27,6 +27,7 @@ import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -49,6 +50,7 @@ import org.eclipse.jgit.lib.CoreConfig.TrustStat; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.util.FileUtils; +import org.eclipse.jgit.util.Iterators; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -275,12 +277,14 @@ class PackDirectory { PackList pList = packList.get(); int retries = 0; SEARCH: for (;;) { - for (Pack p : pList.packs) { + for (Pack p : pList.inReverse()) { try { LocalObjectRepresentation rep = p.representation(curs, otp); p.resetTransientErrorCount(); if (rep != null) { - packer.select(otp, rep); + if (!packer.select(otp, rep)) { + return; + } packer.checkSearchForReuseTimeout(); } } catch (SearchForReuseTimeout e) { @@ -584,5 +588,13 @@ class PackDirectory { this.snapshot = monitor; this.packs = packs; } + + Iterable<Pack> inReverse() { + return Iterators.iterable(reverseIterator()); + } + + Iterator<Pack> reverseIterator() { + return Iterators.reverseIterator(packs); + } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java index f025d4e3d5..27fb814f64 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java @@ -2394,17 +2394,46 @@ public class PackWriter implements AutoCloseable { * this method once for each representation available for an object, to * allow the writer to find the most suitable one for the output. * + * This method tries to take a very simple approach to avoiding delta chains + * during object reuse by selecting from the oldest pack that contains them. + * This helps select many objects from the same pack which helps make good + * use of any WindowCache caching, and it helps prevent cycles because a + * pack must not have a cycle in the delta chain. If both objects A and B + * are chosen out of the same source pack then there cannot be an A->B->A + * cycle. + * + * The oldest pack is also the most likely to have the smallest deltas. It + * generally is the biggest pack in the system and probably came from the + * clone (or last GC) of this repository, where all objects were previously + * considered and packed tightly together. If an object appears again (for + * example due to a revert and a push into this repository) the newer copy + * won't be nearly as small as the older delta version of it, even if the + * newer one is also itself a delta. + * + * Thus this method is optimized for being called in an order that presumes + * that earlier representations are better than later ones, and it expects + * representations from older pack files to be tested first, and it will + * shortcut any searching once it is satisfied with the selected + * representation. Perhaps ideally representation testing ordering should be + * based on packfile object count instead of age since file age can be + * altered, and be deceiving for other reasons. Perhaps the presence of a + * bitmap file for a pack file should prioritize it to be tested even + * earlier than object count? + * * @param otp * the object being packed. * @param next * the next available representation from the repository. + * @return whether the search should continue in the hopes of finding a + * better representation */ - public void select(ObjectToPack otp, StoredObjectRepresentation next) { + public boolean select(ObjectToPack otp, StoredObjectRepresentation next) { int nFmt = next.getFormat(); if (!cachedPacks.isEmpty()) { - if (otp.isEdge()) - return; + if (otp.isEdge()) { + return false; + } if (nFmt == PACK_WHOLE || nFmt == PACK_DELTA) { for (CachedPack pack : cachedPacks) { if (pack.hasObject(otp, next)) { @@ -2412,7 +2441,7 @@ public class PackWriter implements AutoCloseable { otp.clearDeltaBase(); otp.clearReuseAsIs(); pruneCurrentObjectList = true; - return; + return false; } } } @@ -2422,23 +2451,22 @@ public class PackWriter implements AutoCloseable { ObjectId baseId = next.getDeltaBase(); ObjectToPack ptr = objectsMap.get(baseId); if (ptr != null && !ptr.isEdge()) { - otp.setDeltaBase(ptr); - otp.setReuseAsIs(); - } else if (thin && have(ptr, baseId)) { - otp.setDeltaBase(baseId); - otp.setReuseAsIs(); - } else { - otp.clearDeltaBase(); - otp.clearReuseAsIs(); + return useDelta(otp, next, ptr); + } + if (thin && have(ptr, baseId)) { + return useDelta(otp, next, baseId); } + otp.clearDeltaBase(); + otp.clearReuseAsIs(); } else if (nFmt == PACK_WHOLE && config.isReuseObjects()) { int nWeight = next.getWeight(); if (otp.isReuseAsIs() && !otp.isDeltaRepresentation()) { // We've chosen another PACK_WHOLE format for this object, // choose the one that has the smaller compressed size. // - if (otp.getWeight() <= nWeight) - return; + if (otp.getWeight() < nWeight) { + return true; + } } otp.clearDeltaBase(); otp.setReuseAsIs(); @@ -2450,6 +2478,15 @@ public class PackWriter implements AutoCloseable { otp.setDeltaAttempted(reuseDeltas && next.wasDeltaAttempted()); otp.select(next); + return true; + } + + private boolean useDelta(ObjectToPack otp, StoredObjectRepresentation rep, ObjectId base) { + otp.setDeltaBase(base); + otp.setReuseAsIs(); + otp.setDeltaAttempted(reuseDeltas && rep.wasDeltaAttempted()); + otp.select(rep); + return false; } private final boolean have(ObjectToPack ptr, AnyObjectId objectId) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java new file mode 100644 index 0000000000..74b728bdf7 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java @@ -0,0 +1,57 @@ +/* + * Copyright (C) 2025, NVIDIA Corporation. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.util; + +import java.util.Iterator; + +/** + * Utility class for Iterators + * + * @since 6.10.2 + */ +public class Iterators { + /** + * Create an iterator which traverses an array in reverse. + * + * @param array T[] + * @return Iterator<T> + */ + public static <T> Iterator<T> reverseIterator(T[] array) { + return new Iterator<>() { + int index = array.length; + + @Override + public boolean hasNext() { + return index > 0; + } + + @Override + public T next() { + return array[--index]; + } + }; + } + + /** + * Make an iterable for easy use in modern for loops. + * + * @param iterator Iterator<T> + * @return Iterable<T> + */ + public static <T> Iterable<T> iterable(Iterator<T> iterator) { + return new Iterable<>() { + @Override + public Iterator<T> iterator() { + return iterator; + } + }; + } +} |