From c46c720e99baa081bff0dd7bcc1ae8ca48b5e3d1 Mon Sep 17 00:00:00 2001 From: Terry Parker Date: Thu, 18 May 2017 01:30:14 -0700 Subject: [PATCH] Exclude refs/tags from bitmap commit selection Commit db77610 ensured that all refs/tags commits are added to the primary GC pack. It did that by adding all of the refs/tags commits to the primary GC pack PackWriter's "interesting" object set. Unfortunately, all commit objects in the "interesting" set are selected as commits for which bitmap indices will be built. In a repository like chromium with lots of tags, this changed the number of bitmaps created from <700 to >10000. That puts huge memory pressure on the GC task. This change restores the original behavior of ignoring tags when selecting commits for bitmaps. In the "uninteresting" set, commits for refs/heads and refs/tags for unannotated tags can not be differentiated. We instead identify refs/tags commits by passing their ObjectIds as a new "noBitmaps" parameter to the PackWriter.preparePack() methods. PackWriterBitmapPreparer.setupTipCommitBitmaps() can then use that "noBitmaps" parameter to exclude those commits. Change-Id: Icd287c6b04fc1e48de773033fe432a9b0e904ac5 Signed-off-by: Terry Parker --- .../internal/storage/file/PackWriterTest.java | 2 +- .../storage/pack/GcCommitSelectionTest.java | 16 +++- .../storage/dfs/DfsGarbageCollector.java | 39 +++++--- .../jgit/internal/storage/file/GC.java | 45 ++++++--- .../internal/storage/pack/PackWriter.java | 96 ++++++++++++++----- .../pack/PackWriterBitmapPreparer.java | 18 +++- .../eclipse/jgit/transport/UploadPack.java | 2 +- 7 files changed, 159 insertions(+), 59 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java index c817dc3d7..9b97eb4ff 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java @@ -711,7 +711,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { } ObjectWalk ow = walk.toObjectWalkWithSameObjects(); - pw.preparePack(NullProgressMonitor.INSTANCE, ow, want, have); + pw.preparePack(NullProgressMonitor.INSTANCE, ow, want, have, NONE); String id = pw.computeName().getName(); File packdir = new File(repo.getObjectsDirectory(), "pack"); File packFile = new File(packdir, "pack-" + id + ".pack"); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/pack/GcCommitSelectionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/pack/GcCommitSelectionTest.java index 20b8c51ee..d9b58e206 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/pack/GcCommitSelectionTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/pack/GcCommitSelectionTest.java @@ -69,6 +69,15 @@ public class GcCommitSelectionTest extends GcTestCase { @Test public void testBitmapSpansNoMerges() throws Exception { + testBitmapSpansNoMerges(false); + } + + @Test + public void testBitmapSpansNoMergesWithTags() throws Exception { + testBitmapSpansNoMerges(true); + } + + private void testBitmapSpansNoMerges(boolean withTags) throws Exception { /* * Commit counts -> expected bitmap counts for history without merges. * The top 100 contiguous commits should always have bitmaps, and the @@ -89,7 +98,10 @@ public class GcCommitSelectionTest extends GcTestCase { assertTrue(nextCommitCount > currentCommits); // programming error for (int i = currentCommits; i < nextCommitCount; i++) { String str = "A" + i; - bb.commit().message(str).add(str, str).create(); + RevCommit rc = bb.commit().message(str).add(str, str).create(); + if (withTags) { + tr.lightweightTag(str, rc); + } } currentCommits = nextCommitCount; @@ -233,7 +245,7 @@ public class GcCommitSelectionTest extends GcTestCase { m8, m9); PackWriterBitmapPreparer preparer = newPeparer(m9, commits); List selection = new ArrayList<>( - preparer.selectCommits(commits.size())); + preparer.selectCommits(commits.size(), PackWriter.NONE)); // Verify that the output is ordered by the separate "chains" String[] expected = { m0.name(), m1.name(), m2.name(), m4.name(), diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java index de447de4d..e9ec7e718 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java @@ -53,6 +53,7 @@ import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.UN import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX; import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; +import static org.eclipse.jgit.internal.storage.pack.PackWriter.NONE; import java.io.IOException; import java.util.ArrayList; @@ -111,7 +112,8 @@ public class DfsGarbageCollector { private List packsBefore; private List expiredGarbagePacks; - private Set allHeads; + private Set allHeadsAndTags; + private Set allTags; private Set nonHeads; private Set txnHeads; private Set tagTargets; @@ -241,23 +243,36 @@ public class DfsGarbageCollector { Collection refsBefore = getAllRefs(); readPacksBefore(); - allHeads = new HashSet<>(); + Set allHeads = new HashSet<>(); + allHeadsAndTags = new HashSet<>(); + allTags = new HashSet<>(); nonHeads = new HashSet<>(); txnHeads = new HashSet<>(); tagTargets = new HashSet<>(); for (Ref ref : refsBefore) { - if (ref.isSymbolic() || ref.getObjectId() == null) + if (ref.isSymbolic() || ref.getObjectId() == null) { continue; - if (isHead(ref) || isTag(ref)) + } + if (isHead(ref)) { allHeads.add(ref.getObjectId()); - else if (RefTreeNames.isRefTree(refdb, ref.getName())) + } else if (isTag(ref)) { + allTags.add(ref.getObjectId()); + } else if (RefTreeNames.isRefTree(refdb, ref.getName())) { txnHeads.add(ref.getObjectId()); - else + } else { nonHeads.add(ref.getObjectId()); - if (ref.getPeeledObjectId() != null) + } + if (ref.getPeeledObjectId() != null) { tagTargets.add(ref.getPeeledObjectId()); + } } - tagTargets.addAll(allHeads); + // Don't exclude tags that are also branch tips. + allTags.removeAll(allHeads); + allHeadsAndTags.addAll(allHeads); + allHeadsAndTags.addAll(allTags); + + // Hoist all branch tips and tags earlier in the pack file + tagTargets.addAll(allHeadsAndTags); boolean rollback = true; try { @@ -413,12 +428,12 @@ public class DfsGarbageCollector { } private void packHeads(ProgressMonitor pm) throws IOException { - if (allHeads.isEmpty()) + if (allHeadsAndTags.isEmpty()) return; try (PackWriter pw = newPackWriter()) { pw.setTagTargets(tagTargets); - pw.preparePack(pm, allHeads, PackWriter.NONE); + pw.preparePack(pm, allHeadsAndTags, NONE, NONE, allTags); if (0 < pw.getObjectCount()) writePack(GC, pw, pm, estimateGcPackSize(INSERT, RECEIVE, COMPACT, GC)); @@ -432,7 +447,7 @@ public class DfsGarbageCollector { try (PackWriter pw = newPackWriter()) { for (ObjectIdSet packedObjs : newPackObj) pw.excludeObjects(packedObjs); - pw.preparePack(pm, nonHeads, allHeads); + pw.preparePack(pm, nonHeads, allHeadsAndTags); if (0 < pw.getObjectCount()) writePack(GC_REST, pw, pm, estimateGcPackSize(INSERT, RECEIVE, COMPACT, GC_REST)); @@ -446,7 +461,7 @@ public class DfsGarbageCollector { try (PackWriter pw = newPackWriter()) { for (ObjectIdSet packedObjs : newPackObj) pw.excludeObjects(packedObjs); - pw.preparePack(pm, txnHeads, PackWriter.NONE); + pw.preparePack(pm, txnHeads, NONE); if (0 < pw.getObjectCount()) writePack(GC_TXN, pw, pm, 0 /* unknown pack size */); } 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 c68e5f7f3..de8193285 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 @@ -729,7 +729,9 @@ public class GC { long time = System.currentTimeMillis(); Collection refsBefore = getAllRefs(); + Set allHeadsAndTags = new HashSet<>(); Set allHeads = new HashSet<>(); + Set allTags = new HashSet<>(); Set nonHeads = new HashSet<>(); Set txnHeads = new HashSet<>(); Set tagTargets = new HashSet<>(); @@ -739,16 +741,21 @@ public class GC { for (Ref ref : refsBefore) { checkCancelled(); nonHeads.addAll(listRefLogObjects(ref, 0)); - if (ref.isSymbolic() || ref.getObjectId() == null) + if (ref.isSymbolic() || ref.getObjectId() == null) { continue; - if (isHead(ref) || isTag(ref)) + } + if (isHead(ref)) { allHeads.add(ref.getObjectId()); - else if (RefTreeNames.isRefTree(refdb, ref.getName())) + } else if (isTag(ref)) { + allTags.add(ref.getObjectId()); + } else if (RefTreeNames.isRefTree(refdb, ref.getName())) { txnHeads.add(ref.getObjectId()); - else + } else { nonHeads.add(ref.getObjectId()); - if (ref.getPeeledObjectId() != null) + } + if (ref.getPeeledObjectId() != null) { tagTargets.add(ref.getPeeledObjectId()); + } } List excluded = new LinkedList<>(); @@ -758,13 +765,19 @@ public class GC { excluded.add(f.getIndex()); } - tagTargets.addAll(allHeads); + // Don't exclude tags that are also branch tips + allTags.removeAll(allHeads); + allHeadsAndTags.addAll(allHeads); + allHeadsAndTags.addAll(allTags); + + // Hoist all branch tips and tags earlier in the pack file + tagTargets.addAll(allHeadsAndTags); nonHeads.addAll(indexObjects); List ret = new ArrayList<>(2); PackFile heads = null; - if (!allHeads.isEmpty()) { - heads = writePack(allHeads, Collections. emptySet(), + if (!allHeadsAndTags.isEmpty()) { + heads = writePack(allHeadsAndTags, PackWriter.NONE, allTags, tagTargets, excluded); if (heads != null) { ret.add(heads); @@ -772,12 +785,14 @@ public class GC { } } if (!nonHeads.isEmpty()) { - PackFile rest = writePack(nonHeads, allHeads, tagTargets, excluded); + PackFile rest = writePack(nonHeads, allHeadsAndTags, PackWriter.NONE, + tagTargets, excluded); if (rest != null) ret.add(rest); } if (!txnHeads.isEmpty()) { - PackFile txn = writePack(txnHeads, PackWriter.NONE, null, excluded); + PackFile txn = writePack(txnHeads, PackWriter.NONE, PackWriter.NONE, + null, excluded); if (txn != null) ret.add(txn); } @@ -961,8 +976,9 @@ public class GC { } private PackFile writePack(@NonNull Set want, - @NonNull Set have, Set tagTargets, - List excludeObjects) throws IOException { + @NonNull Set have, @NonNull Set tags, + Set tagTargets, List excludeObjects) + throws IOException { checkCancelled(); File tmpPack = null; Map tmpExts = new TreeMap<>( @@ -988,12 +1004,13 @@ public class GC { // prepare the PackWriter pw.setDeltaBaseAsOffset(true); pw.setReuseDeltaCommits(false); - if (tagTargets != null) + if (tagTargets != null) { pw.setTagTargets(tagTargets); + } if (excludeObjects != null) for (ObjectIdSet idx : excludeObjects) pw.excludeObjects(idx); - pw.preparePack(pm, want, have); + pw.preparePack(pm, want, have, PackWriter.NONE, tags); if (pw.getObjectCount() == 0) return null; checkCancelled(); 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 8810a9f60..7271560e3 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 @@ -233,7 +233,9 @@ public class PackWriter implements AutoCloseable { private List cachedPacks = new ArrayList<>(2); - private Set tagTargets = Collections.emptySet(); + private Set tagTargets = NONE; + + private Set excludeFromBitmapSelection = NONE; private ObjectIdSet[] excludeInPacks; @@ -712,8 +714,7 @@ public class PackWriter implements AutoCloseable { public void preparePack(ProgressMonitor countingMonitor, @NonNull Set want, @NonNull Set have) throws IOException { - preparePack(countingMonitor, - want, have, Collections. emptySet()); + preparePack(countingMonitor, want, have, NONE, NONE); } /** @@ -721,9 +722,9 @@ public class PackWriter implements AutoCloseable { *

* Like {@link #preparePack(ProgressMonitor, Set, Set)} but also allows * specifying commits that should not be walked past ("shallow" commits). - * The caller is responsible for filtering out commits that should not - * be shallow any more ("unshallow" commits as in {@link #setShallowPack}) - * from the shallow set. + * The caller is responsible for filtering out commits that should not be + * shallow any more ("unshallow" commits as in {@link #setShallowPack}) from + * the shallow set. * * @param countingMonitor * progress during object enumeration. @@ -731,27 +732,67 @@ public class PackWriter implements AutoCloseable { * objects of interest, ancestors of which will be included in * the pack. Must not be {@code null}. * @param have - * objects whose ancestors (up to and including - * {@code shallow} commits) do not need to be included in the - * pack because they are already available from elsewhere. - * Must not be {@code null}. + * objects whose ancestors (up to and including {@code shallow} + * commits) do not need to be included in the pack because they + * are already available from elsewhere. Must not be + * {@code null}. * @param shallow * commits indicating the boundary of the history marked with - * {@code have}. Shallow commits have parents but those - * parents are considered not to be already available. - * Parents of {@code shallow} commits and earlier generations - * will be included in the pack if requested by {@code want}. - * Must not be {@code null}. + * {@code have}. Shallow commits have parents but those parents + * are considered not to be already available. Parents of + * {@code shallow} commits and earlier generations will be + * included in the pack if requested by {@code want}. Must not be + * {@code null}. * @throws IOException - * an I/O problem occured while reading objects. + * an I/O problem occurred while reading objects. */ public void preparePack(ProgressMonitor countingMonitor, @NonNull Set want, @NonNull Set have, @NonNull Set shallow) throws IOException { + preparePack(countingMonitor, want, have, shallow, NONE); + } + + /** + * Prepare the list of objects to be written to the pack stream. + *

+ * Like {@link #preparePack(ProgressMonitor, Set, Set)} but also allows + * specifying commits that should not be walked past ("shallow" commits). + * The caller is responsible for filtering out commits that should not be + * shallow any more ("unshallow" commits as in {@link #setShallowPack}) from + * the shallow set. + * + * @param countingMonitor + * progress during object enumeration. + * @param want + * objects of interest, ancestors of which will be included in + * the pack. Must not be {@code null}. + * @param have + * objects whose ancestors (up to and including {@code shallow} + * commits) do not need to be included in the pack because they + * are already available from elsewhere. Must not be + * {@code null}. + * @param shallow + * commits indicating the boundary of the history marked with + * {@code have}. Shallow commits have parents but those parents + * are considered not to be already available. Parents of + * {@code shallow} commits and earlier generations will be + * included in the pack if requested by {@code want}. Must not be + * {@code null}. + * @param noBitmaps + * collection of objects to be excluded from bitmap commit + * selection. + * @throws IOException + * an I/O problem occurred while reading objects. + */ + public void preparePack(ProgressMonitor countingMonitor, + @NonNull Set want, + @NonNull Set have, + @NonNull Set shallow, + @NonNull Set noBitmaps) throws IOException { try (ObjectWalk ow = getObjectWalk()) { ow.assumeShallow(shallow); - preparePack(countingMonitor, ow, want, have); + preparePack(countingMonitor, ow, want, have, noBitmaps); } } @@ -784,13 +825,17 @@ public class PackWriter implements AutoCloseable { * points of graph traversal). Pass {@link #NONE} if all objects * reachable from {@code want} are desired, such as when serving * a clone. + * @param noBitmaps + * collection of objects to be excluded from bitmap commit + * selection. * @throws IOException * when some I/O problem occur during reading objects. */ public void preparePack(ProgressMonitor countingMonitor, @NonNull ObjectWalk walk, @NonNull Set interestingObjects, - @NonNull Set uninterestingObjects) + @NonNull Set uninterestingObjects, + @NonNull Set noBitmaps) throws IOException { if (countingMonitor == null) countingMonitor = NullProgressMonitor.INSTANCE; @@ -798,7 +843,7 @@ public class PackWriter implements AutoCloseable { throw new IllegalArgumentException( JGitText.get().shallowPacksRequireDepthWalk); findObjectsToPack(countingMonitor, walk, interestingObjects, - uninterestingObjects); + uninterestingObjects, noBitmaps); } /** @@ -965,8 +1010,9 @@ public class PackWriter implements AutoCloseable { /** * Write the prepared pack to the supplied stream. *

- * Called after {@link #preparePack(ProgressMonitor, ObjectWalk, Set, Set)} - * or {@link #preparePack(ProgressMonitor, Set, Set)}. + * Called after + * {@link #preparePack(ProgressMonitor, ObjectWalk, Set, Set, Set)} or + * {@link #preparePack(ProgressMonitor, Set, Set)}. *

* Performs delta search if enabled and writes the pack stream. *

@@ -1652,12 +1698,14 @@ public class PackWriter implements AutoCloseable { private void findObjectsToPack(@NonNull ProgressMonitor countingMonitor, @NonNull ObjectWalk walker, @NonNull Set want, - @NonNull Set have) throws IOException { + @NonNull Set have, + @NonNull Set noBitmaps) throws IOException { final long countingStart = System.currentTimeMillis(); beginPhase(PackingPhase.COUNTING, countingMonitor, ProgressMonitor.UNKNOWN); stats.interestingObjects = Collections.unmodifiableSet(new HashSet(want)); stats.uninterestingObjects = Collections.unmodifiableSet(new HashSet(have)); + excludeFromBitmapSelection = noBitmaps; canBuildBitmaps = config.isBuildBitmaps() && !shallowPack @@ -2070,8 +2118,8 @@ public class PackWriter implements AutoCloseable { PackWriterBitmapPreparer bitmapPreparer = new PackWriterBitmapPreparer( reader, writeBitmaps, pm, stats.interestingObjects, config); - Collection selectedCommits = - bitmapPreparer.selectCommits(numCommits); + Collection selectedCommits = bitmapPreparer + .selectCommits(numCommits, excludeFromBitmapSelection); beginPhase(PackingPhase.BUILDING_BITMAPS, pm, selectedCommits.size()); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapPreparer.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapPreparer.java index 07a03b404..8bedddb93 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapPreparer.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapPreparer.java @@ -141,6 +141,8 @@ class PackWriterBitmapPreparer { * * @param expectedCommitCount * count of commits in the pack + * @param excludeFromBitmapSelection + * commits that should be excluded from bitmap selection * @return commit objects for which bitmap indices should be built * @throws IncorrectObjectTypeException * if any of the processed objects is not a commit @@ -149,7 +151,8 @@ class PackWriterBitmapPreparer { * @throws MissingObjectException * if an expected object is missing */ - Collection selectCommits(int expectedCommitCount) + Collection selectCommits(int expectedCommitCount, + Set excludeFromBitmapSelection) throws IncorrectObjectTypeException, IOException, MissingObjectException { /* @@ -164,7 +167,7 @@ class PackWriterBitmapPreparer { RevWalk rw = new RevWalk(reader); rw.setRetainBody(false); CommitSelectionHelper selectionHelper = setupTipCommitBitmaps(rw, - expectedCommitCount); + expectedCommitCount, excludeFromBitmapSelection); pm.endTask(); int totCommits = selectionHelper.getCommitCount(); @@ -363,6 +366,8 @@ class PackWriterBitmapPreparer { * @param expectedCommitCount * expected count of commits. The actual count may be less due to * unreachable garbage. + * @param excludeFromBitmapSelection + * commits that should be excluded from bitmap selection * @return a {@link CommitSelectionHelper} containing bitmaps for the tip * commits * @throws IncorrectObjectTypeException @@ -373,8 +378,10 @@ class PackWriterBitmapPreparer { * if an expected object is missing */ private CommitSelectionHelper setupTipCommitBitmaps(RevWalk rw, - int expectedCommitCount) throws IncorrectObjectTypeException, - IOException, MissingObjectException { + int expectedCommitCount, + Set excludeFromBitmapSelection) + throws IncorrectObjectTypeException, IOException, + MissingObjectException { BitmapBuilder reuse = commitBitmapIndex.newBitmapBuilder(); List reuseCommits = new ArrayList<>(); for (PackBitmapIndexRemapper.Entry entry : bitmapRemapper) { @@ -403,7 +410,8 @@ class PackWriterBitmapPreparer { Set peeledWant = new HashSet<>(want.size()); for (AnyObjectId objectId : want) { RevObject ro = rw.peel(rw.parseAny(objectId)); - if (!(ro instanceof RevCommit) || reuse.contains(ro)) { + if (!(ro instanceof RevCommit) || reuse.contains(ro) + || excludeFromBitmapSelection.contains(ro)) { continue; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index ddb2fbf01..17af0b983 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1523,7 +1523,7 @@ public class UploadPack { walk.reset(); ObjectWalk ow = rw.toObjectWalkWithSameObjects(); - pw.preparePack(pm, ow, wantAll, commonBase); + pw.preparePack(pm, ow, wantAll, commonBase, PackWriter.NONE); rw = ow; }