Merge "Load multiple groups more efficiently"

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,24 +269,43 @@ 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()) {
+          String currentUuid = uuidIterator.next();
+          String ref = RefNames.refsGroups(AccountGroup.uuid(currentUuid));
           Ref sha1 = allUsers.exactRef(ref);
           if (sha1 == null) {
-          return Optional.empty();
+            toReturn.put(currentUuid, Optional.empty());
+            continue;
           }
           Cache.GroupKeyProto key =
               Cache.GroupKeyProto.newBuilder()
-                .setUuid(uuid)
+                  .setUuid(currentUuid)
                   .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId()))
                   .build();
-        return Optional.of(persistedCache.get(key));
+          keyList.add(key);
         }
       }
+      persistedCache.getAll(keyList).entrySet().stream()
+          .forEach(g -> toReturn.put(g.getKey().getUuid(), Optional.of(g.getValue())));
+      return toReturn;
+    }
   }
 
   static class PersistedByUUIDLoader extends CacheLoader<Cache.GroupKeyProto, InternalGroup> {

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 =
+        loadGroups(
                 getAllExistingGroups()
                     .filter(group -> isRelevant(pattern, group))
-            .map(this::loadGroup)
+                    .map(g -> g.getUUID())
-            .flatMap(Streams::stream)
+                    .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 =
+        loadGroups(
                 groups
                     .getAllGroupReferences()
                     .filter(group -> isRelevant(pattern, group))
-            .map(this::loadGroup)
+                    .map(g -> g.getUUID())
-            .flatMap(Streams::stream)
+                    .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)
+    groupCache.get(subgroupMembersToLoad).values().stream()
         .map(InternalGroupDescription::new)
-                .map(
+        .forEach(
             subgroup -> {
               GroupControl subgroupControl = groupControlFactory.controlFor(subgroup);
-                      return getTransitiveMemberIds(subgroup, subgroupControl, seenGroups);
+              indirectMembers.addAll(getTransitiveMemberIds(subgroup, subgroupControl, seenGroups));
-                    })
+            });
-                .orElseGet(ImmutableSet::of);
+
-        indirectMembers.addAll(subgroupMembers);
-      }
-    }
     return indirectMembers;
   }