From f102abf035ae242d96368dadafaee8a339f72c7e Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 3 Nov 2015 21:54:48 -0800 Subject: [PATCH 1/2] Replace anonymous BitmapRevFilter subclasses with explicit classes This puts the code for include() in each RevFilter returned by newRevFilter in one place and should make the code easier to understand and modify. Change-Id: I590ac4604b61fc1ffe7aba2ed89f8139847ccac3 --- .../storage/pack/PackWriterBitmapWalker.java | 88 ++++++++++++++----- 1 file changed, 68 insertions(+), 20 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java index cd18a9c5c..4a7585a9d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java @@ -110,7 +110,7 @@ final class PackWriterBitmapWalker { } if (marked) { - BitmapRevFilter filter = newRevFilter(seen, bitmapResult); + RevFilter filter = newRevFilter(seen, bitmapResult); walker.setRevFilter(filter); while (walker.next() != null) { @@ -141,41 +141,89 @@ final class PackWriterBitmapWalker { walker.reset(); } - static BitmapRevFilter newRevFilter( - final BitmapBuilder seen, final BitmapBuilder bitmapResult) { + static RevFilter newRevFilter(BitmapBuilder seen, BitmapBuilder bitmapResult) { if (seen != null) { - return new BitmapRevFilter() { - protected boolean load(RevCommit cmit) { - if (seen.contains(cmit)) - return false; - return bitmapResult.add(cmit, Constants.OBJ_COMMIT); - } - }; + return new AddUnseenToBitmapFilter(seen, bitmapResult); + } + return new AddToBitmapFilter(bitmapResult); + } + + /** + * A RevFilter that adds the visited commits to {@code bitmap} as a side + * effect. + *

+ * When the walk hits a commit that is part of {@code bitmap}'s + * BitmapIndex, that entire bitmap is ORed into {@code bitmap} and the + * commit and its parents are marked as SEEN so that the walk does not + * have to visit its ancestors. This ensures the walk is very short if + * there is good bitmap coverage. + */ + private static class AddToBitmapFilter extends RevFilter { + private final BitmapBuilder bitmap; + + AddToBitmapFilter(BitmapBuilder bitmap) { + this.bitmap = bitmap; } - return new BitmapRevFilter() { - @Override - protected boolean load(RevCommit cmit) { - return bitmapResult.add(cmit, Constants.OBJ_COMMIT); + + @Override + public final boolean include(RevWalk walker, RevCommit cmit) { + if (bitmap.add(cmit, Constants.OBJ_COMMIT)) { + return true; + } + for (RevCommit p : cmit.getParents()) { + p.add(RevFlag.SEEN); } - }; + return false; + } + + @Override + public final RevFilter clone() { + throw new UnsupportedOperationException(); + } + + @Override + public final boolean requiresCommitBody() { + return false; + } } - static abstract class BitmapRevFilter extends RevFilter { - protected abstract boolean load(RevCommit cmit); + /** + * A RevFilter that adds the visited commits to {@code bitmap} as a side + * effect. + *

+ * When the walk hits a commit that is part of {@code bitmap}'s + * BitmapIndex, that entire bitmap is ORed into {@code bitmap} and the + * commit and its parents are marked as SEEN so that the walk does not + * have to visit its ancestors. This ensures the walk is very short if + * there is good bitmap coverage. + *

+ * Commits named in {@code seen} are considered already seen. If one is + * encountered, that commit and its parents will be marked with the SEEN + * flag to prevent the walk from visiting its ancestors. + */ + private static class AddUnseenToBitmapFilter extends RevFilter { + private final BitmapBuilder seen; + private final BitmapBuilder bitmap; + + AddUnseenToBitmapFilter(BitmapBuilder seen, BitmapBuilder bitmapResult) { + this.seen = seen; + this.bitmap = bitmapResult; + } @Override public final boolean include(RevWalk walker, RevCommit cmit) { - if (load(cmit)) { + if (!seen.contains(cmit) && bitmap.add(cmit, Constants.OBJ_COMMIT)) { return true; } - for (RevCommit p : cmit.getParents()) + for (RevCommit p : cmit.getParents()) { p.add(RevFlag.SEEN); + } return false; } @Override public final RevFilter clone() { - return this; + throw new UnsupportedOperationException(); } @Override From fd1ee636a29638e44a3f8c09df846f9ac540c6cf Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 3 Nov 2015 22:33:05 -0800 Subject: [PATCH 2/2] Use .addObject and .or instead of .add in AddToBitmapFilter This is the caller that the BitmapBuilder.add method was designed around. Moving away from .add makes it more verbose but hopefully clearer. Change-Id: I57b1d7c1dc8fb800b242b76c606922b5aa36b9b2 --- .../storage/pack/PackWriterBitmapWalker.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java index 4a7585a9d..01aa93255 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapWalker.java @@ -167,9 +167,17 @@ final class PackWriterBitmapWalker { @Override public final boolean include(RevWalk walker, RevCommit cmit) { - if (bitmap.add(cmit, Constants.OBJ_COMMIT)) { + Bitmap visitedBitmap; + + if (bitmap.contains(cmit)) { + // already included + } else if ((visitedBitmap = bitmap.getBitmapIndex().getBitmap(cmit)) != null) { + bitmap.or(visitedBitmap); + } else { + bitmap.addObject(cmit, Constants.OBJ_COMMIT); return true; } + for (RevCommit p : cmit.getParents()) { p.add(RevFlag.SEEN); } @@ -212,9 +220,17 @@ final class PackWriterBitmapWalker { @Override public final boolean include(RevWalk walker, RevCommit cmit) { - if (!seen.contains(cmit) && bitmap.add(cmit, Constants.OBJ_COMMIT)) { + Bitmap visitedBitmap; + + if (seen.contains(cmit) || bitmap.contains(cmit)) { + // already seen or included + } else if ((visitedBitmap = bitmap.getBitmapIndex().getBitmap(cmit)) != null) { + bitmap.or(visitedBitmap); + } else { + bitmap.addObject(cmit, Constants.OBJ_COMMIT); return true; } + for (RevCommit p : cmit.getParents()) { p.add(RevFlag.SEEN); }