PackWriter: Speed up pruning of objects from cached packs

During object enumeration for the thin pack, very few objects come
out that are duplicated with the cached pack. Typically these are
only cases where a blob or tree was cherry-picked forward, got a
copy or rename, or was reverted... all relatively infrequent events.

Speed up pruning of the thin pack object list by combining the phase
with the object representation selection. Implementers should already
be offering to reuse the object from the cached pack if it is stored
there, at which point the implementation can perform a very fast type
of containment test using the cached pack's identity rather than yet
another index lookup.  For the local disk case this is probably not a
big improvement, but it does help on the DHT implementation where the
two passes combined into one reduces latency.

Change-Id: I6a07fc75d9075bf6233e967360b6546f9e9a2b33
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LocalCachedPack.java

@@ -47,13 +47,14 @@ import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.storage.pack.CachedPack;
+import org.eclipse.jgit.storage.pack.ObjectToPack;
 import org.eclipse.jgit.storage.pack.PackOutputStream;
+import org.eclipse.jgit.storage.pack.StoredObjectRepresentation;
 
 class LocalCachedPack extends CachedPack {
 	private final ObjectDirectory odb;
@@ -62,6 +63,8 @@ class LocalCachedPack extends CachedPack {
 
 	private final String[] packNames;
 
+	private PackFile[] packs;
+
 	LocalCachedPack(ObjectDirectory odb, Set<ObjectId> tips,
 			List<String> packNames) {
 		this.odb = odb;
@@ -82,34 +85,39 @@ class LocalCachedPack extends CachedPack {
 	@Override
 	public long getObjectCount() throws IOException {
 		long cnt = 0;
-		for (String packName : packNames)
+		for (PackFile pack : getPacks())
-			cnt += getPackFile(packName).getObjectCount();
+			cnt += pack.getObjectCount();
 		return cnt;
 	}
 
 	void copyAsIs(PackOutputStream out, boolean validate, WindowCursor wc)
 			throws IOException {
-		for (String packName : packNames)
+		for (PackFile pack : getPacks())
-			getPackFile(packName).copyPackAsIs(out, validate, wc);
+			pack.copyPackAsIs(out, validate, wc);
 	}
 
 	@Override
-	public <T extends ObjectId> Set<ObjectId> hasObject(Iterable<T> toFind)
+	public boolean hasObject(ObjectToPack obj, StoredObjectRepresentation rep) {
-			throws IOException {
+		try {
-		PackFile[] packs = new PackFile[packNames.length];
+			LocalObjectRepresentation local = (LocalObjectRepresentation) rep;
-		for (int i = 0; i < packNames.length; i++)
+			for (PackFile pack : getPacks()) {
-			packs[i] = getPackFile(packNames[i]);
+				if (local.pack == pack)
-
+					return true;
-		Set<ObjectId> have = new HashSet<ObjectId>();
-		for (ObjectId id : toFind) {
-			for (PackFile pack : packs) {
-				if (pack.hasObject(id)) {
-					have.add(id);
-					break;
-				}
 			}
+			return false;
+		} catch (FileNotFoundException packGone) {
+			return false;
+		}
+	}
+
+	private PackFile[] getPacks() throws FileNotFoundException {
+		if (packs == null) {
+			PackFile[] p = new PackFile[packNames.length];
+			for (int i = 0; i < packNames.length; i++)
+				p[i] = getPackFile(packNames[i]);
+			packs = p;
 		}
-		return have;
+		return packs;
 	}
 
 	private PackFile getPackFile(String packName) throws FileNotFoundException {

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/CachedPack.java

@@ -93,16 +93,27 @@ public abstract class CachedPack {
 	}
 
 	/**
-	 * Determine if the pack contains the requested objects.
+	 * Determine if this pack contains the object representation given.
+	 * <p>
+	 * PackWriter uses this method during the finding sources phase to prune
+	 * away any objects from the leading thin-pack that already appear within
+	 * this pack and should not be sent twice.
+	 * <p>
+	 * Implementors are strongly encouraged to rely on looking at {@code rep}
+	 * only and using its internal state to decide if this object is within this
+	 * pack. Implementors should ensure a representation from this cached pack
+	 * is tested as part of
+	 * {@link ObjectReuseAsIs#selectObjectRepresentation(PackWriter, org.eclipse.jgit.lib.ProgressMonitor, Iterable)}
+	 * , ensuring this method would eventually return true if the object would
+	 * be included by this cached pack.
 	 *
-	 * @param <T>
+	 * @param obj
-	 *            any type of ObjectId to search for.
+	 *            the object being packed. Can be used as an ObjectId.
-	 * @param toFind
+	 * @param rep
-	 *            the objects to search for.
+	 *            representation from the {@link ObjectReuseAsIs} instance that
-	 * @return the objects contained in the pack.
+	 *            originally supplied this CachedPack.
-	 * @throws IOException
+	 * @return true if this pack contains this object.
-	 *             the pack cannot be accessed
 	 */
-	public abstract <T extends ObjectId> Set<ObjectId> hasObject(
+	public abstract boolean hasObject(ObjectToPack obj,
-			Iterable<T> toFind) throws IOException;
+			StoredObjectRepresentation rep);
 }

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/ObjectReuseAsIs.java

@@ -81,13 +81,19 @@ public interface ObjectReuseAsIs {
 
 	/**
 	 * Select the best object representation for a packer.
-	 *
+	 * <p>
 	 * Implementations should iterate through all available representations of
 	 * an object, and pass them in turn to the PackWriter though
 	 * {@link PackWriter#select(ObjectToPack, StoredObjectRepresentation)} so
 	 * the writer can select the most suitable representation to reuse into the
 	 * output stream.
-	 *
+	 * <p>
+	 * If the implementation returns CachedPack from {@link #getCachedPacks()},
+	 * it must consider the representation of any object that is stored in any
+	 * of the offered CachedPacks. PackWriter relies on this behavior to prune
+	 * duplicate objects out of the pack stream when it selects a CachedPack and
+	 * the object was also reached through the thin-pack enumeration.
+	 * <p>
 	 * The implementation may choose to consider multiple objects at once on
 	 * concurrent threads, but must evaluate all representations of an object
 	 * within the same thread.

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java

@@ -188,6 +188,8 @@ public class PackWriter {
 
 	private boolean ignoreMissingUninteresting = true;
 
+	private boolean pruneCurrentObjectList;
+
 	/**
 	 * Create writer for specified repository.
 	 * <p>
@@ -526,16 +528,7 @@ public class PackWriter {
 	 */
 	public boolean willInclude(final AnyObjectId id) throws IOException {
 		ObjectToPack obj = objectsMap.get(id);
-		if (obj != null && !obj.isEdge())
+		return obj != null && !obj.isEdge();
-			return true;
-
-		Set<ObjectId> toFind = Collections.singleton(id.toObjectId());
-		for (CachedPack pack : cachedPacks) {
-			if (pack.hasObject(toFind).contains(id))
-				return true;
-		}
-
-		return false;
 	}
 
 	/**
@@ -639,7 +632,10 @@ public class PackWriter {
 		if (writeMonitor == null)
 			writeMonitor = NullProgressMonitor.INSTANCE;
 
-		if ((reuseDeltas || config.isReuseObjects()) && reuseSupport != null)
+		if (reuseSupport != null && (
+				   reuseDeltas
+				|| config.isReuseObjects()
+				|| !cachedPacks.isEmpty()))
 			searchForReuse(compressMonitor);
 		if (config.isDeltaCompress())
 			searchForDeltas(compressMonitor);
@@ -715,8 +711,12 @@ public class PackWriter {
 			cnt += list.size();
 		long start = System.currentTimeMillis();
 		monitor.beginTask(JGitText.get().searchForReuse, cnt);
-		for (List<ObjectToPack> list : objectsLists)
+		for (List<ObjectToPack> list : objectsLists) {
+			pruneCurrentObjectList = false;
 			reuseSupport.selectObjectRepresentation(this, monitor, list);
+			if (pruneCurrentObjectList)
+				pruneEdgesFromObjectList(list);
+		}
 		monitor.endTask();
 		stats.timeSearchingForReuse = System.currentTimeMillis() - start;
 	}
@@ -1324,7 +1324,6 @@ public class PackWriter {
 		for (RevObject obj : haveObjs)
 			walker.markUninteresting(obj);
 
-		int typesToPrune = 0;
 		final int maxBases = config.getDeltaSearchWindowSize();
 		Set<RevTree> baseTrees = new HashSet<RevTree>();
 		BlockList<RevCommit> commits = new BlockList<RevCommit>();
@@ -1388,15 +1387,6 @@ public class PackWriter {
 		}
 		commits = null;
 
-		for (CachedPack p : cachedPacks) {
-			for (ObjectId d : p.hasObject(objectsLists[Constants.OBJ_COMMIT])) {
-				if (baseTrees.size() <= maxBases)
-					baseTrees.add(walker.lookupCommit(d).getTree());
-				objectsMap.get(d).setEdge();
-				typesToPrune |= 1 << Constants.OBJ_COMMIT;
-			}
-		}
-
 		BaseSearch bases = new BaseSearch(countingMonitor, baseTrees, //
 				objectsMap, edgeObjects, reader);
 		RevObject o;
@@ -1413,39 +1403,13 @@ public class PackWriter {
 			countingMonitor.update(1);
 		}
 
-		for (CachedPack p : cachedPacks) {
-			for (ObjectId d : p.hasObject(objectsLists[Constants.OBJ_TREE])) {
-				objectsMap.get(d).setEdge();
-				typesToPrune |= 1 << Constants.OBJ_TREE;
-			}
-			for (ObjectId d : p.hasObject(objectsLists[Constants.OBJ_BLOB])) {
-				objectsMap.get(d).setEdge();
-				typesToPrune |= 1 << Constants.OBJ_BLOB;
-			}
-			for (ObjectId d : p.hasObject(objectsLists[Constants.OBJ_TAG])) {
-				objectsMap.get(d).setEdge();
-				typesToPrune |= 1 << Constants.OBJ_TAG;
-			}
-		}
-
-		if (typesToPrune != 0) {
-			pruneObjectList(typesToPrune, Constants.OBJ_COMMIT);
-			pruneObjectList(typesToPrune, Constants.OBJ_TREE);
-			pruneObjectList(typesToPrune, Constants.OBJ_BLOB);
-			pruneObjectList(typesToPrune, Constants.OBJ_TAG);
-		}
-
 		for (CachedPack pack : cachedPacks)
 			countingMonitor.update((int) pack.getObjectCount());
 		countingMonitor.endTask();
 		stats.timeCounting = System.currentTimeMillis() - countingStart;
 	}
 
-	private void pruneObjectList(int typesToPrune, int typeCode) {
+	private static void pruneEdgesFromObjectList(List<ObjectToPack> list) {
-		if ((typesToPrune & (1 << typeCode)) == 0)
-			return;
-
-		final List<ObjectToPack> list = objectsLists[typeCode];
 		final int size = list.size();
 		int src = 0;
 		int dst = 0;
@@ -1544,6 +1508,23 @@ public class PackWriter {
 	 */
 	public void select(ObjectToPack otp, StoredObjectRepresentation next) {
 		int nFmt = next.getFormat();
+
+		if (!cachedPacks.isEmpty()) {
+			if (otp.isEdge())
+				return;
+			if ((nFmt == PACK_WHOLE) | (nFmt == PACK_DELTA)) {
+				for (CachedPack pack : cachedPacks) {
+					if (pack.hasObject(otp, next)) {
+						otp.setEdge();
+						otp.clearDeltaBase();
+						otp.clearReuseAsIs();
+						pruneCurrentObjectList = true;
+						return;
+					}
+				}
+			}
+		}
+
 		int nWeight;
 		if (otp.isReuseAsIs()) {
 			// We've already chosen to reuse a packed form, if next