MergeSuperSet: Avoid quadratic behavior by walking once per target ref

When multiple changes in a line of history are already part of the
ChangeSet, Gerrit repeats work by walking history for each of the
changes separately. For example, suppose a linear sequence of n changes
are discovered as part of a topic; then we visit O(n^2) revisions that
we already knew.

Speed it up by using a single RevWalk per target branch.

Change-Id: I8d27f36bb508a06c4bdb6fe5c51bc666c0088096
Helped-By: Jonathan Nieder <jrn@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java

@@ -18,6 +18,8 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Strings;
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.common.data.SubmitTypeRecord;
@@ -51,6 +53,8 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -149,17 +153,61 @@ public class MergeSuperSet {
     return str.type;
   }
 
+  private static ImmutableListMultimap<Branch.NameKey, ChangeData>
+      byBranch(Iterable<ChangeData> changes) throws OrmException {
+    ImmutableListMultimap.Builder<Branch.NameKey, ChangeData> builder =
+        ImmutableListMultimap.builder();
+    for (ChangeData cd : changes) {
+      builder.put(cd.change().getDest(), cd);
+    }
+    return builder.build();
+  }
+
+  private Set<String> walkChangesByHashes(Collection<RevCommit> sourceCommits,
+      Set<String> ignoreHashes, OpenRepo or, Optional<RevCommit> head)
+          throws IOException {
+    Set<String> destHashes = new HashSet<>();
+    or.rw.reset();
+    if (head.isPresent()) {
+      or.rw.markUninteresting(head.get());
+    }
+    for (RevCommit c : sourceCommits) {
+      String name = c.name();
+      if (ignoreHashes.contains(name)) {
+        continue;
+      }
+      destHashes.add(name);
+      or.rw.markStart(c);
+    }
+    for (RevCommit c : or.rw) {
+      String name = c.name();
+      if (ignoreHashes.contains(name)) {
+        continue;
+      }
+      destHashes.add(name);
+    }
+
+    return destHashes;
+  }
+
   private ChangeSet completeChangeSetWithoutTopic(ReviewDb db,
       ChangeSet changes, CurrentUser user) throws IOException, OrmException {
-    List<ChangeData> visibleChanges = new ArrayList<>();
+    Collection<ChangeData> visibleChanges = new ArrayList<>();
-    List<ChangeData> nonVisibleChanges = new ArrayList<>();
+    Collection<ChangeData> nonVisibleChanges = new ArrayList<>();
 
-    for (ChangeData cd :
+    // For each target branch we run a separate rev walk to find open changes
-        Iterables.concat(changes.changes(), changes.nonVisibleChanges())) {
+    // reachable from changes already in the merge super set.
+    ImmutableListMultimap<Branch.NameKey, ChangeData> bc = byBranch(
+        Iterables.concat(changes.changes(), changes.nonVisibleChanges()));
+    for (Branch.NameKey b : bc.keySet()) {
+      OpenRepo or = getRepo(b.getParentKey());
+      List<RevCommit> visibleCommits = new ArrayList<>();
+      List<RevCommit> nonVisibleCommits = new ArrayList<>();
+      for (ChangeData cd : bc.get(b)) {
         checkState(cd.hasChangeControl(),
             "completeChangeSet forgot to set changeControl for current user"
                 + " at ChangeData creation time");
-      OpenRepo or = getRepo(cd.change().getProject());
+
         boolean visible = changes.ids().contains(cd.getId());
         if (visible && !cd.changeControl().isVisible(db, cd)) {
           // We thought the change was visible, but it isn't.
@@ -167,7 +215,8 @@ public class MergeSuperSet {
           // completeChangeSet computation, for example.
           visible = false;
         }
-      List<ChangeData> dest = visible ? visibleChanges : nonVisibleChanges;
+        Collection<RevCommit> toWalk = visible ?
+            visibleCommits : nonVisibleCommits;
 
         // Pick a revision to use for traversal.  If any of the patch sets
         // is visible, we use the most recent one.  Otherwise, use the current
@@ -184,7 +233,12 @@ public class MergeSuperSet {
         }
 
         if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) {
-        dest.add(cd);
+          if (visible) {
+            visibleChanges.add(cd);
+          } else {
+            nonVisibleChanges.add(cd);
+          }
+
           continue;
         }
 
@@ -192,37 +246,33 @@ public class MergeSuperSet {
         String objIdStr = ps.getRevision().get();
         RevCommit commit = or.rw.parseCommit(ObjectId.fromString(objIdStr));
 
-      // Collect unmerged ancestors
-      Branch.NameKey destBranch = cd.change().getDest();
-      Ref ref = or.repo.getRefDatabase().getRef(destBranch.get());
-
-      or.rw.reset();
-      or.rw.markStart(commit);
-      if (ref != null) {
-        RevCommit head = or.rw.parseCommit(ref.getObjectId());
-        or.rw.markUninteresting(head);
-      }
-
-      List<String> hashes = new ArrayList<>();
         // Always include the input, even if merged. This allows
         // SubmitStrategyOp to correct the situation later, assuming it gets
         // returned by byCommitsOnBranchNotMerged below.
-      hashes.add(objIdStr);
+        toWalk.add(commit);
-      for (RevCommit c : or.rw) {
-        if (!c.equals(commit)) {
-          hashes.add(c.name());
-        }
       }
 
-      if (!hashes.isEmpty()) {
+      Ref ref = or.repo.getRefDatabase().getRef(b.get());
-        Iterable<ChangeData> destChanges = query()
+      Optional<RevCommit> head =
-            .byCommitsOnBranchNotMerged(
+          ref != null
-              or.repo, db, cd.change().getDest(), hashes);
+              ? Optional.<RevCommit>of(or.rw.parseCommit(ref.getObjectId()))
-        for (ChangeData chd : destChanges) {
+              : Optional.<RevCommit>absent();
+
+      Set<String> emptySet = Collections.emptySet();
+      Set<String> visibleHashes = walkChangesByHashes(visibleCommits,
+          emptySet, or, head);
+
+      Iterable<ChangeData> cds = query()
+          .byCommitsOnBranchNotMerged(or.repo, db, b, visibleHashes);
+      for (ChangeData chd : cds) {
         chd.changeControl(user);
-          dest.add(chd);
+        visibleChanges.add(chd);
-        }
       }
+
+      Set<String> nonVisibleHashes = walkChangesByHashes(nonVisibleCommits,
+          visibleHashes, or, head);
+      Iterables.addAll(nonVisibleChanges,
+          query().byCommitsOnBranchNotMerged(or.repo, db, b, nonVisibleHashes));
     }
 
     return new ChangeSet(visibleChanges, nonVisibleChanges);

gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java

@@ -153,7 +153,7 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
   }
 
   public Iterable<ChangeData> byCommitsOnBranchNotMerged(Repository repo,
-      ReviewDb db, Branch.NameKey branch, List<String> hashes)
+      ReviewDb db, Branch.NameKey branch, Collection<String> hashes)
       throws OrmException, IOException {
     return byCommitsOnBranchNotMerged(repo, db, branch, hashes,
         // Account for all commit predicates plus ref, project, status.
@@ -162,7 +162,7 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
 
   @VisibleForTesting
   Iterable<ChangeData> byCommitsOnBranchNotMerged(Repository repo, ReviewDb db,
-      Branch.NameKey branch, List<String> hashes, int indexLimit)
+      Branch.NameKey branch, Collection<String> hashes, int indexLimit)
       throws OrmException, IOException {
     if (hashes.size() > indexLimit) {
       return byCommitsOnBranchNotMergedFromDatabase(repo, db, branch, hashes);
@@ -172,7 +172,7 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
 
   private Iterable<ChangeData> byCommitsOnBranchNotMergedFromDatabase(
       Repository repo, final ReviewDb db, final Branch.NameKey branch,
-      List<String> hashes) throws OrmException, IOException {
+      Collection<String> hashes) throws OrmException, IOException {
     Set<Change.Id> changeIds = Sets.newHashSetWithExpectedSize(hashes.size());
     String lastPrefix = null;
     for (Ref ref :
@@ -208,7 +208,7 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
   }
 
   private Iterable<ChangeData> byCommitsOnBranchNotMergedFromIndex(
-      Branch.NameKey branch, List<String> hashes) throws OrmException {
+      Branch.NameKey branch, Collection<String> hashes) throws OrmException {
     return query(and(
         ref(branch),
         project(branch.getParentKey()),
@@ -216,7 +216,7 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
         or(commits(hashes))));
   }
 
-  private static List<Predicate<ChangeData>> commits(List<String> hashes) {
+  private static List<Predicate<ChangeData>> commits(Collection<String> hashes) {
     List<Predicate<ChangeData>> commits = new ArrayList<>(hashes.size());
     for (String s : hashes) {
       commits.add(commit(s));