Load multiple groups more efficiently

This is more efficient to getAll and loadAll the groups just once
for all groups instead of once per group. This is true mostly because we
would only need to open the repository just once this way.

Places in the code that could utilize the new methods were updated.

Change-Id: I43601cc2c574ff195d93867c0571a33c1a76cd57

java/com/google/gerrit/server/account/GroupCache.java

@@ -16,6 +16,8 @@ package com.google.gerrit.server.account;
 
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.InternalGroup;
+import java.util.Collection;
+import java.util.Map;
 import java.util.Optional;
 
 /** Tracks group objects in memory for efficient access. */
@@ -47,6 +49,18 @@ public interface GroupCache {
    */
   Optional<InternalGroup> get(AccountGroup.UUID groupUuid);
 
+  /**
+   * Returns a {@code Map} of {@code AccountGroup.UUID} to {@code InternalGroup} for the given
+   * groups UUIDs. If not cached yet the groups are loaded. If a group can't be loaded (e.g. because
+   * it is missing), the entry will be missing from the result.
+   *
+   * @param groupUuids UUIDs of the groups that should be retrieved
+   * @return {@code Map} of {@code AccountGroup.UUID} to {@code InternalGroup} instances for the
+   *     given group UUIDs, if a group can't be loaded (e.g. because it is missing), the entry will
+   *     be missing from the result.
+   */
+  Map<AccountGroup.UUID, InternalGroup> get(Collection<AccountGroup.UUID> groupUuids);
+
   /**
    * Removes the association of the given ID with a group.
    *
@@ -88,4 +102,7 @@ public interface GroupCache {
    * @param groupUuid the UUID of a possibly associated group
    */
   void evict(AccountGroup.UUID groupUuid);
+
+  /** @see #evict(AccountGroup.UUID); */
+  void evict(Collection<AccountGroup.UUID> groupUuid);
 }

java/com/google/gerrit/server/account/GroupCacheImpl.java

@@ -14,8 +14,14 @@
 
 package com.google.gerrit.server.account;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.InternalGroup;
@@ -40,7 +46,14 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Named;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import org.bouncycastle.util.Strings;
 import org.eclipse.jgit.lib.ObjectId;
@@ -156,6 +169,20 @@ public class GroupCacheImpl implements GroupCache {
     }
   }
 
+  @Override
+  public Map<AccountGroup.UUID, InternalGroup> get(Collection<AccountGroup.UUID> groupUuids) {
+    try {
+      Set<String> groupUuidsStringSet =
+          groupUuids.stream().map(u -> u.get()).collect(toImmutableSet());
+      return byUUID.getAll(groupUuidsStringSet).entrySet().stream()
+          .filter(g -> g.getValue().isPresent())
+          .collect(toImmutableMap(g -> AccountGroup.uuid(g.getKey()), g -> g.getValue().get()));
+    } catch (ExecutionException e) {
+      logger.atWarning().withCause(e).log("Cannot look up groups %s by uuids", groupUuids);
+      return ImmutableMap.of();
+    }
+  }
+
   @Override
   public void evict(AccountGroup.Id groupId) {
     if (groupId != null) {
@@ -180,6 +207,14 @@ public class GroupCacheImpl implements GroupCache {
     }
   }
 
+  @Override
+  public void evict(Collection<AccountGroup.UUID> groupUuids) {
+    if (groupUuids != null && !groupUuids.isEmpty()) {
+      logger.atFine().log("Evict groups %s by UUID", groupUuids);
+      byUUID.invalidateAll(groupUuids);
+    }
+  }
+
   static class ByIdLoader extends CacheLoader<AccountGroup.Id, Optional<InternalGroup>> {
     private final Provider<InternalGroupQuery> groupQueryProvider;
 
@@ -234,23 +269,42 @@ public class GroupCacheImpl implements GroupCache {
 
     @Override
     public Optional<InternalGroup> load(String uuid) throws Exception {
+      return loadAll(ImmutableSet.of(uuid)).get(uuid);
+    }
+
+    @Override
+    public Map<String, Optional<InternalGroup>> loadAll(Iterable<? extends String> uuids)
+        throws Exception {
+      Map<String, Optional<InternalGroup>> toReturn = new HashMap<>();
+      if (Iterables.isEmpty(uuids)) {
+        return toReturn;
+      }
+      Iterator<? extends String> uuidIterator = uuids.iterator();
+      List<Cache.GroupKeyProto> keyList = new ArrayList<>();
       try (TraceTimer ignored =
               TraceContext.newTimer(
                   "Loading group from serialized cache",
-                  Metadata.builder().groupUuid(uuid).build());
+                  Metadata.builder().cacheName(BYUUID_NAME_PERSISTED).build());
           Repository allUsers = repoManager.openRepository(allUsersName)) {
-        String ref = RefNames.refsGroups(AccountGroup.uuid(uuid));
+        while (uuidIterator.hasNext()) {
-        Ref sha1 = allUsers.exactRef(ref);
+          String currentUuid = uuidIterator.next();
-        if (sha1 == null) {
+          String ref = RefNames.refsGroups(AccountGroup.uuid(currentUuid));
-          return Optional.empty();
+          Ref sha1 = allUsers.exactRef(ref);
+          if (sha1 == null) {
+            toReturn.put(currentUuid, Optional.empty());
+            continue;
+          }
+          Cache.GroupKeyProto key =
+              Cache.GroupKeyProto.newBuilder()
+                  .setUuid(currentUuid)
+                  .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId()))
+                  .build();
+          keyList.add(key);
         }
-        Cache.GroupKeyProto key =
-            Cache.GroupKeyProto.newBuilder()
-                .setUuid(uuid)
-                .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId()))
-                .build();
-        return Optional.of(persistedCache.get(key));
       }
+      persistedCache.getAll(keyList).entrySet().stream()
+          .forEach(g -> toReturn.put(g.getKey().getUuid(), Optional.of(g.getValue())));
+      return toReturn;
     }
   }
 

java/com/google/gerrit/server/account/IncludingGroupMembership.java

@@ -27,7 +27,6 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -83,6 +82,9 @@ public class IncludingGroupMembership implements GroupMembership {
     }
 
     if (tryExpanding) {
+      Set<AccountGroup.UUID> queryIdsSet = new HashSet<>();
+      queryIds.forEach(i -> queryIdsSet.add(i));
+      Map<AccountGroup.UUID, InternalGroup> groups = groupCache.get(queryIdsSet);
       for (AccountGroup.UUID id : queryIds) {
         if (memberOf.containsKey(id)) {
           // Membership was earlier proven to be false.
@@ -90,15 +92,15 @@ public class IncludingGroupMembership implements GroupMembership {
         }
 
         memberOf.put(id, false);
-        Optional<InternalGroup> group = groupCache.get(id);
+        InternalGroup group = groups.get(id);
-        if (!group.isPresent()) {
+        if (group == null) {
           continue;
         }
-        if (user.isIdentifiedUser() && group.get().getMembers().contains(user.getAccountId())) {
+        if (user.isIdentifiedUser() && group.getMembers().contains(user.getAccountId())) {
           memberOf.put(id, true);
           return true;
         }
-        if (search(group.get().getSubgroups())) {
+        if (search(group.getSubgroups())) {
           memberOf.put(id, true);
           return true;
         }

java/com/google/gerrit/server/index/group/AllGroupsIndexer.java

@@ -35,7 +35,7 @@ import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Optional;
+import java.util.Map;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -88,16 +88,17 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro
     AtomicInteger done = new AtomicInteger();
     AtomicInteger failed = new AtomicInteger();
     Stopwatch sw = Stopwatch.createStarted();
+    groupCache.evict(uuids);
+    Map<AccountGroup.UUID, InternalGroup> reindexedGroups = groupCache.get(uuids);
     for (AccountGroup.UUID uuid : uuids) {
       String desc = "group " + uuid;
       ListenableFuture<?> future =
           executor.submit(
               () -> {
                 try {
-                  groupCache.evict(uuid);
+                  InternalGroup internalGroup = reindexedGroups.get(uuid);
-                  Optional<InternalGroup> internalGroup = groupCache.get(uuid);
+                  if (internalGroup != null) {
-                  if (internalGroup.isPresent()) {
+                    index.replace(internalGroup);
-                    index.replace(internalGroup.get());
                   } else {
                     index.delete(uuid);
 

java/com/google/gerrit/server/restapi/group/GetAuditLog.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.restapi.group;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static java.util.Comparator.comparing;
 
 import com.google.gerrit.entities.AccountGroup;
@@ -42,7 +43,7 @@ import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Optional;
+import java.util.Map;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Repository;
 
@@ -105,14 +106,17 @@ public class GetAuditLog implements RestReadView<GroupResource> {
                   member));
         }
       }
-
+      List<AccountGroupByIdAudit> subGroupsAudit =
-      for (AccountGroupByIdAudit auditEvent :
+          groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID());
-          groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID())) {
+      Map<AccountGroup.UUID, InternalGroup> groups =
+          groupCache.get(
+              subGroupsAudit.stream().map(a -> a.includeUuid()).collect(toImmutableList()));
+      for (AccountGroupByIdAudit auditEvent : subGroupsAudit) {
         AccountGroup.UUID includedGroupUUID = auditEvent.includeUuid();
-        Optional<InternalGroup> includedGroup = groupCache.get(includedGroupUUID);
+        InternalGroup includedGroup = groups.get(includedGroupUUID);
         GroupInfo member;
-        if (includedGroup.isPresent()) {
+        if (includedGroup != null) {
-          member = groupJson.format(new InternalGroupDescription(includedGroup.get()));
+          member = groupJson.format(new InternalGroupDescription(includedGroup));
         } else {
           member = new GroupInfo();
           member.id = Url.encode(includedGroupUUID.get());

java/com/google/gerrit/server/restapi/group/ListGroups.java

@@ -15,12 +15,12 @@
 package com.google.gerrit.server.restapi.group;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Streams;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.GroupDescription;
@@ -57,7 +57,6 @@ import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
-import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
@@ -273,10 +272,12 @@ public class ListGroups implements RestReadView<TopLevelResource> {
       throws IOException, ConfigInvalidException, PermissionBackendException {
     Pattern pattern = getRegexPattern();
     Stream<GroupDescription.Internal> existingGroups =
-        getAllExistingGroups()
+        loadGroups(
-            .filter(group -> isRelevant(pattern, group))
+                getAllExistingGroups()
-            .map(this::loadGroup)
+                    .filter(group -> isRelevant(pattern, group))
-            .flatMap(Streams::stream)
+                    .map(g -> g.getUUID())
+                    .collect(toImmutableSet()))
+            .stream()
             .filter(this::isVisible)
             .sorted(GROUP_COMPARATOR)
             .skip(start);
@@ -359,11 +360,13 @@ public class ListGroups implements RestReadView<TopLevelResource> {
       throws IOException, ConfigInvalidException, PermissionBackendException {
     Pattern pattern = getRegexPattern();
     Stream<? extends GroupDescription.Internal> foundGroups =
-        groups
+        loadGroups(
-            .getAllGroupReferences()
+                groups
-            .filter(group -> isRelevant(pattern, group))
+                    .getAllGroupReferences()
-            .map(this::loadGroup)
+                    .filter(group -> isRelevant(pattern, group))
-            .flatMap(Streams::stream)
+                    .map(g -> g.getUUID())
+                    .collect(toImmutableSet()))
+            .stream()
             .filter(this::isVisible)
             .filter(filter)
             .sorted(GROUP_COMPARATOR)
@@ -379,8 +382,10 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return groupInfos;
   }
 
-  private Optional<GroupDescription.Internal> loadGroup(GroupReference groupReference) {
+  private Set<GroupDescription.Internal> loadGroups(Collection<AccountGroup.UUID> groupUuids) {
-    return groupCache.get(groupReference.getUUID()).map(InternalGroupDescription::new);
+    return groupCache.get(groupUuids).values().stream()
+        .map(InternalGroupDescription::new)
+        .collect(toImmutableSet());
   }
 
   private List<GroupInfo> getGroupsOwnedBy(String id)

java/com/google/gerrit/server/restapi/group/ListMembers.java

@@ -144,23 +144,21 @@ public class ListMembers implements RestReadView<GroupResource> {
   private Set<Account.Id> getIndirectMemberIds(
       GroupDescription.Internal group, HashSet<AccountGroup.UUID> seenGroups) {
     Set<Account.Id> indirectMembers = new HashSet<>();
+    Set<AccountGroup.UUID> subgroupMembersToLoad = new HashSet<>();
     for (AccountGroup.UUID subgroupUuid : group.getSubgroups()) {
       if (!seenGroups.contains(subgroupUuid)) {
         seenGroups.add(subgroupUuid);
-
+        subgroupMembersToLoad.add(subgroupUuid);
-        Set<Account.Id> subgroupMembers =
-            groupCache
-                .get(subgroupUuid)
-                .map(InternalGroupDescription::new)
-                .map(
-                    subgroup -> {
-                      GroupControl subgroupControl = groupControlFactory.controlFor(subgroup);
-                      return getTransitiveMemberIds(subgroup, subgroupControl, seenGroups);
-                    })
-                .orElseGet(ImmutableSet::of);
-        indirectMembers.addAll(subgroupMembers);
       }
     }
+    groupCache.get(subgroupMembersToLoad).values().stream()
+        .map(InternalGroupDescription::new)
+        .forEach(
+            subgroup -> {
+              GroupControl subgroupControl = groupControlFactory.controlFor(subgroup);
+              indirectMembers.addAll(getTransitiveMemberIds(subgroup, subgroupControl, seenGroups));
+            });
+
     return indirectMembers;
   }