MergeSuperSet: Avoid some repeated index queries

When the main completeChangeSet loop iterates multiple times, it will
quite likely re-walk some identical sections of history in the branch,
discovering identical sets of commits to look up in the index. Avoid
such repeated queries with a simple per-instance query cache.

We could be a little smarter and try to map commit SHA-1s to changes,
so as to be able to reuse some results from the query even if the
exact set of discovered commits is not identical. However, this turns
out to be more complicated than it first appears, because we don't
know when looking at a search result _which_ commit in the input
caused that change to be returned, so we have to iterate over all
patch sets, which is less than ideal. This change is simpler and still
better than nothing.

Change-Id: Ia5deb02be96fb8b941c3b25c2b9d821440fcde80

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

@@ -17,8 +17,10 @@ package com.google.gerrit.server.git;
 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.auto.value.AutoValue;
 import com.google.common.base.Optional;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -55,8 +57,10 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -81,10 +85,23 @@ public class MergeSuperSet {
     }
   }
 
+  @AutoValue
+  abstract static class QueryKey {
+    private static QueryKey create(
+        Branch.NameKey branch, Iterable<String> hashes) {
+      return new AutoValue_MergeSuperSet_QueryKey(
+          branch, ImmutableSet.copyOf(hashes));
+    }
+
+    abstract Branch.NameKey branch();
+    abstract ImmutableSet<String> hashes();
+  }
+
   private final ChangeData.Factory changeDataFactory;
   private final Provider<InternalChangeQuery> queryProvider;
   private final Provider<MergeOpRepoManager> repoManagerProvider;
   private final Config cfg;
+  private final Map<QueryKey, List<ChangeData>> queryCache;
 
   private MergeOpRepoManager orm;
   private boolean closeOrm;
@@ -98,6 +115,7 @@ public class MergeSuperSet {
     this.changeDataFactory = changeDataFactory;
     this.queryProvider = queryProvider;
     this.repoManagerProvider = repoManagerProvider;
+    queryCache = new HashMap<>();
   }
 
   public MergeSuperSet setMergeOpRepoManager(MergeOpRepoManager orm) {
@@ -262,8 +280,8 @@ public class MergeSuperSet {
       Set<String> visibleHashes = walkChangesByHashes(visibleCommits,
           emptySet, or, head);
 
-      Iterable<ChangeData> cds = query()
-          .byCommitsOnBranchNotMerged(or.repo, db, b, visibleHashes);
+      List<ChangeData> cds =
+          byCommitsOnBranchNotMerged(or, db, user, b, visibleHashes);
       for (ChangeData chd : cds) {
         chd.changeControl(user);
         visibleChanges.add(chd);
@@ -272,7 +290,7 @@ public class MergeSuperSet {
       Set<String> nonVisibleHashes = walkChangesByHashes(nonVisibleCommits,
           visibleHashes, or, head);
       Iterables.addAll(nonVisibleChanges,
-          query().byCommitsOnBranchNotMerged(or.repo, db, b, nonVisibleHashes));
+          byCommitsOnBranchNotMerged(or, db, user, b, nonVisibleHashes));
     }
 
     return new ChangeSet(visibleChanges, nonVisibleChanges);
@@ -292,6 +310,29 @@ public class MergeSuperSet {
     }
   }
 
+  private List<ChangeData> byCommitsOnBranchNotMerged(OpenRepo or, ReviewDb db,
+      CurrentUser user, Branch.NameKey branch, Set<String> hashes)
+      throws OrmException, IOException {
+    if (hashes.isEmpty()) {
+      return ImmutableList.of();
+    }
+    QueryKey k = QueryKey.create(branch, hashes);
+    List<ChangeData> cached = queryCache.get(k);
+    if (cached != null) {
+      return cached;
+    }
+
+    List<ChangeData> result = new ArrayList<>();
+    Iterable<ChangeData> destChanges = query()
+        .byCommitsOnBranchNotMerged(or.repo, db, branch, hashes);
+    for (ChangeData chd : destChanges) {
+      chd.changeControl(user);
+      result.add(chd);
+    }
+    queryCache.put(k, result);
+    return result;
+  }
+
   /**
    * Completes {@code cs} with any additional changes from its topics
    * <p>

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

@@ -43,6 +43,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.CurrentUser;
@@ -703,10 +704,7 @@ public class ChangeData {
   public ChangeControl changeControl(CurrentUser user) throws OrmException {
     if (changeControl != null) {
       CurrentUser oldUser = user;
-      // TODO(dborowitz): This is a hack; general CurrentUser equality would be
-      // better.
-      if (user.isIdentifiedUser() && oldUser.isIdentifiedUser()
-          && user.getAccountId().equals(oldUser.getAccountId())) {
+      if (sameUser(user, oldUser)) {
         return changeControl;
       }
       throw new IllegalStateException(
@@ -725,6 +723,19 @@ public class ChangeData {
     return changeControl;
   }
 
+  private static boolean sameUser(CurrentUser a, CurrentUser b) {
+    // TODO(dborowitz): This is a hack; general CurrentUser equality would be
+    // better.
+    if (a.isInternalUser() && b.isInternalUser()) {
+      return true;
+    } else if (a instanceof AnonymousUser && b instanceof AnonymousUser) {
+      return true;
+    } else if (a.isIdentifiedUser() && b.isIdentifiedUser()) {
+      return a.getAccountId().equals(b.getAccountId());
+    }
+    return false;
+  }
+
   void cacheVisibleTo(ChangeControl ctl) {
     visibleTo = ctl.getUser();
     changeControl = ctl;