From 437be8dfade74b039eeee24e400addcbb8fe2ca9 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 8 Mar 2013 12:25:12 -0800 Subject: [PATCH] Simplify UploadPack by parsing wants separately from haves The DHT backend was very slow at parsing objects. To work around that performance limitation I obfuscated UploadPack by folding both the want and have sets together in a single parse queue. Since DHT was removed the complexity is no longer constructive to JGit. Doing this refactoring prepares the code for a slightly future change where the have lines need to be handled specially from the want lines. Splitting the parsing up into two phases makes such a modification trivial. Change-Id: If7aad533b82448bbb688278e21f709282e5ccf4b --- .../eclipse/jgit/transport/UploadPack.java | 143 ++++++++---------- 1 file changed, 60 insertions(+), 83 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 bc4045416..f0ba0cdc5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -787,74 +787,23 @@ private ObjectId processHaveLines(List peerHas, ObjectId last) preUploadHook.onBeginNegotiateRound(this, wantIds, peerHas.size()); if (peerHas.isEmpty()) return last; + if (wantAll.isEmpty() && !wantIds.isEmpty()) + parseWants(); - List toParse = peerHas; - HashSet peerHasSet = null; - boolean needMissing = false; sentReady = 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; - } - - Set notAdvertisedWants = null; int haveCnt = 0; - AsyncRevObjectQueue q = walk.parseAny(toParse, needMissing); + AsyncRevObjectQueue q = walk.parseAny(peerHas, false); try { for (;;) { RevObject obj; try { obj = q.next(); } catch (MissingObjectException notFound) { - ObjectId id = notFound.getObjectId(); - if (wantIds.contains(id)) { - String msg = MessageFormat.format( - JGitText.get().wantNotValid, id.name()); - throw new PackProtocolException(msg, 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) && requestPolicy != RequestPolicy.ANY) { - if (notAdvertisedWants == null) - notAdvertisedWants = new HashSet(); - notAdvertisedWants.add(obj); - } - - if (!obj.has(WANT)) { - obj.add(WANT); - wantAll.add(obj); - } - - if (!(obj instanceof RevCommit)) - obj.add(SATISFIED); - - 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; haveCnt++; @@ -891,25 +840,6 @@ private ObjectId processHaveLines(List peerHas, ObjectId last) q.release(); } - // If the client asked for non advertised object, check our policy. - if (notAdvertisedWants != null && !notAdvertisedWants.isEmpty()) { - switch (requestPolicy) { - case ADVERTISED: - default: - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, - notAdvertisedWants.iterator().next().name())); - - case REACHABLE_COMMIT: - checkNotAdvertisedWants(notAdvertisedWants); - break; - - case ANY: - // Allow whatever was asked for. - break; - } - } - int missCnt = peerHas.size() - haveCnt; // If we don't have one of the objects but we're also willing to @@ -952,7 +882,61 @@ private ObjectId processHaveLines(List peerHas, ObjectId last) return last; } - private void checkNotAdvertisedWants(Set notAdvertisedWants) + private void parseWants() throws IOException { + AsyncRevObjectQueue q = walk.parseAny(wantIds, true); + try { + List checkReachable = null; + RevObject obj; + while ((obj = q.next()) != null) { + if (!advertised.contains(obj)) { + switch (requestPolicy) { + case ADVERTISED: + default: + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, obj)); + case REACHABLE_COMMIT: + if (!(obj instanceof RevCommit)) { + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, obj)); + } + if (checkReachable == null) + checkReachable = new ArrayList(); + checkReachable.add((RevCommit) obj); + break; + case ANY: + break; + } + } + want(obj); + + if (!(obj instanceof RevCommit)) + obj.add(SATISFIED); + if (obj instanceof RevTag) { + obj = walk.peel(obj); + if (obj instanceof RevCommit) + want(obj); + } + } + if (checkReachable != null) + checkNotAdvertisedWants(checkReachable); + wantIds.clear(); + } catch (MissingObjectException notFound) { + ObjectId id = notFound.getObjectId(); + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, id.name()), notFound); + } finally { + q.release(); + } + } + + private void want(RevObject obj) { + if (!obj.has(WANT)) { + obj.add(WANT); + wantAll.add(obj); + } + } + + private void checkNotAdvertisedWants(List notAdvertisedWants) throws MissingObjectException, IncorrectObjectTypeException, IOException { // Walk the requested commits back to the advertised commits. // If any commit exists, a branch was deleted or rewound and @@ -960,15 +944,8 @@ private void checkNotAdvertisedWants(Set notAdvertisedWants) // If the requested commit is merged into an advertised branch // it will be marked UNINTERESTING and no commits return. - for (RevObject o : notAdvertisedWants) { - if (!(o instanceof RevCommit)) { - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, - notAdvertisedWants.iterator().next().name())); - } - walk.markStart((RevCommit) o); - } - + for (RevCommit c : notAdvertisedWants) + walk.markStart(c); for (ObjectId id : advertised) { try { walk.markUninteresting(walk.parseCommit(id));