From b1944c0fc0637fcab03e636a40092b2f252980ab Mon Sep 17 00:00:00 2001 From: Sasa Zivkov Date: Thu, 14 Nov 2013 16:07:50 +0100 Subject: [PATCH] canReadCommit: further improve performance by using IncludedInResolver When searching for a commit the IncludedInResolver avoids traversing the same commit (sub)graph for the second time if it already failed to find that commit once in that (sub)graph. Partition the refs into two sets: after = commits with time >= target.time before = commits with time < target.time It is highly likely that the target is reachable from the "after" set. It could be reachable from the "before" set only in case of a clock skew. Sorting both "after" and "before" sets by time will increase the probability that a cached failure to find a commit will be reused when traversing the commit graph from newer commits. Before invoking the IncludedInResolver, filter out all refs for which the user doesn't have the READ access. Also, avoid fetching all refs from the ref database. This change required some refactoring of the IncludedInResolver class. Change-Id: Ic5651bb0b71701ad64920e195ab735038edef377 --- .../server/change/IncludedInResolver.java | 116 ++++++++++++++---- .../gerrit/server/project/ProjectControl.java | 35 +++--- 2 files changed, 106 insertions(+), 45 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedInResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedInResolver.java index 9c4817e5b9..201ee149a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedInResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedInResolver.java @@ -49,7 +49,31 @@ public class IncludedInResolver { public static IncludedInDetail resolve(final Repository repo, final RevWalk rw, final RevCommit commit) throws IOException { + return new IncludedInResolver(repo, rw, commit).resolve(); + } + public static boolean includedInOne(final Repository repo, final RevWalk rw, + final RevCommit commit, final Collection refs) throws IOException { + return new IncludedInResolver(repo, rw, commit).includedInOne(refs); + } + + private final Repository repo; + private final RevWalk rw; + private final RevCommit target; + + private final RevFlag containsTarget; + private Multimap commitToRef; + private List tipsByCommitTime; + + private IncludedInResolver(final Repository repo, final RevWalk rw, + final RevCommit target) { + this.repo = repo; + this.rw = rw; + this.target = target; + this.containsTarget = rw.newFlag("CONTAINS_TARGET"); + } + + private IncludedInDetail resolve() throws IOException { RefDatabase refDb = repo.getRefDatabase(); Collection tags = refDb.getRefs(Constants.R_TAGS).values(); Collection branches = refDb.getRefs(Constants.R_HEADS).values(); @@ -57,8 +81,8 @@ public class IncludedInResolver { tags.size() + branches.size()); allTagsAndBranches.addAll(tags); allTagsAndBranches.addAll(branches); - Set allMatchingTagsAndBranches = - includedIn(repo, rw, commit, allTagsAndBranches); + parseCommits(allTagsAndBranches); + Set allMatchingTagsAndBranches = includedIn(tipsByCommitTime, 0); IncludedInDetail detail = new IncludedInDetail(); detail @@ -68,27 +92,22 @@ public class IncludedInResolver { return detail; } + private boolean includedInOne(final Collection refs) throws IOException { + parseCommits(refs); + List before = Lists.newLinkedList(); + List after = Lists.newLinkedList(); + partition(before, after); + // It is highly likely that the target is reachable from the "after" set + // Within the "before" set we are trying to handle cases arising from clock skew + return !includedIn(after, 1).isEmpty() || !includedIn(before, 1).isEmpty(); + } + /** * Resolves which tip refs include the target commit. */ - private static Set includedIn(final Repository repo, final RevWalk rw, - final RevCommit target, final Collection tipRefs) throws IOException, - MissingObjectException, IncorrectObjectTypeException { - + private Set includedIn(final Collection tips, int limit) + throws IOException, MissingObjectException, IncorrectObjectTypeException { Set result = Sets.newHashSet(); - - Multimap tipsAndCommits = parseCommits(repo, rw, tipRefs); - - List tips = Lists.newArrayList(tipsAndCommits.keySet()); - Collections.sort(tips, new Comparator() { - @Override - public int compare(RevCommit c1, RevCommit c2) { - return c1.getCommitTime() - c2.getCommitTime(); - } - }); - - RevFlag containsTarget = rw.newFlag("CONTAINS_TARGET"); - for (RevCommit tip : tips) { boolean commitFound = false; rw.resetRetain(RevFlag.UNINTERESTING, containsTarget); @@ -97,18 +116,51 @@ public class IncludedInResolver { if (commit.equals(target) || commit.has(containsTarget)) { commitFound = true; tip.add(containsTarget); - result.addAll(tipsAndCommits.get(tip)); + result.addAll(commitToRef.get(tip)); break; } } if (!commitFound) { rw.markUninteresting(tip); + } else if (0 < limit && limit < result.size()) { + break; } } - return result; } + /** + * Partition the reference tips into two sets: + *
    + *
  • before = commits with time < target.getCommitTime() + *
  • after = commits with time >= target.getCommitTime() + *
+ * + * Each of the before/after lists is sorted by the the commit time. + * + * @param before + * @param after + */ + private void partition(final List before, + final List after) { + int insertionPoint = Collections.binarySearch(tipsByCommitTime, target, + new Comparator() { + @Override + public int compare(RevCommit c1, RevCommit c2) { + return c1.getCommitTime() - c2.getCommitTime(); + } + }); + if (insertionPoint < 0) { + insertionPoint = - (insertionPoint + 1); + } + if (0 < insertionPoint) { + before.addAll(tipsByCommitTime.subList(0, insertionPoint)); + } + if (insertionPoint < tipsByCommitTime.size()) { + after.addAll(tipsByCommitTime.subList(insertionPoint, tipsByCommitTime.size())); + } + } + /** * Returns the short names of refs which are as well in the matchingRefs list * as well as in the allRef list. @@ -127,9 +179,11 @@ public class IncludedInResolver { /** * Parse commit of ref and store the relation between ref and commit. */ - private static Multimap parseCommits(final Repository repo, - final RevWalk rw, final Collection refs) throws IOException { - Multimap result = LinkedListMultimap.create(); + private void parseCommits(final Collection refs) throws IOException { + if (commitToRef != null) { + return; + } + commitToRef = LinkedListMultimap.create(); for (Ref ref : refs) { final RevCommit commit; try { @@ -146,8 +200,18 @@ public class IncludedInResolver { + " points to dangling object " + ref.getObjectId()); continue; } - result.put(commit, ref.getName()); + commitToRef.put(commit, ref.getName()); } - return result; + tipsByCommitTime = Lists.newArrayList(commitToRef.keySet()); + sortOlderFirst(tipsByCommitTime); + } + + private void sortOlderFirst(final List tips) { + Collections.sort(tips, new Comparator() { + @Override + public int compare(RevCommit c1, RevCommit c2) { + return c1.getCommitTime() - c2.getCommitTime(); + } + }); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 6c6cba490e..cc929fd234 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.project; -import static org.eclipse.jgit.lib.RefDatabase.ALL; - import com.google.common.collect.Lists; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.PageLinks; @@ -34,6 +32,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.InternalUser; +import com.google.gerrit.server.change.IncludedInResolver; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GitReceivePackGroups; import com.google.gerrit.server.config.GitUploadPackGroups; @@ -42,8 +41,9 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; -import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -58,7 +58,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.Map.Entry; /** Access control management for a user accessing a project's data. */ public class ProjectControl { @@ -498,23 +497,21 @@ public class ProjectControl { try { Repository repo = repoManager.openRepository(projName); try { - Map allRefs = repo.getRefDatabase().getRefs(ALL); - for (Entry entry : allRefs.entrySet()) { - String refName = entry.getKey(); - if (!refName.startsWith("refs/heads") && !refName.startsWith("refs/tags")) { - continue; - } - RevCommit tip; - try { - tip = rw.parseCommit(entry.getValue().getObjectId()); - } catch (IncorrectObjectTypeException e) { - continue; - } - if (controlForRef(entry.getKey()).canPerform(Permission.READ) - && rw.isMergedInto(commit, tip)) { - return true; + RefDatabase refDb = repo.getRefDatabase(); + List allRefs = Lists.newLinkedList(); + allRefs.addAll(refDb.getRefs(Constants.R_HEADS).values()); + allRefs.addAll(refDb.getRefs(Constants.R_TAGS).values()); + List canReadRefs = Lists.newLinkedList(); + for (Ref r : allRefs) { + if (controlForRef(r.getName()).canPerform(Permission.READ)) { + canReadRefs.add(r); } } + + if (!canReadRefs.isEmpty() && IncludedInResolver.includedInOne( + repo, rw, commit, canReadRefs)) { + return true; + } } finally { repo.close(); }