Don't limit PerThreadCache rather limit PerThreadProjectCache
Before this change, PerThreadCache has an enforcing limit of 25. The
main reason for having a limit was to restrict the number of projects
cached per thread to avoid OOM exceptions (Ic2f6e11719). By enforcing
a limit on the whole PerThreadCache for just projects use case is
effecting other use cases (such as PerThreadRefByNameCache in
I7de4e2183) of PerThreadCache.
Having a limit for number of projects stored in PerThreadCache is
needed. But as PerThreadCache is designed to be generic, that limit
must be applied to just projects use case, not the whole PerThreadCache.
Introduce PerThreadProjectCache which enforces the limit instead of
PerThreadCache. Update tests accordingly.
PerThreadCache is needed in creating a per-request ref cache using
a snapshot of RefDirectory in I174fbfdb95. This change makes it
possible to create a per-request ref cache.
Also, code-owners plugin uses PerThreadCache in a couple of places.
As one of the usage is projects specific, update code-owners plugin
to use PerThreadProjectCache post this change.
Release-Notes: skip
Change-Id: I1090446b6555296c64da1c01db39b7ac3dc6f2a3
diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
index ef00b80..8ae9710 100644
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ b/java/com/google/gerrit/server/cache/PerThreadCache.java
@@ -43,19 +43,9 @@
* <p>Lastly, this class offers a cache, that requires callers to also provide a {@code Supplier} in
* case the object is not present in the cache, while {@code CurrentUser} provides a storage where
* just retrieving stored values is a valid operation.
- *
- * <p>To prevent OOM errors on requests that would cache a lot of objects, this class enforces an
- * internal limit after which no new elements are cached. All {@code get} calls are served by
- * invoking the {@code Supplier} after that.
*/
public class PerThreadCache implements AutoCloseable {
private static final ThreadLocal<PerThreadCache> CACHE = new ThreadLocal<>();
- /**
- * Cache at maximum 25 values per thread. This value was chosen arbitrarily. Some endpoints (like
- * ListProjects) break the assumption that the data cached in a request is limited. To prevent
- * this class from accumulating an unbound number of objects, we enforce this limit.
- */
- private static final int PER_THREAD_CACHE_SIZE = 25;
/**
* Unique key for key-value mappings stored in PerThreadCache. The key is based on the value's
@@ -119,7 +109,7 @@
return cache != null ? cache.get(key, loader) : loader.get();
}
- private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
+ private final Map<Key<?>, Object> cache = Maps.newHashMap();
private PerThreadCache() {}
@@ -132,9 +122,7 @@
T value = (T) cache.get(key);
if (value == null) {
value = loader.get();
- if (cache.size() < PER_THREAD_CACHE_SIZE) {
- cache.put(key, value);
- }
+ cache.put(key, value);
}
return value;
}
diff --git a/java/com/google/gerrit/server/cache/PerThreadProjectCache.java b/java/com/google/gerrit/server/cache/PerThreadProjectCache.java
new file mode 100644
index 0000000..86f1d2d
--- /dev/null
+++ b/java/com/google/gerrit/server/cache/PerThreadProjectCache.java
@@ -0,0 +1,64 @@
+// Copyright (C) 2023 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;
+
+import com.google.common.collect.Maps;
+import com.google.gerrit.entities.Project;
+import java.util.Map;
+import java.util.function.Supplier;
+
+/**
+ * To prevent OOM errors on requests that would cache a lot of objects, this class enforces an
+ * internal limit after which no new elements are cached. All {@code computeIfAbsentWithinLimit}
+ * calls are served by invoking the {@code Supplier} after that.
+ */
+public class PerThreadProjectCache {
+ private static final PerThreadCache.Key<PerThreadProjectCache> PER_THREAD_PROJECT_CACHE_KEY =
+ PerThreadCache.Key.create(PerThreadProjectCache.class);
+ /**
+ * Cache at maximum 25 values per thread. This value was chosen arbitrarily. Some endpoints (like
+ * ListProjects) break the assumption that the data cached in a request is limited. To prevent
+ * this class from accumulating an unbound number of objects, we enforce this limit.
+ */
+ private static final int PER_THREAD_PROJECT_CACHE_SIZE = 25;
+
+ private final Map<PerThreadCache.Key<Project.NameKey>, Object> valueByNameKey =
+ Maps.newHashMapWithExpectedSize(PER_THREAD_PROJECT_CACHE_SIZE);
+
+ private PerThreadProjectCache() {}
+
+ public static <T> T getOrCompute(PerThreadCache.Key<Project.NameKey> key, Supplier<T> loader) {
+ PerThreadCache perThreadCache = PerThreadCache.get();
+ if (perThreadCache != null) {
+ PerThreadProjectCache perThreadProjectCache =
+ perThreadCache.get(PER_THREAD_PROJECT_CACHE_KEY, PerThreadProjectCache::new);
+ return perThreadProjectCache.computeIfAbsentWithinLimit(key, loader);
+ }
+ return loader.get();
+ }
+
+ protected <T> T computeIfAbsentWithinLimit(
+ PerThreadCache.Key<Project.NameKey> key, Supplier<T> loader) {
+ @SuppressWarnings("unchecked")
+ T value = (T) valueByNameKey.get(key);
+ if (value == null) {
+ value = loader.get();
+ if (valueByNameKey.size() < PER_THREAD_PROJECT_CACHE_SIZE) {
+ valueByNameKey.put(key, value);
+ }
+ }
+ return value;
+ }
+}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index aa49852..bf4d05a 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -35,6 +35,7 @@
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.account.CapabilityCollection;
import com.google.gerrit.server.cache.PerThreadCache;
+import com.google.gerrit.server.cache.PerThreadProjectCache;
import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -124,8 +125,8 @@
public ForProject project(Project.NameKey project) {
try {
ProjectControl control =
- PerThreadCache.getOrCompute(
- PerThreadCache.Key.create(ProjectControl.class, project, user.getCacheKey()),
+ PerThreadProjectCache.getOrCompute(
+ PerThreadCache.Key.create(Project.NameKey.class, project, user.getCacheKey()),
() ->
projectControlFactory.create(
user, projectCache.get(project).orElseThrow(illegalState(project))));
diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD
index c708e09..cbe64a5 100644
--- a/javatests/com/google/gerrit/server/cache/BUILD
+++ b/javatests/com/google/gerrit/server/cache/BUILD
@@ -5,6 +5,7 @@
name = "tests",
srcs = glob(["*Test.java"]),
deps = [
+ "//java/com/google/gerrit/entities",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//javatests/com/google/gerrit/util/http/testutil",
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
index 1bb9784..e6974d1 100644
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
@@ -85,20 +85,4 @@
assertThat(thrown).hasMessageThat().contains("called create() twice on the same request");
}
}
-
- @Test
- public void enforceMaxSize() {
- try (PerThreadCache cache = PerThreadCache.create()) {
- // Fill the cache
- for (int i = 0; i < 50; i++) {
- PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, i);
- cache.get(key, () -> "cached value");
- }
- // Assert that the value was not persisted
- PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, 1000);
- cache.get(key, () -> "new value");
- String value = cache.get(key, () -> "directly served");
- assertThat(value).isEqualTo("directly served");
- }
- }
}
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadProjectCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadProjectCacheTest.java
new file mode 100644
index 0000000..055b95d
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/PerThreadProjectCacheTest.java
@@ -0,0 +1,51 @@
+// Copyright (C) 2023 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;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.entities.Project;
+import org.junit.Test;
+
+public class PerThreadProjectCacheTest {
+ @Test
+ public void testValueIsCachedWithinSizeLimit() {
+ try (PerThreadCache cache = PerThreadCache.create()) {
+ PerThreadCache.Key<Project.NameKey> key =
+ PerThreadCache.Key.create(Project.NameKey.class, Project.nameKey("test-project"));
+ PerThreadProjectCache.getOrCompute(key, () -> "cached");
+ String value = PerThreadProjectCache.getOrCompute(key, () -> "directly served");
+ assertThat(value).isEqualTo("cached");
+ }
+ }
+
+ @Test
+ public void testEnforceMaxSize() {
+ try (PerThreadCache cache = PerThreadCache.create()) {
+ // Fill the cache
+ for (int i = 0; i < 50; i++) {
+ PerThreadCache.Key<Project.NameKey> key =
+ PerThreadCache.Key.create(Project.NameKey.class, Project.nameKey("test-project" + i));
+ PerThreadProjectCache.getOrCompute(key, () -> "cached");
+ }
+ // Assert that the value was not persisted
+ PerThreadCache.Key<Project.NameKey> key =
+ PerThreadCache.Key.create(Project.NameKey.class, "Project" + 1000);
+ PerThreadProjectCache.getOrCompute(key, () -> "new value");
+ String value = PerThreadProjectCache.getOrCompute(key, () -> "directly served");
+ assertThat(value).isEqualTo("directly served");
+ }
+ }
+}
