[JGIT PATCH 0/5] Fix major performance problems in UploadPack

[JGIT PATCH 0/5] Fix major performance problems in UploadPack

[Shawn O. Pearce]

If the client is really far ahead of the server, JGit's UploadPack
can spend a massive amount of CPU time on the server just doing the
same work over and over again.  This series addresses the issue by
taking advantage of cached data more frequently.

Shawn O. Pearce (5):
  Make RevTag getObject(), getName() final to prevent overrides
  Allow exceptions to be created with integer type codes
  Unify RevWalk parsing code to be more consistent across types
  Change RevObject dispose() semantics to avoid reparses
  UploadPack: Only recompute okToGiveUp() if bases changed

 .../org/spearce/jgit/revwalk/RevWalkTestCase.java  |    2 +-
 .../jgit/errors/IncorrectObjectTypeException.java  |   13 +++
 .../jgit/errors/MissingObjectException.java        |   12 +++
 .../src/org/spearce/jgit/lib/PackWriter.java       |    3 +-
 .../spearce/jgit/revwalk/BoundaryGenerator.java    |    2 +-
 .../spearce/jgit/revwalk/MergeBaseGenerator.java   |    2 +-
 .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |    4 +-
 .../org/spearce/jgit/revwalk/PendingGenerator.java |    6 +-
 .../src/org/spearce/jgit/revwalk/RevBlob.java      |    5 -
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |   29 ++++----
 .../src/org/spearce/jgit/revwalk/RevObject.java    |   34 ++++++--
 .../src/org/spearce/jgit/revwalk/RevTag.java       |   32 ++++----
 .../src/org/spearce/jgit/revwalk/RevTree.java      |    5 -
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |   82 ++++++++++++++-----
 .../spearce/jgit/revwalk/RewriteTreeFilter.java    |    2 +-
 .../src/org/spearce/jgit/transport/UploadPack.java |   31 +++++---
 .../jgit/transport/WalkFetchConnection.java        |   15 +---
 17 files changed, 179 insertions(+), 100 deletions(-)

[JGIT PATCH 1/5] Make RevTag getObject(), getName() final to prevent overrides

[Shawn O. Pearce]

These methods exist solely to export the object headers from the
tag buffer.  Overriding them may create confusion by trying to
replace the value with something other than what was parsed from
the tag headers.  Other methods like getShortMessage() and the
getTaggerIdent() are likewise already marked final.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/revwalk/RevTag.java       |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
index cace82d..83fd873 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
@@ -189,7 +189,7 @@ public Tag asTag(final RevWalk walk) {
 	 * 
 	 * @return object this tag refers to.
 	 */
-	public RevObject getObject() {
+	public final RevObject getObject() {
 		return object;
 	}
 
@@ -198,7 +198,7 @@ public RevObject getObject() {
 	 * 
 	 * @return name of the tag, according to the tag header.
 	 */
-	public String getName() {
+	public final String getName() {
 		return name;
 	}
 
-- 
1.6.3.2.367.gf0de

[JGIT PATCH 2/5] Allow exceptions to be created with integer type codes

[Shawn O. Pearce]

This may be easier in some contexts where we have the type code
we expect handy, but not the type string constant.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../jgit/errors/IncorrectObjectTypeException.java  |   13 +++++++++++++
 .../jgit/errors/MissingObjectException.java        |   12 ++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/IncorrectObjectTypeException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/IncorrectObjectTypeException.java
index a194f19..3af1f44 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/errors/IncorrectObjectTypeException.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/IncorrectObjectTypeException.java
@@ -40,6 +40,7 @@
 
 import java.io.IOException;
 
+import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
 
 /**
@@ -62,4 +63,16 @@
 	public IncorrectObjectTypeException(final ObjectId id, final String type) {
 		super("Object " + id.name() + " is not a " + type + ".");
 	}
+
+	/**
+	 * Construct and IncorrectObjectTypeException for the specified object id.
+	 *
+	 * Provide the type to make it easier to track down the problem.
+	 *
+	 * @param id SHA-1
+	 * @param type object type
+	 */
+	public IncorrectObjectTypeException(final ObjectId id, final int type) {
+		this(id, Constants.typeString(type));
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/MissingObjectException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/MissingObjectException.java
index 37147c3..7b7b8f3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/errors/MissingObjectException.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/MissingObjectException.java
@@ -40,6 +40,7 @@
 
 import java.io.IOException;
 
+import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
 
 /**
@@ -58,4 +59,15 @@
 	public MissingObjectException(final ObjectId id, final String type) {
 		super("Missing " + type + " " + id.name());
 	}
+
+	/**
+	 * Construct a MissingObjectException for the specified object id.
+	 * Expected type is reported to simplify tracking down the problem.
+	 *
+	 * @param id SHA-1
+	 * @param type object type
+	 */
+	public MissingObjectException(final ObjectId id, final int type) {
+		this(id, Constants.typeString(type));
+	}
 }
-- 
1.6.3.2.367.gf0de

[JGIT PATCH 3/5] Unify RevWalk parsing code to be more consistent across types

[Shawn O. Pearce]

The parse() method now verifies the object actually exists, and
not just that we have a handle to it (e.g. in the case of a tree
or a blob type).  This allowed us to push a chunk of the loading
code up into the RevObject base class, removing some duplication.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/revwalk/RevBlob.java      |    5 ----
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |   11 +--------
 .../src/org/spearce/jgit/revwalk/RevObject.java    |   23 +++++++++++++++++--
 .../src/org/spearce/jgit/revwalk/RevTag.java       |    9 +-------
 .../src/org/spearce/jgit/revwalk/RevTree.java      |    5 ----
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |   14 ++---------
 6 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
index cf241cf..70eeac0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
@@ -53,11 +53,6 @@ protected RevBlob(final AnyObjectId id) {
 	}
 
 	@Override
-	void parse(final RevWalk walk) {
-		flags |= PARSED;
-	}
-	
-	@Override
 	public final int getType() {
 		return Constants.OBJ_BLOB;
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
index 2a59ec4..679718e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
@@ -46,7 +46,6 @@
 import org.spearce.jgit.lib.Commit;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.MutableObjectId;
-import org.spearce.jgit.lib.ObjectLoader;
 import org.spearce.jgit.lib.PersonIdent;
 import org.spearce.jgit.util.RawParseUtils;
 
@@ -54,8 +53,6 @@
 public class RevCommit extends RevObject {
 	static final RevCommit[] NO_PARENTS = {};
 
-	private static final String TYPE_COMMIT = Constants.TYPE_COMMIT;
-
 	private RevTree tree;
 
 	RevCommit[] parents;
@@ -79,13 +76,7 @@ protected RevCommit(final AnyObjectId id) {
 	@Override
 	void parse(final RevWalk walk) throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
-		final ObjectLoader ldr = walk.db.openObject(walk.curs, this);
-		if (ldr == null)
-			throw new MissingObjectException(this, TYPE_COMMIT);
-		final byte[] data = ldr.getCachedBytes();
-		if (Constants.OBJ_COMMIT != ldr.getType())
-			throw new IncorrectObjectTypeException(this, TYPE_COMMIT);
-		parseCanonical(walk, data);
+		parseCanonical(walk, loadCanonical(walk));
 	}
 
 	void parseCanonical(final RevWalk walk, final byte[] raw) {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index 1a13d0a..0e0386c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -39,11 +39,13 @@
 
 import java.io.IOException;
 
+import org.spearce.jgit.errors.CorruptObjectException;
 import org.spearce.jgit.errors.IncorrectObjectTypeException;
 import org.spearce.jgit.errors.MissingObjectException;
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ObjectLoader;
 
 /** Base object type accessed during revision walking. */
 public abstract class RevObject extends ObjectId {
@@ -55,9 +57,24 @@ RevObject(final AnyObjectId name) {
 		super(name);
 	}
 
-	abstract void parse(RevWalk walk) throws MissingObjectException,
-			IncorrectObjectTypeException, IOException;
-	
+	void parse(final RevWalk walk) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		loadCanonical(walk);
+		flags |= PARSED;
+	}
+
+	final byte[] loadCanonical(final RevWalk walk) throws IOException,
+			MissingObjectException, IncorrectObjectTypeException,
+			CorruptObjectException {
+		final ObjectLoader ldr = walk.db.openObject(walk.curs, this);
+		if (ldr == null)
+			throw new MissingObjectException(this, getType());
+		final byte[] data = ldr.getCachedBytes();
+		if (getType() != ldr.getType())
+			throw new IncorrectObjectTypeException(this, getType());
+		return data;
+	}
+
 	/**
 	 * Get Git object type. See {@link Constants}.
 	 * 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
index 83fd873..e876025 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
@@ -45,7 +45,6 @@
 import org.spearce.jgit.errors.MissingObjectException;
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.lib.Constants;
-import org.spearce.jgit.lib.ObjectLoader;
 import org.spearce.jgit.lib.PersonIdent;
 import org.spearce.jgit.lib.Tag;
 import org.spearce.jgit.util.MutableInteger;
@@ -72,13 +71,7 @@ protected RevTag(final AnyObjectId id) {
 	@Override
 	void parse(final RevWalk walk) throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
-		final ObjectLoader ldr = walk.db.openObject(walk.curs, this);
-		if (ldr == null)
-			throw new MissingObjectException(this, Constants.TYPE_TAG);
-		final byte[] data = ldr.getCachedBytes();
-		if (Constants.OBJ_TAG != ldr.getType())
-			throw new IncorrectObjectTypeException(this, Constants.TYPE_TAG);
-		parseCanonical(walk, data);
+		parseCanonical(walk, loadCanonical(walk));
 	}
 
 	void parseCanonical(final RevWalk walk, final byte[] rawTag)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
index 4d767e4..e9f5d91 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
@@ -53,11 +53,6 @@ protected RevTree(final AnyObjectId id) {
 	}
 
 	@Override
-	void parse(final RevWalk walk) {
-		flags |= PARSED;
-	}
-	
-	@Override
 	public final int getType() {
 		return Constants.OBJ_TREE;
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
index f567a33..40bdb4e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
@@ -667,15 +667,7 @@ else if (!(c instanceof RevTree))
 					Constants.TYPE_TREE);
 		else
 			t = (RevTree) c;
-
-		if ((t.flags & PARSED) != 0)
-			return t;
-		final ObjectLoader ldr = db.openObject(curs, t);
-		if (ldr == null)
-			throw new MissingObjectException(t, Constants.TYPE_TREE);
-		if (ldr.getType() != Constants.OBJ_TREE)
-			throw new IncorrectObjectTypeException(t, Constants.TYPE_TREE);
-		t.flags |= PARSED;
+		parse(t);
 		return t;
 	}
 
@@ -731,8 +723,8 @@ public RevObject parseAny(final AnyObjectId id)
 				throw new IllegalArgumentException("Bad object type: " + type);
 			}
 			objects.add(r);
-		} else if ((r.flags & PARSED) == 0)
-			r.parse(this);
+		} else
+			parse(r);
 		return r;
 	}
 
-- 
1.6.3.2.367.gf0de

[JGIT PATCH 4/5] Change RevObject dispose() semantics to avoid reparses

[Shawn O. Pearce]

UploadPack was suffering from performance problems because dispose()
was unsetting the PARSED flag, causing another visit to a commit to
reparse the object from disk, even though we already had its parents
in memory.  If a client was 50,000 commits ahead of the server,
then the client could send 50,000 have requests, each time the
server is executing a full reparse of its own commit graph trying
to determine if the common set is sufficient yet.

Instead of asking applications to dispose of commit buffers when the
commit is popped out of the RevWalk we now dispose of the buffer as
soon as the headers are parsed.  For applications that never need
the buffer but have to process a lot of commits in the pending queue,
this can reduce memory usage by a fair amount, as the pending queue
no longer has to retain the buffers until the commits are returned
to the application for buffer disposal.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/RevWalkTestCase.java  |    2 +-
 .../src/org/spearce/jgit/lib/PackWriter.java       |    3 +-
 .../spearce/jgit/revwalk/BoundaryGenerator.java    |    2 +-
 .../spearce/jgit/revwalk/MergeBaseGenerator.java   |    2 +-
 .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |    4 +-
 .../org/spearce/jgit/revwalk/PendingGenerator.java |    6 +-
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |   18 ++++-
 .../src/org/spearce/jgit/revwalk/RevObject.java    |   13 ++--
 .../src/org/spearce/jgit/revwalk/RevTag.java       |   19 ++++-
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |   72 ++++++++++++++++----
 .../spearce/jgit/revwalk/RewriteTreeFilter.java    |    2 +-
 .../src/org/spearce/jgit/transport/UploadPack.java |    4 +-
 .../jgit/transport/WalkFetchConnection.java        |   15 +---
 13 files changed, 111 insertions(+), 51 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/revwalk/RevWalkTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/revwalk/RevWalkTestCase.java
index 0a1e7b8..befc3d5 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/revwalk/RevWalkTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/revwalk/RevWalkTestCase.java
@@ -160,7 +160,7 @@ protected RevTag tag(final String name, final RevObject dst)
 	}
 
 	protected <T extends RevObject> T parse(final T t) throws Exception {
-		rw.parse(t);
+		rw.parseBody(t);
 		return t;
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
index a35f61d..e25fe8f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
@@ -820,6 +820,7 @@ private ObjectWalk setUpWalker(
 			throws MissingObjectException, IOException,
 			IncorrectObjectTypeException {
 		final ObjectWalk walker = new ObjectWalk(db);
+		walker.setRetainBody(false);
 		walker.sort(RevSort.TOPO);
 		walker.sort(RevSort.COMMIT_TIME_DESC, true);
 		if (thin)
@@ -854,12 +855,10 @@ private void findObjectsToPack(final ObjectWalk walker)
 
 		while ((o = walker.next()) != null) {
 			addObject(o);
-			o.dispose();
 			initMonitor.update(1);
 		}
 		while ((o = walker.nextObject()) != null) {
 			addObject(o);
-			o.dispose();
 			initMonitor.update(1);
 		}
 		initMonitor.endTask();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/BoundaryGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/BoundaryGenerator.java
index f167830..6e3d6cf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/BoundaryGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/BoundaryGenerator.java
@@ -115,7 +115,7 @@ RevCommit next() throws MissingObjectException,
 				if ((c.flags & DUPLICATE) != 0)
 					continue;
 				if ((c.flags & PARSED) == 0)
-					c.parse(walk);
+					c.parseHeaders(walk);
 				c.flags |= DUPLICATE;
 				boundary.add(c);
 			}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/MergeBaseGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/MergeBaseGenerator.java
index 8694e4c..afe3dfe 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/MergeBaseGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/MergeBaseGenerator.java
@@ -137,7 +137,7 @@ RevCommit next() throws MissingObjectException,
 				if ((p.flags & IN_PENDING) != 0)
 					continue;
 				if ((p.flags & PARSED) == 0)
-					p.parse(walker);
+					p.parseHeaders(walker);
 				p.flags |= IN_PENDING;
 				pending.add(p);
 			}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
index 484d104..b2329cf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
@@ -134,7 +134,7 @@ public void markStart(RevObject o) throws MissingObjectException,
 		while (o instanceof RevTag) {
 			addObject(o);
 			o = ((RevTag) o).getObject();
-			parse(o);
+			parseHeaders(o);
 		}
 
 		if (o instanceof RevCommit)
@@ -186,7 +186,7 @@ public void markUninteresting(RevObject o) throws MissingObjectException,
 			if (hasRevSort(RevSort.BOUNDARY))
 				addObject(o);
 			o = ((RevTag) o).getObject();
-			parse(o);
+			parseHeaders(o);
 		}
 
 		if (o instanceof RevCommit)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
index 8e3d6bc..0b97f92 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
@@ -135,7 +135,7 @@ RevCommit next() throws MissingObjectException,
 					if ((p.flags & SEEN) != 0)
 						continue;
 					if ((p.flags & PARSED) == 0)
-						p.parse(walker);
+						p.parseHeaders(walker);
 					p.flags |= SEEN;
 					pending.add(p);
 				}
@@ -157,14 +157,14 @@ RevCommit next() throws MissingObjectException,
 						overScan = OVER_SCAN;
 					}
 					if (canDispose)
-						c.dispose();
+						c.disposeBody();
 					continue;
 				}
 
 				if (produce)
 					return last = c;
 				else if (canDispose)
-					c.dispose();
+					c.disposeBody();
 			}
 		} catch (StopWalkException swe) {
 			walker.curs.release();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
index 679718e..f211dfd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
@@ -74,11 +74,21 @@ protected RevCommit(final AnyObjectId id) {
 	}
 
 	@Override
-	void parse(final RevWalk walk) throws MissingObjectException,
+	void parseHeaders(final RevWalk walk) throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
 		parseCanonical(walk, loadCanonical(walk));
 	}
 
+	@Override
+	void parseBody(final RevWalk walk) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		if (buffer == null) {
+			buffer = loadCanonical(walk);
+			if ((flags & PARSED) == 0)
+				parseCanonical(walk, buffer);
+		}
+	}
+
 	void parseCanonical(final RevWalk walk, final byte[] raw) {
 		final MutableObjectId idBuffer = walk.idBuffer;
 		idBuffer.fromString(raw, 5);
@@ -125,7 +135,8 @@ else if (nParents == 1) {
 			commitTime = RawParseUtils.parseBase10(raw, ptr, null);
 		}
 
-		buffer = raw;
+		if (walk.isRetainBody())
+			buffer = raw;
 		flags |= PARSED;
 	}
 	
@@ -375,8 +386,7 @@ public void reset() {
 		inDegree = 0;
 	}
 
-	public void dispose() {
-		flags &= ~PARSED;
+	final void disposeBody() {
 		buffer = null;
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index 0e0386c..024e9ec 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -57,12 +57,18 @@ RevObject(final AnyObjectId name) {
 		super(name);
 	}
 
-	void parse(final RevWalk walk) throws MissingObjectException,
+	void parseHeaders(final RevWalk walk) throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
 		loadCanonical(walk);
 		flags |= PARSED;
 	}
 
+	void parseBody(final RevWalk walk) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		if ((flags & PARSED) == 0)
+			parseHeaders(walk);
+	}
+
 	final byte[] loadCanonical(final RevWalk walk) throws IOException,
 			MissingObjectException, IncorrectObjectTypeException,
 			CorruptObjectException {
@@ -180,11 +186,6 @@ public final void remove(final RevFlagSet set) {
 		flags &= ~set.mask;
 	}
 
-	/** Release as much memory as possible from this object. */
-	public void dispose() {
-		// Nothing needs to be done for most objects.
-	}
-
 	@Override
 	public String toString() {
 		final StringBuilder s = new StringBuilder();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
index e876025..2fab266 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
@@ -69,11 +69,21 @@ protected RevTag(final AnyObjectId id) {
 	}
 
 	@Override
-	void parse(final RevWalk walk) throws MissingObjectException,
+	void parseHeaders(final RevWalk walk) throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
 		parseCanonical(walk, loadCanonical(walk));
 	}
 
+	@Override
+	void parseBody(final RevWalk walk) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		if (buffer == null) {
+			buffer = loadCanonical(walk);
+			if ((flags & PARSED) == 0)
+				parseCanonical(walk, buffer);
+		}
+	}
+
 	void parseCanonical(final RevWalk walk, final byte[] rawTag)
 			throws CorruptObjectException {
 		final MutableInteger pos = new MutableInteger();
@@ -87,7 +97,9 @@ void parseCanonical(final RevWalk walk, final byte[] rawTag)
 		int p = pos.value += 4; // "tag "
 		final int nameEnd = RawParseUtils.nextLF(rawTag, p) - 1;
 		name = RawParseUtils.decode(Constants.CHARSET, rawTag, p, nameEnd);
-		buffer = rawTag;
+
+		if (walk.isRetainBody())
+			buffer = rawTag;
 		flags |= PARSED;
 	}
 
@@ -195,8 +207,7 @@ public final String getName() {
 		return name;
 	}
 
-	public void dispose() {
-		flags &= ~PARSED;
+	final void disposeBody() {
 		buffer = null;
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
index 40bdb4e..5589fb0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
@@ -175,6 +175,8 @@
 
 	private TreeFilter treeFilter;
 
+	private boolean retainBody;
+
 	/**
 	 * Create a new revision walker for a given repository.
 	 * 
@@ -192,6 +194,7 @@ public RevWalk(final Repository repo) {
 		sorting = EnumSet.of(RevSort.NONE);
 		filter = RevFilter.ALL;
 		treeFilter = TreeFilter.ALL;
+		retainBody = true;
 	}
 
 	/**
@@ -237,7 +240,7 @@ public void markStart(final RevCommit c) throws MissingObjectException,
 		if ((c.flags & SEEN) != 0)
 			return;
 		if ((c.flags & PARSED) == 0)
-			c.parse(this);
+			c.parseHeaders(this);
 		c.flags |= SEEN;
 		roots.add(c);
 		queue.add(c);
@@ -510,6 +513,32 @@ public void setTreeFilter(final TreeFilter newFilter) {
 	}
 
 	/**
+	 * Should the body of a commit or tag be retained after parsing its headers?
+	 * <p>
+	 * Usually the body is always retained, but some application code might not
+	 * care and would prefer to discard the body of a commit as early as
+	 * possible, to reduce memory usage.
+	 *
+	 * @return true if the body should be retained; false it is discarded.
+	 */
+	public boolean isRetainBody() {
+		return retainBody;
+	}
+
+	/**
+	 * Set whether or not the body of a commit or tag is retained.
+	 * <p>
+	 * If a body of a commit or tag is not retained, the application must
+	 * call {@link #parseBody(RevObject)} before the body can be safely
+	 * accessed through the type specific access methods.
+	 *
+	 * @param retain true to retain bodies; false to discard them early.
+	 */
+	public void setRetainBody(final boolean retain) {
+		retainBody = retain;
+	}
+
+	/**
 	 * Locate a reference to a blob without loading it.
 	 * <p>
 	 * The blob may or may not exist in the repository. It is impossible to tell
@@ -625,7 +654,7 @@ public RevCommit parseCommit(final AnyObjectId id)
 		RevObject c = parseAny(id);
 		while (c instanceof RevTag) {
 			c = ((RevTag) c).getObject();
-			parse(c);
+			parseHeaders(c);
 		}
 		if (!(c instanceof RevCommit))
 			throw new IncorrectObjectTypeException(id.toObjectId(),
@@ -637,7 +666,7 @@ public RevCommit parseCommit(final AnyObjectId id)
 	 * Locate a reference to a tree.
 	 * <p>
 	 * This method only returns successfully if the tree object exists, is
-	 * verified to be a tree, and was parsed without error.
+	 * verified to be a tree.
 	 *
 	 * @param id
 	 *            name of the tree object, or a commit or annotated tag that may
@@ -656,7 +685,7 @@ public RevTree parseTree(final AnyObjectId id)
 		RevObject c = parseAny(id);
 		while (c instanceof RevTag) {
 			c = ((RevTag) c).getObject();
-			parse(c);
+			parseHeaders(c);
 		}
 
 		final RevTree t;
@@ -667,12 +696,12 @@ else if (!(c instanceof RevTree))
 					Constants.TYPE_TREE);
 		else
 			t = (RevTree) c;
-		parse(t);
+		parseHeaders(t);
 		return t;
 	}
 
 	/**
-	 * Locate a reference to any object and immediately parse its content.
+	 * Locate a reference to any object and immediately parse its headers.
 	 * <p>
 	 * This method only returns successfully if the object exists and was parsed
 	 * without error. Parsing an object can be expensive as the type must be
@@ -724,12 +753,12 @@ public RevObject parseAny(final AnyObjectId id)
 			}
 			objects.add(r);
 		} else
-			parse(r);
+			parseHeaders(r);
 		return r;
 	}
 
 	/**
-	 * Ensure the object's content has been parsed.
+	 * Ensure the object's critical headers have been parsed.
 	 * <p>
 	 * This method only returns successfully if the object exists and was parsed
 	 * without error.
@@ -741,11 +770,28 @@ public RevObject parseAny(final AnyObjectId id)
 	 * @throws IOException
 	 *             a pack file or loose object could not be read.
 	 */
-	public void parse(final RevObject obj) throws MissingObjectException,
-			IOException {
-		if ((obj.flags & PARSED) != 0)
-			return;
-		obj.parse(this);
+	public void parseHeaders(final RevObject obj)
+			throws MissingObjectException, IOException {
+		if ((obj.flags & PARSED) == 0)
+			obj.parseHeaders(this);
+	}
+
+	/**
+	 * Ensure the object's fully body content is available.
+	 * <p>
+	 * This method only returns successfully if the object exists and was parsed
+	 * without error.
+	 * 
+	 * @param obj
+	 *            the object the caller needs to be parsed.
+	 * @throws MissingObjectException
+	 *             the supplied does not exist.
+	 * @throws IOException
+	 *             a pack file or loose object could not be read.
+	 */
+	public void parseBody(final RevObject obj)
+			throws MissingObjectException, IOException {
+		obj.parseBody(this);
 	}
 
 	/**
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RewriteTreeFilter.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RewriteTreeFilter.java
index a5edbf0..754be07 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RewriteTreeFilter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RewriteTreeFilter.java
@@ -94,7 +94,7 @@ public boolean include(final RevWalk walker, final RevCommit c)
 		for (int i = 0; i < nParents; i++) {
 			final RevCommit p = c.parents[i];
 			if ((p.flags & PARSED) == 0)
-				p.parse(walker);
+				p.parseHeaders(walker);
 			trees[i] = p.getTree();
 		}
 		trees[nParents] = c.getTree();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
index 45d57b3..5127dad 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
@@ -137,6 +137,7 @@
 	public UploadPack(final Repository copyFrom) {
 		db = copyFrom;
 		walk = new RevWalk(db);
+		walk.setRetainBody(false);
 
 		ADVERTISED = walk.newFlag("ADVERTISED");
 		WANT = walk.newFlag("WANT");
@@ -270,7 +271,7 @@ private void writeAdvertisedTag(final StringBuilder m, final char[] idtmp,
 		while (o instanceof RevTag) {
 			// Fully unwrap here so later on we have these already parsed.
 			try {
-				walk.parse(((RevTag) o).getObject());
+				walk.parseHeaders(((RevTag) o).getObject());
 			} catch (IOException err) {
 				return;
 			}
@@ -434,7 +435,6 @@ private boolean wantSatisfied(final RevCommit want) throws IOException {
 				}
 				return true;
 			}
-			c.dispose();
 		}
 		return false;
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
index 4d14305..6929c5f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
@@ -193,6 +193,7 @@ WalkFetchConnection(final WalkTransport t, final WalkRemoteObjectDatabase w) {
 		packLocks = new ArrayList<PackLock>(4);
 
 		revWalk = new RevWalk(local);
+		revWalk.setRetainBody(false);
 		treeWalk = new TreeWalk(local);
 		COMPLETE = revWalk.newFlag("COMPLETE");
 		IN_WORK_QUEUE = revWalk.newFlag("IN_WORK_QUEUE");
@@ -266,7 +267,7 @@ private void process(final ObjectId id) throws TransportException {
 				obj = (RevObject) id;
 				if (obj.has(COMPLETE))
 					return;
-				revWalk.parse(obj);
+				revWalk.parseHeaders(obj);
 			} else {
 				obj = revWalk.parseAny(id);
 				if (obj.has(COMPLETE))
@@ -276,12 +277,6 @@ private void process(final ObjectId id) throws TransportException {
 			throw new TransportException("Cannot read " + id.name(), e);
 		}
 
-		// We only care about traversal; disposing an object throws its
-		// message buffer (if any) away but retains the links so we can
-		// continue to navigate, but use less memory long-term.
-		//
-		obj.dispose();
-
 		switch (obj.getType()) {
 		case Constants.OBJ_BLOB:
 			processBlob(obj);	
@@ -694,9 +689,8 @@ private void markLocalRefsComplete(final Set<ObjectId> have) throws TransportExc
 	private void markLocalObjComplete(RevObject obj) throws IOException {
 		while (obj.getType() == Constants.OBJ_TAG) {
 			obj.add(COMPLETE);
-			obj.dispose();
 			obj = ((RevTag) obj).getObject();
-			revWalk.parse(obj);
+			revWalk.parseHeaders(obj);
 		}
 
 		switch (obj.getType()) {
@@ -734,11 +728,10 @@ private void pushLocalCommit(final RevCommit p)
 			throws MissingObjectException, IOException {
 		if (p.has(LOCALLY_SEEN))
 			return;
-		revWalk.parse(p);
+		revWalk.parseHeaders(p);
 		p.add(LOCALLY_SEEN);
 		p.add(COMPLETE);
 		p.carry(COMPLETE);
-		p.dispose();
 		localCommitQueue.add(p);
 	}
 
-- 
1.6.3.2.367.gf0de

[JGIT PATCH 5/5] UploadPack: Only recompute okToGiveUp() if bases changed

[Shawn O. Pearce]

If we get a commit we don't recognize, but we haven't added any new
common bases to the commonBase set since the last time we ran the
okToGiveUp() algorithm, then there is no point in running it again,
as the result will not change.  Instead we cache the result in a
Boolean and just return the cached result until another common base
can be found and added to the set.

This helps to fix a performance problem when the client is 50,000
commits ahead of the server, and keeps sending them in "have"
lines, but the server doesn't recognize them.  On every "have"
line we will not have updated the common base, but we also aren't
yet satisfied that it is OK to give up negotiation.

Between the changes introduced by the parent commit and this commit,
negotiation in this case of 50,000 ahead now completes instantly,
instead of taking hours.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/transport/UploadPack.java |   27 ++++++++++++++------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
index 5127dad..5c8df62 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/UploadPack.java
@@ -112,6 +112,9 @@
 	/** Objects on both sides, these don't have to be sent. */
 	private final List<RevObject> commonBase = new ArrayList<RevObject>();
 
+	/** null if {@link #commonBase} should be examined again. */
+	private Boolean okToGiveUp;
+
 	/** Marked on objects we sent in our advertisement list. */
 	private final RevFlag ADVERTISED;
 
@@ -396,15 +399,26 @@ private boolean matchHave(final ObjectId id) {
 			o.add(PEER_HAS);
 			if (o instanceof RevCommit)
 				((RevCommit) o).carry(PEER_HAS);
-			if (!o.has(COMMON)) {
-				o.add(COMMON);
-				commonBase.add(o);
-			}
+			addCommonBase(o);
 		}
 		return true;
 	}
 
+	private void addCommonBase(final RevObject o) {
+		if (!o.has(COMMON)) {
+			o.add(COMMON);
+			commonBase.add(o);
+			okToGiveUp = null;
+		}
+	}
+
 	private boolean okToGiveUp() throws PackProtocolException {
+		if (okToGiveUp == null)
+			okToGiveUp = Boolean.valueOf(okToGiveUpImp());
+		return okToGiveUp.booleanValue();
+	}
+
+	private boolean okToGiveUpImp() throws PackProtocolException {
 		if (commonBase.isEmpty())
 			return false;
 
@@ -429,10 +443,7 @@ private boolean wantSatisfied(final RevCommit want) throws IOException {
 			if (c == null)
 				break;
 			if (c.has(PEER_HAS)) {
-				if (!c.has(COMMON)) {
-					c.add(COMMON);
-					commonBase.add(c);
-				}
+				addCommonBase(c);
 				return true;
 			}
 		}
-- 
1.6.3.2.367.gf0de