From f9afb09ef028282c9cf919fd84b13a359be8a6b2 Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Wed, 17 Mar 2021 15:41:18 +0100 Subject: [PATCH] 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 --- .../gerrit/server/account/GroupCache.java | 17 +++++ .../gerrit/server/account/GroupCacheImpl.java | 76 ++++++++++++++++--- .../account/IncludingGroupMembership.java | 12 +-- .../server/index/group/AllGroupsIndexer.java | 11 +-- .../server/restapi/group/GetAuditLog.java | 18 +++-- .../server/restapi/group/ListGroups.java | 31 ++++---- .../server/restapi/group/ListMembers.java | 22 +++--- 7 files changed, 134 insertions(+), 53 deletions(-) diff --git a/java/com/google/gerrit/server/account/GroupCache.java b/java/com/google/gerrit/server/account/GroupCache.java index 224ac1465e..aaae95a51d 100644 --- a/java/com/google/gerrit/server/account/GroupCache.java +++ b/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 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 get(Collection 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 groupUuid); } diff --git a/java/com/google/gerrit/server/account/GroupCacheImpl.java b/java/com/google/gerrit/server/account/GroupCacheImpl.java index 6f06dd3b91..eaec9baecd 100644 --- a/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/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 get(Collection groupUuids) { + try { + Set 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 groupUuids) { + if (groupUuids != null && !groupUuids.isEmpty()) { + logger.atFine().log("Evict groups %s by UUID", groupUuids); + byUUID.invalidateAll(groupUuids); + } + } + static class ByIdLoader extends CacheLoader> { private final Provider groupQueryProvider; @@ -234,23 +269,42 @@ public class GroupCacheImpl implements GroupCache { @Override public Optional load(String uuid) throws Exception { + return loadAll(ImmutableSet.of(uuid)).get(uuid); + } + + @Override + public Map> loadAll(Iterable uuids) + throws Exception { + Map> toReturn = new HashMap<>(); + if (Iterables.isEmpty(uuids)) { + return toReturn; + } + Iterator uuidIterator = uuids.iterator(); + List 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)); - Ref sha1 = allUsers.exactRef(ref); - if (sha1 == null) { - return Optional.empty(); + while (uuidIterator.hasNext()) { + String currentUuid = uuidIterator.next(); + String ref = RefNames.refsGroups(AccountGroup.uuid(currentUuid)); + 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; } } diff --git a/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/java/com/google/gerrit/server/account/IncludingGroupMembership.java index a277fac347..8cec8bf1c9 100644 --- a/java/com/google/gerrit/server/account/IncludingGroupMembership.java +++ b/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 queryIdsSet = new HashSet<>(); + queryIds.forEach(i -> queryIdsSet.add(i)); + Map 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 group = groupCache.get(id); - if (!group.isPresent()) { + InternalGroup group = groups.get(id); + 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; } diff --git a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java index 3edd26ba40..b3ef679675 100644 --- a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java +++ b/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 reindexedGroups = groupCache.get(uuids); for (AccountGroup.UUID uuid : uuids) { String desc = "group " + uuid; ListenableFuture future = executor.submit( () -> { try { - groupCache.evict(uuid); - Optional internalGroup = groupCache.get(uuid); - if (internalGroup.isPresent()) { - index.replace(internalGroup.get()); + InternalGroup internalGroup = reindexedGroups.get(uuid); + if (internalGroup != null) { + index.replace(internalGroup); } else { index.delete(uuid); diff --git a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java index 706a1f4272..e3aa0f3697 100644 --- a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java +++ b/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 { member)); } } - - for (AccountGroupByIdAudit auditEvent : - groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID())) { + List subGroupsAudit = + groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID()); + Map groups = + groupCache.get( + subGroupsAudit.stream().map(a -> a.includeUuid()).collect(toImmutableList())); + for (AccountGroupByIdAudit auditEvent : subGroupsAudit) { AccountGroup.UUID includedGroupUUID = auditEvent.includeUuid(); - Optional includedGroup = groupCache.get(includedGroupUUID); + InternalGroup includedGroup = groups.get(includedGroupUUID); GroupInfo member; - if (includedGroup.isPresent()) { - member = groupJson.format(new InternalGroupDescription(includedGroup.get())); + if (includedGroup != null) { + member = groupJson.format(new InternalGroupDescription(includedGroup)); } else { member = new GroupInfo(); member.id = Url.encode(includedGroupUUID.get()); diff --git a/java/com/google/gerrit/server/restapi/group/ListGroups.java b/java/com/google/gerrit/server/restapi/group/ListGroups.java index 3e2a577eae..96402be88a 100644 --- a/java/com/google/gerrit/server/restapi/group/ListGroups.java +++ b/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 { throws IOException, ConfigInvalidException, PermissionBackendException { Pattern pattern = getRegexPattern(); Stream existingGroups = - getAllExistingGroups() - .filter(group -> isRelevant(pattern, group)) - .map(this::loadGroup) - .flatMap(Streams::stream) + loadGroups( + getAllExistingGroups() + .filter(group -> isRelevant(pattern, group)) + .map(g -> g.getUUID()) + .collect(toImmutableSet())) + .stream() .filter(this::isVisible) .sorted(GROUP_COMPARATOR) .skip(start); @@ -359,11 +360,13 @@ public class ListGroups implements RestReadView { throws IOException, ConfigInvalidException, PermissionBackendException { Pattern pattern = getRegexPattern(); Stream foundGroups = - groups - .getAllGroupReferences() - .filter(group -> isRelevant(pattern, group)) - .map(this::loadGroup) - .flatMap(Streams::stream) + loadGroups( + groups + .getAllGroupReferences() + .filter(group -> isRelevant(pattern, group)) + .map(g -> g.getUUID()) + .collect(toImmutableSet())) + .stream() .filter(this::isVisible) .filter(filter) .sorted(GROUP_COMPARATOR) @@ -379,8 +382,10 @@ public class ListGroups implements RestReadView { return groupInfos; } - private Optional loadGroup(GroupReference groupReference) { - return groupCache.get(groupReference.getUUID()).map(InternalGroupDescription::new); + private Set loadGroups(Collection groupUuids) { + return groupCache.get(groupUuids).values().stream() + .map(InternalGroupDescription::new) + .collect(toImmutableSet()); } private List getGroupsOwnedBy(String id) diff --git a/java/com/google/gerrit/server/restapi/group/ListMembers.java b/java/com/google/gerrit/server/restapi/group/ListMembers.java index cba6408dea..218eb98056 100644 --- a/java/com/google/gerrit/server/restapi/group/ListMembers.java +++ b/java/com/google/gerrit/server/restapi/group/ListMembers.java @@ -144,23 +144,21 @@ public class ListMembers implements RestReadView { private Set getIndirectMemberIds( GroupDescription.Internal group, HashSet seenGroups) { Set indirectMembers = new HashSet<>(); + Set subgroupMembersToLoad = new HashSet<>(); for (AccountGroup.UUID subgroupUuid : group.getSubgroups()) { if (!seenGroups.contains(subgroupUuid)) { seenGroups.add(subgroupUuid); - - Set 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); + subgroupMembersToLoad.add(subgroupUuid); } } + groupCache.get(subgroupMembersToLoad).values().stream() + .map(InternalGroupDescription::new) + .forEach( + subgroup -> { + GroupControl subgroupControl = groupControlFactory.controlFor(subgroup); + indirectMembers.addAll(getTransitiveMemberIds(subgroup, subgroupControl, seenGroups)); + }); + return indirectMembers; }