IndexPack: Tighten up new and base object bookkeeping

The only current consumer of these collections is ReceivePack,
where it needs to test ObjectId equality between a RevObject and an
ObjectId.  There we were copying from a traditional HashSet<ObjectId>
into an ObjectIdSubclassMap<ObjectId>, as the latter can perform
hashing using ObjectId's native value support, bypassing RevObject's
override on hashCode() and equals().  Instead of doing that copy,
directly create ObjectIdSubclassMap instances inside of ReceivePack.

We also only need to record the objects that do not appear in the
incoming pack, and were therefore copied from the local repositiory
in order to complete delta resolution.  Instead of listing everything
that used an OBJ_REF_DELTA format, list only the objects that we
pulled from the destination repository via a normal ObjectLoader.

ReceivePack can now discard the IndexPack object, and all of its
other data, as soon as these collections are held by the check
connectivity method.  This frees up memory for the ObjectWalk's
own RevObject pool.

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

2 files changed, 39 insertions, 31 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
index 29f200c52d..6eeccea841 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
@@ -54,10 +54,7 @@ import java.io.RandomAccessFile;
 import java.security.MessageDigest;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Deflater;
@@ -173,7 +170,14 @@ public class IndexPack {
 
 	private PackedObjectInfo[] entries;
 
-	private Set<ObjectId> newObjectIds;
+	/**
+	 * Every object contained within the incoming pack.
+	 * <p>
+	 * This is a subset of {@link #entries}, as thin packs can add additional
+	 * objects to {@code entries} by copying already existing objects from the
+	 * repository onto the end of the thin pack to make it self-contained.
+	 */
+	private ObjectIdSubclassMap<ObjectId> newObjectIds;
 
 	private int deltaCount;
 
@@ -183,7 +187,14 @@ public class IndexPack {
 
 	private ObjectIdSubclassMap<DeltaChain> baseById;
 
-	private Set<ObjectId> baseIds;
+	/**
+	 * Objects referenced by their name from deltas, that aren't in this pack.
+	 * <p>
+	 * This is the set of objects that were copied onto the end of this pack to
+	 * make it complete. These objects were not transmitted by the remote peer,
+	 * but instead were assumed to already exist in the local repository.
+	 */
+	private ObjectIdSubclassMap<ObjectId> baseObjectIds;
 
 	private LongMap<UnresolvedDelta> baseByPos;
 
@@ -287,7 +298,7 @@ public class IndexPack {
 	 */
 	public void setNeedNewObjectIds(boolean b) {
 		if (b)
-			newObjectIds = new HashSet<ObjectId>();
+			newObjectIds = new ObjectIdSubclassMap<ObjectId>();
 		else
 			newObjectIds = null;
 	}
@@ -311,17 +322,17 @@ public class IndexPack {
 	}
 
 	/** @return the new objects that were sent by the user */
-	public Set<ObjectId> getNewObjectIds() {
-		return newObjectIds == null ?
-				Collections.<ObjectId>emptySet() : newObjectIds;
+	public ObjectIdSubclassMap<ObjectId> getNewObjectIds() {
+		if (newObjectIds != null)
+			return newObjectIds;
+		return new ObjectIdSubclassMap<ObjectId>();
 	}
 
-	/**
-	 *  @return the set of objects the incoming pack assumed for delta purposes
-	 */
-	public Set<ObjectId> getBaseObjectIds() {
-		return baseIds == null ?
-				Collections.<ObjectId>emptySet() : baseIds;
+	/** @return set of objects the incoming pack assumed for delta purposes */
+	public ObjectIdSubclassMap<ObjectId> getBaseObjectIds() {
+		if (baseObjectIds != null)
+			return baseObjectIds;
+		return new ObjectIdSubclassMap<ObjectId>();
 	}
 
 	/**
@@ -390,12 +401,6 @@ public class IndexPack {
 					if (packOut == null)
 						throw new IOException("need packOut");
 					resolveDeltas(progress);
-					if (needBaseObjectIds) {
-						baseIds = new HashSet<ObjectId>();
-						for (DeltaChain c : baseById) {
-							baseIds.add(c);
-						}
-					}
 					if (entryCount < objectCount) {
 						if (!fixThin) {
 							throw new IOException("pack has "
@@ -566,6 +571,9 @@ public class IndexPack {
 	private void fixThinPack(final ProgressMonitor progress) throws IOException {
 		growEntries();
 
+		if (needBaseObjectIds)
+			baseObjectIds = new ObjectIdSubclassMap<ObjectId>();
+
 		packDigest.reset();
 		originalEOF = packOut.length() - 20;
 		final Deflater def = new Deflater(Deflater.DEFAULT_COMPRESSION, false);
@@ -574,6 +582,8 @@ public class IndexPack {
 		for (final DeltaChain baseId : baseById) {
 			if (baseId.head == null)
 				continue;
+			if (needBaseObjectIds)
+				baseObjectIds.add(baseId);
 			final ObjectLoader ldr = repo.openObject(readCurs, baseId);
 			if (ldr == null) {
 				missing.add(baseId);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
index 8dbeb95985..cce0a17d00 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
@@ -776,12 +776,14 @@ public class ReceivePack {
 	}
 
 	private void checkConnectivity() throws IOException {
-		final Set<ObjectId> baseObjects;
+		ObjectIdSubclassMap<ObjectId> baseObjects = null;
+		ObjectIdSubclassMap<ObjectId> providedObjects = null;
 
-		if (ensureObjectsProvidedVisible)
+		if (ensureObjectsProvidedVisible) {
 			baseObjects = ip.getBaseObjectIds();
-		else
-			baseObjects = Collections.emptySet();
+			providedObjects = ip.getNewObjectIds();
+		}
+		ip = null;
 
 		final ObjectWalk ow = new ObjectWalk(db);
 		for (final ReceiveCommand cmd : commands) {
@@ -805,21 +807,17 @@ public class ReceivePack {
 			}
 		}
 
-		ObjectIdSubclassMap<ObjectId> provided =
-			new ObjectIdSubclassMap<ObjectId>();
 		if (ensureObjectsProvidedVisible) {
 			for (ObjectId id : baseObjects) {
 				   RevObject b = ow.lookupAny(id, Constants.OBJ_BLOB);
 				   if (!b.has(RevFlag.UNINTERESTING))
 				     throw new MissingObjectException(b, b.getType());
 			}
-			for (ObjectId id : ip.getNewObjectIds())
-				provided.add(id);
 		}
 
 		RevCommit c;
 		while ((c = ow.next()) != null) {
-			if (ensureObjectsProvidedVisible && !provided.contains(c))
+			if (ensureObjectsProvidedVisible && !providedObjects.contains(c))
 				throw new MissingObjectException(c, Constants.TYPE_COMMIT);
 		}
 
@@ -828,7 +826,7 @@ public class ReceivePack {
 			if (o instanceof RevBlob && !db.hasObject(o))
 				throw new MissingObjectException(o, Constants.TYPE_BLOB);
 
-			if (ensureObjectsProvidedVisible && !provided.contains(o))
+			if (ensureObjectsProvidedVisible && !providedObjects.contains(o))
 				throw new MissingObjectException(o, Constants.TYPE_BLOB);
 		}
 	}