Introduce a PassthroughLoadingCache for disabled caches
Gerrit documentation states that when the memoryLimit of a cache
is zero then it is disabled. However, the code told a different
story because there was always a Guava/Caffeine cache created
with a potential impact on performance due to the inherent locking
of the underlying ConcurrentHashMap.
The ConcurrentHashMap would always lock additions and removals on
the same key, and the only way to avoid locking is to avoid
the instantiation of the ConcurrentHashMap.
Why should someone want to ever disable a cache?
Disabling the cache typically isn't a good idea, however, there
could be situations where:
1. The rate of the access of the cached value for K is low and the
load time is high
2. The rate of the eviction of the cached value for K is high
The Issue 16379 is one example of the above condition, however,
there are more instances of the same problem.
The eviction done at 2. for K would then cause all the incoming threads
to wait until the load at 1. for K is done.
Introduce a simple LoadingCache implementation called
PassthroughLoadingCache that is *really* just delegating the
calls to the underlying loader without involving any caching
or locking.
This change also makes the memoryLimit behaviour consistent with the
diskLimit set to zero means disabling the implementation of the cache.
Bug: Issue 16379
Release-Notes: Increase overall performance of disabled caches by avoiding any locking due to Guava/Caffeine caches with zero weight.
Change-Id: I5906a1f250014d96aaadcda0f64a37a78030d047
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 46df4cb..75df348 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -830,8 +830,14 @@
* `"plugin_resources"`: default is 2m (2 MiB of memory)
+
-If set to 0 the cache is disabled. Entries are removed immediately
-after being stored by the cache. This is primarily useful for testing.
+If set to 0 the cache is disabled; entries are loaded but not stored
+in-memory.
++
+**NOTE**: When the cache is disabled, there is no locking when accessing
+the same key/value, and therefore multiple threads may
+load the same value concurrently with a higher memory footprint.
+To keep a minimum caching and avoid concurrent loading of the same
+key/value, set `memoryLimit` to `1` and `maxAge` to `1`.
[[cache.name.expireFromMemoryAfterAccess]]cache.<name>.expireFromMemoryAfterAccess::
+
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 8e06294..42f4879 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -60,17 +60,18 @@
@Override
public <K, V> LoadingCache<K, V> build(
CacheDef<K, V> def, CacheLoader<K, V> loader, CacheBackend backend) {
- return backend.isLegacyBackend()
- ? createLegacy(def).build(loader)
- : CaffeinatedGuava.build(create(def), loader);
+ return cacheMaximumWeight(def) == 0
+ ? new PassthroughLoadingCache<>(loader)
+ : (backend.isLegacyBackend()
+ ? createLegacy(def).build(loader)
+ : CaffeinatedGuava.build(create(def), loader));
}
@SuppressWarnings("unchecked")
private <K, V> CacheBuilder<K, V> createLegacy(CacheDef<K, V> def) {
CacheBuilder<K, V> builder = newLegacyCacheBuilder();
builder.recordStats();
- builder.maximumWeight(
- cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight()));
+ builder.maximumWeight(cacheMaximumWeight(def));
builder = builder.removalListener(forwardingRemovalListenerFactory.create(def.name()));
@@ -126,8 +127,7 @@
private <K, V> Caffeine<K, V> create(CacheDef<K, V> def) {
Caffeine<K, V> builder = newCacheBuilder();
builder.recordStats();
- builder.maximumWeight(
- cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight()));
+ builder.maximumWeight(cacheMaximumWeight(def));
builder = builder.removalListener(newRemovalListener(def.name()));
builder.weigher(newWeigher(def.weigher()));
@@ -174,6 +174,10 @@
return builder;
}
+ private <K, V> long cacheMaximumWeight(CacheDef<K, V> def) {
+ return cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight());
+ }
+
private static long toSeconds(@Nullable Duration duration) {
return duration != null ? duration.getSeconds() : 0;
}
diff --git a/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
new file mode 100644
index 0000000..4ed8cfa
--- /dev/null
+++ b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
@@ -0,0 +1,141 @@
+// Copyright (C) 2022 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.mem;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.CacheLoader.UnsupportedLoadingOperationException;
+import com.google.common.cache.CacheStats;
+import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import java.util.Map;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutionException;
+
+/** Implementation of a NOOP cache that just passes all gets to the loader */
+public class PassthroughLoadingCache<K, V> implements LoadingCache<K, V> {
+
+ private final CacheLoader<? super K, V> cacheLoader;
+
+ public PassthroughLoadingCache(CacheLoader<? super K, V> cacheLoader) {
+ this.cacheLoader = cacheLoader;
+ }
+
+ @Override
+ public @Nullable V getIfPresent(Object key) {
+ return null;
+ }
+
+ @Override
+ public V get(K key, Callable<? extends V> loader) throws ExecutionException {
+ try {
+ return loader.call();
+ } catch (Exception e) {
+ throw new ExecutionException(e);
+ }
+ }
+
+ @Override
+ public ImmutableMap<K, V> getAllPresent(Iterable<?> keys) {
+ return ImmutableMap.of();
+ }
+
+ @Override
+ public void put(K key, V value) {}
+
+ @Override
+ public void putAll(Map<? extends K, ? extends V> m) {}
+
+ @Override
+ public void invalidate(Object key) {}
+
+ @Override
+ public void invalidateAll(Iterable<?> keys) {}
+
+ @Override
+ public void invalidateAll() {}
+
+ @Override
+ public long size() {
+ return 0;
+ }
+
+ @Override
+ public CacheStats stats() {
+ return new CacheStats(0, 0, 0, 0, 0, 0);
+ }
+
+ @Override
+ public void cleanUp() {}
+
+ @Override
+ public V get(K key) throws ExecutionException {
+ try {
+ return cacheLoader.load(key);
+ } catch (Exception e) {
+ throw new ExecutionException(e);
+ }
+ }
+
+ @Override
+ public V getUnchecked(K key) {
+ try {
+ return cacheLoader.load(key);
+ } catch (Exception e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ @Override
+ public ImmutableMap<K, V> getAll(Iterable<? extends K> keys) throws ExecutionException {
+ try {
+ try {
+ return getAllBulk(keys);
+ } catch (UnsupportedLoadingOperationException e) {
+ return getAllIndividually(keys);
+ }
+ } catch (Exception e) {
+ throw new ExecutionException(e);
+ }
+ }
+
+ private ImmutableMap<K, V> getAllIndividually(Iterable<? extends K> keys) throws Exception {
+ ImmutableMap.Builder<K, V> builder = ImmutableMap.builder();
+ for (K k : keys) {
+ builder.put(k, cacheLoader.load(k));
+ }
+ return builder.build();
+ }
+
+ @SuppressWarnings("unchecked")
+ public ImmutableMap<K, V> getAllBulk(Iterable<? extends K> keys) throws Exception {
+ return (ImmutableMap<K, V>) cacheLoader.loadAll(keys);
+ }
+
+ @Override
+ public V apply(K key) {
+ return getUnchecked(key);
+ }
+
+ @Override
+ public void refresh(K key) {}
+
+ @Override
+ public ConcurrentMap<K, V> asMap() {
+ return new ConcurrentHashMap<>();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/cache/mem/BUILD b/javatests/com/google/gerrit/server/cache/mem/BUILD
new file mode 100644
index 0000000..a263c7b
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/mem/BUILD
@@ -0,0 +1,14 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+ name = "tests",
+ srcs = glob(["*Test.java"]),
+ deps = [
+ "//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/cache/mem",
+ "//lib:jgit",
+ "//lib:junit",
+ "//lib/guice",
+ "//lib/truth",
+ ],
+)
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
new file mode 100644
index 0000000..e0c0c34
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -0,0 +1,180 @@
+// Copyright (C) 2022 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.mem;
+
+import static com.google.common.base.Functions.identity;
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.cache.Weigher;
+import com.google.gerrit.server.cache.CacheBackend;
+import com.google.gerrit.server.cache.CacheDef;
+import com.google.inject.TypeLiteral;
+import java.time.Duration;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Function;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+public class DefaultMemoryCacheFactoryTest {
+
+ private static final String TEST_CACHE = "test-cache";
+ private static final long TEST_TIMEOUT_SEC = 1;
+ private static final int TEST_CACHE_KEY = 1;
+
+ private DefaultMemoryCacheFactory memoryCacheFactory;
+ private Config memoryCacheConfig;
+ private ScheduledExecutorService executor;
+ private CyclicBarrier cacheGetStarted;
+ private CyclicBarrier cacheGetCompleted;
+
+ @Before
+ public void setUp() {
+ memoryCacheConfig = new Config();
+ memoryCacheFactory = new DefaultMemoryCacheFactory(memoryCacheConfig, null);
+ executor = Executors.newScheduledThreadPool(1);
+ cacheGetStarted = new CyclicBarrier(2);
+ cacheGetCompleted = new CyclicBarrier(2);
+ }
+
+ @Test
+ public void shouldNotBlockEvictionsWhenCacheIsDisabledByDefault() throws Exception {
+ LoadingCache<Integer, Integer> disabledCache =
+ memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+ assertCacheEvictionIsNotBlocking(disabledCache);
+ }
+
+ @Test
+ public void shouldNotBlockEvictionsWhenCacheIsDisabledByConfiguration() throws Exception {
+ memoryCacheConfig.setInt("cache", TEST_CACHE, "memoryLimit", 0);
+ LoadingCache<Integer, Integer> disabledCache =
+ memoryCacheFactory.build(newCacheDef(1), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+ assertCacheEvictionIsNotBlocking(disabledCache);
+ }
+
+ @Test
+ public void shouldBlockEvictionsWhenCacheIsEnabled() throws Exception {
+ LoadingCache<Integer, Integer> cache =
+ memoryCacheFactory.build(newCacheDef(1), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+ ScheduledFuture<Integer> cacheValue =
+ executor.schedule(() -> cache.getUnchecked(TEST_CACHE_KEY), 0, TimeUnit.SECONDS);
+
+ cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ cache.invalidate(TEST_CACHE_KEY);
+
+ assertThat(cacheValue.isDone()).isTrue();
+ assertThat(cacheValue.get()).isEqualTo(TEST_CACHE_KEY);
+ }
+
+ private void assertCacheEvictionIsNotBlocking(LoadingCache<Integer, Integer> disabledCache)
+ throws InterruptedException, BrokenBarrierException, TimeoutException, ExecutionException {
+ ScheduledFuture<Integer> cacheValue =
+ executor.schedule(() -> disabledCache.getUnchecked(TEST_CACHE_KEY), 0, TimeUnit.SECONDS);
+ cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ disabledCache.invalidate(TEST_CACHE_KEY);
+
+ // The invalidate did not wait for the cache loader to finish, therefore the cacheValue isn't
+ // done yet
+ assertThat(cacheValue.isDone()).isFalse();
+
+ // The cache loader completes after the invalidation
+ cacheGetCompleted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ assertThat(cacheValue.get()).isEqualTo(TEST_CACHE_KEY);
+ }
+
+ private CacheLoader<Integer, Integer> newCacheLoader(Function<Integer, Integer> loadFunc) {
+ return new CacheLoader<Integer, Integer>() {
+
+ @Override
+ public Integer load(Integer n) throws Exception {
+ Integer v = 0;
+ try {
+ cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ v = loadFunc.apply(n);
+ cacheGetCompleted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ } catch (TimeoutException | BrokenBarrierException e) {
+ }
+ return v;
+ }
+ };
+ }
+
+ private CacheDef<Integer, Integer> newCacheDef(long maximumWeight) {
+ return new CacheDef<Integer, Integer>() {
+
+ @Override
+ public String name() {
+ return TEST_CACHE;
+ }
+
+ @Override
+ public String configKey() {
+ return TEST_CACHE;
+ }
+
+ @Override
+ public TypeLiteral<Integer> keyType() {
+ return null;
+ }
+
+ @Override
+ public TypeLiteral<Integer> valueType() {
+ return null;
+ }
+
+ @Override
+ public long maximumWeight() {
+ return maximumWeight;
+ }
+
+ @Override
+ public Duration expireAfterWrite() {
+ return null;
+ }
+
+ @Override
+ public Duration expireFromMemoryAfterAccess() {
+ return null;
+ }
+
+ @Override
+ public Duration refreshAfterWrite() {
+ return null;
+ }
+
+ @Override
+ public Weigher<Integer, Integer> weigher() {
+ return null;
+ }
+
+ @Override
+ public CacheLoader<Integer, Integer> loader() {
+ return null;
+ }
+ };
+ }
+}
