Persist group cache by uuid
We split the groups by uuid cache into two caches:
1. "groups_byuuid" stays the same. Caches by uuid without reading the
SHA-1 of the ref from NoteDb.
2. "groups_byuuid_persisted" is the persistent variant of the above
cache. This is used in the loader of "groups_byuuid" and it has the
SHA-1 of the ref in the key.
The new setup eases cold start times. The split is useful to allow
accessing groups without loading the SHA-1 for each lookup.
As a follow-up, we can also implement another method that gets multiple
groups at the same time. This will be more efficient given that we will
only need to access NoteDb once instead of once per group. This will be
needed when getting the subgroups.
While at it, we needed to add a new method that loads a group (to load
it for a specific revision) so we also add a few @Nullable for other
methods (since the sha-1 can be null).
Change-Id: Ia52b3a3edcae0589c785e9d093776ec26dc324ca
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 4ce5360..8793458 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -815,6 +815,7 @@
* `"groups"`: default is unlimited
* `"groups_byname"`: default is unlimited
* `"groups_byuuid"`: default is unlimited
+* `"groups_byuuid_persisted"`: default is `1g` (1 GiB of disk space)
* `"plugin_resources"`: default is 2m (2 MiB of memory)
+
@@ -1038,6 +1039,17 @@
External group membership obtained from LDAP is cached under
`"ldap_groups"`.
+cache `"groups_byuuid_persisted"`::
++
+Caches the basic group information of internal groups by group UUID,
+including the group owner, name, and description.
++
+This is the persisted version of `groups_byuuid` cache. The intention of this
+cache is to have an in-memory size of 0.
++
+External group membership obtained from LDAP is cached under
+`"ldap_groups"`.
+
cache `"groups_bymember"`::
+
Caches the groups which contain a specific member (account). If direct
diff --git a/java/com/google/gerrit/server/account/GroupCacheImpl.java b/java/com/google/gerrit/server/account/GroupCacheImpl.java
index 1ea4282..6f06dd3 100644
--- a/java/com/google/gerrit/server/account/GroupCacheImpl.java
+++ b/java/com/google/gerrit/server/account/GroupCacheImpl.java
@@ -19,7 +19,16 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.proto.Cache;
+import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
+import com.google.gerrit.server.cache.serialize.ProtobufSerializer;
+import com.google.gerrit.server.cache.serialize.entities.InternalGroupSerializer;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.group.db.Groups;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -33,6 +42,10 @@
import com.google.inject.name.Named;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
+import org.bouncycastle.util.Strings;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
/** Tracks group objects in memory for efficient access. */
@Singleton
@@ -42,6 +55,7 @@
private static final String BYID_NAME = "groups";
private static final String BYNAME_NAME = "groups_byname";
private static final String BYUUID_NAME = "groups_byuuid";
+ private static final String BYUUID_NAME_PERSISTED = "groups_byuuid_persisted";
public static Module module() {
return new CacheModule() {
@@ -55,9 +69,35 @@
.maximumWeight(Long.MAX_VALUE)
.loader(ByNameLoader.class);
+ // We split the group cache into two parts for performance reasons:
+ // 1) An in-memory part that has only the group ref uuid as key.
+ // 2) A persisted part that has the group ref uuid and sha1 of the ref as key.
+ //
+ // When loading dashboards or returning change query results we potentially
+ // need to access many groups.
+ // We want the persisted cache to be immutable and we want it to be impossible that a
+ // value for a given key is out of date. We therefore require the sha-1 in the key. That
+ // is in line with the rest of the caches in Gerrit.
+ //
+ // Splitting the cache into two chunks internally in this class allows us to retain
+ // the existing performance guarantees of not requiring reads for the repo for values
+ // cached in-memory but also to persist the cache which leads to a much improved
+ // cold-start behavior and in-memory miss latency.
+
cache(BYUUID_NAME, String.class, new TypeLiteral<Optional<InternalGroup>>() {})
.maximumWeight(Long.MAX_VALUE)
- .loader(ByUUIDLoader.class);
+ .loader(ByUUIDInMemoryLoader.class);
+
+ persist(
+ BYUUID_NAME_PERSISTED,
+ Cache.GroupKeyProto.class,
+ new TypeLiteral<InternalGroup>() {})
+ .loader(PersistedByUUIDLoader.class)
+ .keySerializer(new ProtobufSerializer<>(Cache.GroupKeyProto.parser()))
+ .valueSerializer(PersistedInternalGroupSerializer.INSTANCE)
+ .diskLimit(1 << 30) // 1 GiB
+ .version(1)
+ .maximumWeight(0);
bind(GroupCacheImpl.class);
bind(GroupCache.class).to(GroupCacheImpl.class);
@@ -150,7 +190,7 @@
@Override
public Optional<InternalGroup> load(AccountGroup.Id key) throws Exception {
- try (TraceTimer timer =
+ try (TraceTimer ignored =
TraceContext.newTimer(
"Loading group by ID", Metadata.builder().groupId(key.get()).build())) {
return groupQueryProvider.get().byId(key);
@@ -168,7 +208,7 @@
@Override
public Optional<InternalGroup> load(String name) throws Exception {
- try (TraceTimer timer =
+ try (TraceTimer ignored =
TraceContext.newTimer(
"Loading group by name", Metadata.builder().groupName(name).build())) {
return groupQueryProvider.get().byName(AccountGroup.nameKey(name));
@@ -176,21 +216,89 @@
}
}
- static class ByUUIDLoader extends CacheLoader<String, Optional<InternalGroup>> {
- private final Groups groups;
+ static class ByUUIDInMemoryLoader extends CacheLoader<String, Optional<InternalGroup>> {
+ private final LoadingCache<Cache.GroupKeyProto, InternalGroup> persistedCache;
+ private final GitRepositoryManager repoManager;
+ private final AllUsersName allUsersName;
@Inject
- ByUUIDLoader(Groups groups) {
- this.groups = groups;
+ ByUUIDInMemoryLoader(
+ @Named(BYUUID_NAME_PERSISTED)
+ LoadingCache<Cache.GroupKeyProto, InternalGroup> persistedCache,
+ GitRepositoryManager repoManager,
+ AllUsersName allUsersName) {
+ this.persistedCache = persistedCache;
+ this.repoManager = repoManager;
+ this.allUsersName = allUsersName;
}
@Override
public Optional<InternalGroup> load(String uuid) throws Exception {
- try (TraceTimer timer =
- TraceContext.newTimer(
- "Loading group by UUID", Metadata.builder().groupUuid(uuid).build())) {
- return groups.getGroup(AccountGroup.uuid(uuid));
+ try (TraceTimer ignored =
+ TraceContext.newTimer(
+ "Loading group from serialized cache",
+ Metadata.builder().groupUuid(uuid).build());
+ Repository allUsers = repoManager.openRepository(allUsersName)) {
+ String ref = RefNames.refsGroups(AccountGroup.uuid(uuid));
+ Ref sha1 = allUsers.exactRef(ref);
+ if (sha1 == null) {
+ return Optional.empty();
+ }
+ Cache.GroupKeyProto key =
+ Cache.GroupKeyProto.newBuilder()
+ .setUuid(uuid)
+ .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId()))
+ .build();
+ return Optional.of(persistedCache.get(key));
}
}
}
+
+ static class PersistedByUUIDLoader extends CacheLoader<Cache.GroupKeyProto, InternalGroup> {
+ private final Groups groups;
+
+ @Inject
+ PersistedByUUIDLoader(Groups groups) {
+ this.groups = groups;
+ }
+
+ @Override
+ public InternalGroup load(Cache.GroupKeyProto key) throws Exception {
+ try (TraceTimer ignored =
+ TraceContext.newTimer(
+ "Loading group by UUID", Metadata.builder().groupUuid(key.getUuid()).build())) {
+ ObjectId sha1 = ObjectIdConverter.create().fromByteString(key.getRevision());
+ Optional<InternalGroup> loadedGroup =
+ groups.getGroup(AccountGroup.uuid(key.getUuid()), sha1);
+ if (!loadedGroup.isPresent()) {
+ throw new IllegalStateException(
+ String.format(
+ "group %s should have the sha-1 %s, but " + "it was not found",
+ key.getUuid(), sha1.getName()));
+ }
+ return loadedGroup.get();
+ }
+ }
+ }
+
+ private enum PersistedInternalGroupSerializer implements CacheSerializer<InternalGroup> {
+ INSTANCE;
+
+ @Override
+ public byte[] serialize(InternalGroup value) {
+ if (value == null) {
+ return new byte[0];
+ }
+ return Protos.toByteArray(InternalGroupSerializer.serialize(value));
+ }
+
+ @Override
+ public InternalGroup deserialize(byte[] in) {
+ if (Strings.fromByteArray(in).isEmpty()) {
+ return null;
+ }
+ return InternalGroupSerializer.deserialize(
+ Protos.parseUnchecked(Cache.InternalGroupProto.parser(), in));
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializer.java b/java/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializer.java
new file mode 100644
index 0000000..7449917
--- /dev/null
+++ b/java/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializer.java
@@ -0,0 +1,82 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.cache.serialize.entities;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.InternalGroup;
+import com.google.gerrit.server.cache.proto.Cache;
+import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
+import java.sql.Timestamp;
+
+/** Helper to (de)serialize values for caches. */
+public class InternalGroupSerializer {
+ public static InternalGroup deserialize(Cache.InternalGroupProto proto) {
+ InternalGroup.Builder builder =
+ InternalGroup.builder()
+ .setId(AccountGroup.id(proto.getId()))
+ .setNameKey(AccountGroup.nameKey(proto.getName()))
+ .setOwnerGroupUUID(AccountGroup.uuid(proto.getOwnerGroupUuid()))
+ .setVisibleToAll(proto.getIsVisibleToAll())
+ .setGroupUUID(AccountGroup.uuid(proto.getGroupUuid()))
+ .setCreatedOn(new Timestamp(proto.getCreatedOn()))
+ .setMembers(
+ proto.getMembersIdsList().stream()
+ .map(a -> Account.id(a))
+ .collect(toImmutableSet()))
+ .setSubgroups(
+ proto.getSubgroupUuidsList().stream()
+ .map(s -> AccountGroup.uuid(s))
+ .collect(toImmutableSet()));
+
+ if (!proto.getDescription().isEmpty()) {
+ builder.setDescription(proto.getDescription());
+ }
+
+ if (!proto.getRefState().isEmpty()) {
+ builder.setRefState(ObjectIdConverter.create().fromByteString(proto.getRefState()));
+ }
+
+ return builder.build();
+ }
+
+ public static Cache.InternalGroupProto serialize(InternalGroup autoValue) {
+ Cache.InternalGroupProto.Builder builder =
+ Cache.InternalGroupProto.newBuilder()
+ .setId(autoValue.getId().get())
+ .setName(autoValue.getName())
+ .setOwnerGroupUuid(autoValue.getOwnerGroupUUID().get())
+ .setIsVisibleToAll(autoValue.isVisibleToAll())
+ .setGroupUuid(autoValue.getGroupUUID().get())
+ .setCreatedOn(autoValue.getCreatedOn().getTime());
+
+ autoValue.getMembers().stream().forEach(m -> builder.addMembersIds(m.get()));
+ autoValue.getSubgroups().stream().forEach(s -> builder.addSubgroupUuids(s.get()));
+
+ if (autoValue.getDescription() != null) {
+ builder.setDescription(autoValue.getDescription());
+ }
+
+ if (autoValue.getRefState() != null) {
+ builder.setRefState(ObjectIdConverter.create().toByteString(autoValue.getRefState()));
+ }
+
+ return builder.build();
+ }
+
+ private InternalGroupSerializer() {}
+}
diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java
index a5f5cfc..b2d1849b 100644
--- a/java/com/google/gerrit/server/group/db/GroupConfig.java
+++ b/java/com/google/gerrit/server/group/db/GroupConfig.java
@@ -24,6 +24,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
@@ -156,7 +157,7 @@
Project.NameKey projectName,
Repository repository,
AccountGroup.UUID groupUuid,
- ObjectId groupRefObjectId)
+ @Nullable ObjectId groupRefObjectId)
throws IOException, ConfigInvalidException {
GroupConfig groupConfig = new GroupConfig(groupUuid);
if (groupRefObjectId == null) {
diff --git a/java/com/google/gerrit/server/group/db/Groups.java b/java/com/google/gerrit/server/group/db/Groups.java
index 50c339e..30e37cb 100644
--- a/java/com/google/gerrit/server/group/db/Groups.java
+++ b/java/com/google/gerrit/server/group/db/Groups.java
@@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.AccountGroupByIdAudit;
import com.google.gerrit.entities.AccountGroupMemberAudit;
@@ -80,6 +81,23 @@
}
/**
+ * Returns the {@code InternalGroup} for the specified UUID and groupRefObjectId
+ *
+ * @param groupUuid the UUID of the group
+ * @param groupRefObjectId the ref revision of this group
+ * @return the found {@code InternalGroup} if it exists, or else an empty {@code Optional}
+ * @throws IOException if the group couldn't be retrieved from NoteDb
+ * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb
+ */
+ public Optional<InternalGroup> getGroup(
+ AccountGroup.UUID groupUuid, @Nullable ObjectId groupRefObjectId)
+ throws IOException, ConfigInvalidException {
+ try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
+ return getGroupFromNoteDb(allUsersName, allUsersRepo, groupUuid, groupRefObjectId);
+ }
+ }
+
+ /**
* Loads an internal group from NoteDb using the group UUID. This method returns the latest state
* of the internal group.
*/
@@ -97,7 +115,7 @@
AllUsersName allUsersName,
Repository allUsersRepository,
AccountGroup.UUID uuid,
- ObjectId groupRefObjectId)
+ @Nullable ObjectId groupRefObjectId)
throws IOException, ConfigInvalidException {
GroupConfig groupConfig =
GroupConfig.loadForGroup(allUsersName, allUsersRepository, uuid, groupRefObjectId);
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializerTest.java
new file mode 100644
index 0000000..8d301e4
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/InternalGroupSerializerTest.java
@@ -0,0 +1,61 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.cache.serialize.entities;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.cache.serialize.entities.InternalGroupSerializer.deserialize;
+import static com.google.gerrit.server.cache.serialize.entities.InternalGroupSerializer.serialize;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.InternalGroup;
+import com.google.gerrit.server.util.time.TimeUtil;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Test;
+
+public class InternalGroupSerializerTest {
+ static final InternalGroup MINIMAL_VALUES_SET =
+ InternalGroup.builder()
+ .setId(AccountGroup.id(123456))
+ .setNameKey(AccountGroup.nameKey("group name"))
+ .setOwnerGroupUUID(AccountGroup.uuid("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
+ .setVisibleToAll(false)
+ .setGroupUUID(AccountGroup.uuid("deadbeefdeadbeefdeadbeefdeadbeef12345678"))
+ .setCreatedOn(TimeUtil.nowTs())
+ .setMembers(ImmutableSet.of(Account.id(123), Account.id(321)))
+ .setSubgroups(
+ ImmutableSet.of(
+ AccountGroup.uuid("87654321deadbeefdeadbeefdeadbeefdeadbeef"),
+ AccountGroup.uuid("deadbeefdeadbeefdeadbeefdeadbeef87654321")))
+ .build();
+
+ static final InternalGroup ALL_VALUES_SET =
+ MINIMAL_VALUES_SET
+ .toBuilder()
+ .setDescription("description")
+ .setRefState(ObjectId.fromString("12345678deadbeefdeadbeefdeadbeefdeadbeef"))
+ .build();
+
+ @Test
+ public void roundTrip() {
+ assertThat(deserialize(serialize(ALL_VALUES_SET))).isEqualTo(ALL_VALUES_SET);
+ }
+
+ @Test
+ public void roundTripWithMinimalValues() {
+ assertThat(deserialize(serialize(MINIMAL_VALUES_SET))).isEqualTo(MINIMAL_VALUES_SET);
+ }
+}
diff --git a/proto/cache.proto b/proto/cache.proto
index 874e60a..292a225 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -286,6 +286,29 @@
repeated ExternalGroupProto external_group = 1;
}
+// Serialized key for com.google.gerrit.server.account.GroupCacheImpl.
+// Next ID: 3
+message GroupKeyProto {
+ string uuid = 1;
+ bytes revision = 2;
+}
+
+
+// Serialized form of com.google.gerrit.entities.InternalGroup.
+// Next ID: 11
+message InternalGroupProto {
+ int32 id = 1;
+ string name = 2;
+ string description = 3;
+ string owner_group_uuid = 4;
+ bool is_visible_to_all = 5;
+ string group_uuid = 6;
+ int64 created_on = 7;
+ repeated int32 members_ids = 8;
+ repeated string subgroup_uuids = 9;
+ bytes ref_state = 10;
+}
+
// Key for com.google.gerrit.server.git.PureRevertCache.
// Next ID: 4
message PureRevertKeyProto {
