From 2096c749c30c2420362859383bbdd1e307790569 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 3 Feb 2011 12:37:39 -0800 Subject: [PATCH] UploadPack: Avoid parsing want list on clone If a client wants to perform a clone of the repository, it sends wants, but no haves. There is no point in parsing the want list within UploadPack, as there won't be a common merge base search. Instead just defer the parsing to PackWriter, which will do its own parsing and object enumeration. If the client does have a "have" set, defer parsing of the want list until the have list is also parsed, and parse them together in a single batch queue. This lets the underlying storage system use a larger lookup batch if there is significant latency involved when resolving an ObjectId to a RevObject. Change-Id: I9c30d34f8e344da05c8a2c041a6dc181d8e8bc19 Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/transport/UploadPack.java | 106 ++++++++++-------- 1 file changed, 60 insertions(+), 46 deletions(-) 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 81da24792..4ff1c5c5a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -49,6 +49,7 @@ import java.io.OutputStream; import java.text.MessageFormat; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -143,6 +144,9 @@ public class UploadPack { /** Capabilities requested by the client. */ private final Set options = new HashSet(); + /** Raw ObjectIds the client has asked for, before validating them. */ + private final Set wantIds = new HashSet(); + /** Objects the client wants to obtain. */ private final List wantAll = new ArrayList(); @@ -335,7 +339,7 @@ private void service() throws IOException { } recvWants(); - if (wantAll.isEmpty()) + if (wantIds.isEmpty()) return; if (options.contains(OPTION_MULTI_ACK_DETAILED)) @@ -374,7 +378,6 @@ public void sendAdvertisedRefs(final RefAdvertiser adv) throws IOException { } private void recvWants() throws IOException { - HashSet wantIds = new HashSet(); boolean isFirst = true; for (;;) { String line; @@ -403,46 +406,6 @@ private void recvWants() throws IOException { wantIds.add(ObjectId.fromString(line.substring(5))); isFirst = false; } - - if (wantIds.isEmpty()) - return; - - AsyncRevObjectQueue q = walk.parseAny(wantIds, true); - try { - for (;;) { - RevObject o; - try { - o = q.next(); - } catch (IOException error) { - throw new PackProtocolException(MessageFormat.format( - JGitText.get().notValid, error.getMessage()), error); - } - if (o == null) - break; - if (o.has(WANT)) { - // Already processed, the client repeated itself. - - } else if (advertised.contains(o)) { - o.add(WANT); - wantAll.add(o); - - if (o instanceof RevTag) { - o = walk.peel(o); - if (o instanceof RevCommit) { - if (!o.has(WANT)) { - o.add(WANT); - wantAll.add(o); - } - } - } - } else { - throw new PackProtocolException(MessageFormat.format( - JGitText.get().notValid, o.name())); - } - } - } finally { - q.release(); - } } private boolean negotiate() throws IOException { @@ -489,20 +452,65 @@ private ObjectId processHaveLines(List peerHas, ObjectId last) if (peerHas.isEmpty()) return last; - // If both sides have the same object; let the client know. - // - AsyncRevObjectQueue q = walk.parseAny(peerHas, false); + List toParse = peerHas; + HashSet peerHasSet = null; + boolean needMissing = false; + + if (wantAll.isEmpty() && !wantIds.isEmpty()) { + // We have not yet parsed the want list. Parse it now. + peerHasSet = new HashSet(peerHas); + int cnt = wantIds.size() + peerHasSet.size(); + toParse = new ArrayList(cnt); + toParse.addAll(wantIds); + toParse.addAll(peerHasSet); + needMissing = true; + } + + AsyncRevObjectQueue q = walk.parseAny(toParse, needMissing); try { for (;;) { RevObject obj; try { obj = q.next(); } catch (MissingObjectException notFound) { + if (wantIds.contains(notFound.getObjectId())) { + throw new PackProtocolException( + MessageFormat.format(JGitText.get().notValid, + notFound.getMessage()), notFound); + } continue; } if (obj == null) break; + // If the object is still found in wantIds, the want + // list wasn't parsed earlier, and was done in this batch. + // + if (wantIds.remove(obj)) { + if (!advertised.contains(obj)) { + throw new PackProtocolException(MessageFormat.format( + JGitText.get().notValid, obj.name())); + } + + if (!obj.has(WANT)) { + obj.add(WANT); + wantAll.add(obj); + } + + if (obj instanceof RevTag) { + RevObject target = walk.peel(obj); + if (target instanceof RevCommit) { + if (!target.has(WANT)) { + target.add(WANT); + wantAll.add(target); + } + } + } + + if (!peerHasSet.contains(obj)) + continue; + } + last = obj; if (obj.has(PEER_HAS)) continue; @@ -512,6 +520,8 @@ private ObjectId processHaveLines(List peerHas, ObjectId last) ((RevCommit) obj).carry(PEER_HAS); addCommonBase(obj); + // If both sides have the same object; let the client know. + // switch (multiAck) { case OFF: if (commonBase.size() == 1) @@ -631,6 +641,10 @@ private void sendPack() throws IOException { } } + Collection want = wantAll; + if (want.isEmpty()) + want = wantIds; + PackConfig cfg = packConfig; if (cfg == null) cfg = new PackConfig(db); @@ -639,7 +653,7 @@ private void sendPack() throws IOException { pw.setUseCachedPacks(true); pw.setDeltaBaseAsOffset(options.contains(OPTION_OFS_DELTA)); pw.setThin(options.contains(OPTION_THIN_PACK)); - pw.preparePack(pm, wantAll, commonBase); + pw.preparePack(pm, want, commonBase); if (options.contains(OPTION_INCLUDE_TAG)) { for (final Ref r : refs.values()) { final RevObject o;