Switch PatchListCache to using legacy cache backend
The implementation of PatchListLoader is using internally the
PatchListCache, to retrieve rebase related edits. The loading function
calling back into the cache violates the Caffeine interface contracts.
Guava could, theoretically, be more tolerant than Caffeine. This is due
to the hashtable design of locking and table expansion. However both are
explicit that the cache loader cannot write into the cache. Caffeine
will fail a fibonacci test, whereas Guava might pass. But that is
taking a lot of risk by relying on implementation details and violating
the contracts.
Switch to using legacy cache backend for PatchListCache, and consider
to switch using Caffeine cache backend and remove the legacy cache
support altogether, when the PatchListLoader is fixed to be recursion
free.
Bug: Issue 11900
Change-Id: I8f129dcf9b5574f024a600162d98aa381b3513cc
(cherry picked from commit 1d8f5ca43512ea7bc293520bfdbca23f1ad663fd)
diff --git a/java/com/google/gerrit/server/cache/CacheBackend.java b/java/com/google/gerrit/server/cache/CacheBackend.java
new file mode 100644
index 0000000..ec9876f
--- /dev/null
+++ b/java/com/google/gerrit/server/cache/CacheBackend.java
@@ -0,0 +1,25 @@
+// Copyright (C) 2019 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;
+
+/** Caffeine is used as default cache backend, but can be overridden with Guava backend. */
+public enum CacheBackend {
+ CAFFEINE,
+ GUAVA;
+
+ public boolean isLegacyBackend() {
+ return this == GUAVA;
+ }
+}
diff --git a/java/com/google/gerrit/server/cache/CacheModule.java b/java/com/google/gerrit/server/cache/CacheModule.java
index 2878624..0fdc6f5 100644
--- a/java/com/google/gerrit/server/cache/CacheModule.java
+++ b/java/com/google/gerrit/server/cache/CacheModule.java
@@ -68,7 +68,8 @@
*/
protected <K, V> CacheBinding<K, V> cache(
String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) {
- CacheProvider<K, V> m = new CacheProvider<>(this, name, keyType, valType);
+ CacheProvider<K, V> m =
+ new CacheProvider<>(this, name, keyType, valType, CacheBackend.CAFFEINE);
bindCache(m, name, keyType, valType);
return m;
}
@@ -123,7 +124,20 @@
*/
protected <K, V> PersistentCacheBinding<K, V> persist(
String name, Class<K> keyType, Class<V> valType) {
- return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType));
+ return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType), CacheBackend.CAFFEINE);
+ }
+
+ /**
+ * Declare a named in-memory/on-disk cache.
+ *
+ * @param <K> type of key used to lookup entries.
+ * @param <V> type of value stored by the cache.
+ * @param backend cache backend.
+ * @return binding to describe the cache.
+ */
+ protected <K, V> PersistentCacheBinding<K, V> persist(
+ String name, Class<K> keyType, Class<V> valType, CacheBackend backend) {
+ return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType), backend);
}
/**
@@ -135,7 +149,7 @@
*/
protected <K, V> PersistentCacheBinding<K, V> persist(
String name, Class<K> keyType, TypeLiteral<V> valType) {
- return persist(name, TypeLiteral.get(keyType), valType);
+ return persist(name, TypeLiteral.get(keyType), valType, CacheBackend.CAFFEINE);
}
/**
@@ -146,8 +160,9 @@
* @return binding to describe the cache.
*/
protected <K, V> PersistentCacheBinding<K, V> persist(
- String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) {
- PersistentCacheProvider<K, V> m = new PersistentCacheProvider<>(this, name, keyType, valType);
+ String name, TypeLiteral<K> keyType, TypeLiteral<V> valType, CacheBackend backend) {
+ PersistentCacheProvider<K, V> m =
+ new PersistentCacheProvider<>(this, name, keyType, valType, backend);
bindCache(m, name, keyType, valType);
Type cacheDefType =
diff --git a/java/com/google/gerrit/server/cache/CacheProvider.java b/java/com/google/gerrit/server/cache/CacheProvider.java
index b1a9b91..fe4244c 100644
--- a/java/com/google/gerrit/server/cache/CacheProvider.java
+++ b/java/com/google/gerrit/server/cache/CacheProvider.java
@@ -30,6 +30,7 @@
class CacheProvider<K, V> implements Provider<Cache<K, V>>, CacheBinding<K, V>, CacheDef<K, V> {
private final CacheModule module;
+ private final CacheBackend backend;
final String name;
private final TypeLiteral<K> keyType;
private final TypeLiteral<V> valType;
@@ -44,11 +45,17 @@
private MemoryCacheFactory memoryCacheFactory;
private boolean frozen;
- CacheProvider(CacheModule module, String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) {
+ CacheProvider(
+ CacheModule module,
+ String name,
+ TypeLiteral<K> keyType,
+ TypeLiteral<V> valType,
+ CacheBackend backend) {
this.module = module;
this.name = name;
this.keyType = keyType;
this.valType = valType;
+ this.backend = backend;
}
@Inject(optional = true)
@@ -159,7 +166,9 @@
public Cache<K, V> get() {
freeze();
CacheLoader<K, V> ldr = loader();
- return ldr != null ? memoryCacheFactory.build(this, ldr) : memoryCacheFactory.build(this);
+ return ldr != null
+ ? memoryCacheFactory.build(this, ldr, backend)
+ : memoryCacheFactory.build(this, backend);
}
protected void checkNotFrozen() {
diff --git a/java/com/google/gerrit/server/cache/MemoryCacheFactory.java b/java/com/google/gerrit/server/cache/MemoryCacheFactory.java
index fc55753..558380d 100644
--- a/java/com/google/gerrit/server/cache/MemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/MemoryCacheFactory.java
@@ -19,7 +19,8 @@
import com.google.common.cache.LoadingCache;
public interface MemoryCacheFactory {
- <K, V> Cache<K, V> build(CacheDef<K, V> def);
+ <K, V> Cache<K, V> build(CacheDef<K, V> def, CacheBackend backend);
- <K, V> LoadingCache<K, V> build(CacheDef<K, V> def, CacheLoader<K, V> loader);
+ <K, V> LoadingCache<K, V> build(
+ CacheDef<K, V> def, CacheLoader<K, V> loader, CacheBackend backend);
}
diff --git a/java/com/google/gerrit/server/cache/PersistentCacheFactory.java b/java/com/google/gerrit/server/cache/PersistentCacheFactory.java
index 27fa9ca..93f91ef 100644
--- a/java/com/google/gerrit/server/cache/PersistentCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/PersistentCacheFactory.java
@@ -19,9 +19,10 @@
import com.google.common.cache.LoadingCache;
public interface PersistentCacheFactory {
- <K, V> Cache<K, V> build(PersistentCacheDef<K, V> def);
+ <K, V> Cache<K, V> build(PersistentCacheDef<K, V> def, CacheBackend backend);
- <K, V> LoadingCache<K, V> build(PersistentCacheDef<K, V> def, CacheLoader<K, V> loader);
+ <K, V> LoadingCache<K, V> build(
+ PersistentCacheDef<K, V> def, CacheLoader<K, V> loader, CacheBackend backend);
void onStop(String plugin);
}
diff --git a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java
index 59d66e3..4fc107f 100644
--- a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java
+++ b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java
@@ -30,6 +30,7 @@
class PersistentCacheProvider<K, V> extends CacheProvider<K, V>
implements Provider<Cache<K, V>>, PersistentCacheBinding<K, V>, PersistentCacheDef<K, V> {
+ private final CacheBackend backend;
private int version;
private long diskLimit;
private CacheSerializer<K> keySerializer;
@@ -39,9 +40,19 @@
PersistentCacheProvider(
CacheModule module, String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) {
- super(module, name, keyType, valType);
+ this(module, name, keyType, valType, CacheBackend.CAFFEINE);
+ }
+
+ PersistentCacheProvider(
+ CacheModule module,
+ String name,
+ TypeLiteral<K> keyType,
+ TypeLiteral<V> valType,
+ CacheBackend backend) {
+ super(module, name, keyType, valType, backend);
version = -1;
diskLimit = 128 << 20;
+ this.backend = backend;
}
@Inject(optional = true)
@@ -130,8 +141,8 @@
freeze();
CacheLoader<K, V> ldr = loader();
return ldr != null
- ? persistentCacheFactory.build(this, ldr)
- : persistentCacheFactory.build(this);
+ ? persistentCacheFactory.build(this, ldr, backend)
+ : persistentCacheFactory.build(this, backend);
}
private static <T> void checkSerializer(
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
index af1228d..2b068aa 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
@@ -21,6 +21,7 @@
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.MemoryCacheFactory;
import com.google.gerrit.server.cache.PersistentCacheDef;
import com.google.gerrit.server.cache.PersistentCacheFactory;
@@ -156,18 +157,21 @@
@SuppressWarnings({"unchecked"})
@Override
- public <K, V> Cache<K, V> build(PersistentCacheDef<K, V> in) {
+ public <K, V> Cache<K, V> build(PersistentCacheDef<K, V> in, CacheBackend backend) {
long limit = config.getLong("cache", in.configKey(), "diskLimit", in.diskLimit());
if (cacheDir == null || limit <= 0) {
- return memCacheFactory.build(in);
+ return memCacheFactory.build(in, backend);
}
H2CacheDefProxy<K, V> def = new H2CacheDefProxy<>(in);
SqlStore<K, V> store = newSqlStore(def, limit);
H2CacheImpl<K, V> cache =
new H2CacheImpl<>(
- executor, store, def.keyType(), (Cache<K, ValueHolder<V>>) memCacheFactory.build(def));
+ executor,
+ store,
+ def.keyType(),
+ (Cache<K, ValueHolder<V>>) memCacheFactory.build(def, backend));
synchronized (caches) {
caches.add(cache);
}
@@ -176,11 +180,12 @@
@SuppressWarnings("unchecked")
@Override
- public <K, V> LoadingCache<K, V> build(PersistentCacheDef<K, V> in, CacheLoader<K, V> loader) {
+ public <K, V> LoadingCache<K, V> build(
+ PersistentCacheDef<K, V> in, CacheLoader<K, V> loader, CacheBackend backend) {
long limit = config.getLong("cache", in.configKey(), "diskLimit", in.diskLimit());
if (cacheDir == null || limit <= 0) {
- return memCacheFactory.build(in, loader);
+ return memCacheFactory.build(in, loader, backend);
}
H2CacheDefProxy<K, V> def = new H2CacheDefProxy<>(in);
@@ -188,7 +193,9 @@
Cache<K, ValueHolder<V>> mem =
(Cache<K, ValueHolder<V>>)
memCacheFactory.build(
- def, (CacheLoader<K, V>) new H2CacheImpl.Loader<>(executor, store, loader));
+ def,
+ (CacheLoader<K, V>) new H2CacheImpl.Loader<>(executor, store, loader),
+ backend);
H2CacheImpl<K, V> cache = new H2CacheImpl<>(executor, store, def.keyType(), mem);
synchronized (caches) {
caches.add(cache);
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 0a42ce1..9906b3d 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -23,10 +23,12 @@
import com.github.benmanes.caffeine.guava.CaffeinatedGuava;
import com.google.common.base.Strings;
import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.RemovalNotification;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.CacheDef;
import com.google.gerrit.server.cache.ForwardingRemovalListener;
import com.google.gerrit.server.cache.MemoryCacheFactory;
@@ -49,13 +51,61 @@
}
@Override
- public <K, V> Cache<K, V> build(CacheDef<K, V> def) {
- return CaffeinatedGuava.build(create(def));
+ public <K, V> Cache<K, V> build(CacheDef<K, V> def, CacheBackend backend) {
+ return backend.isLegacyBackend()
+ ? createLegacy(def).build()
+ : CaffeinatedGuava.build(create(def));
}
@Override
- public <K, V> LoadingCache<K, V> build(CacheDef<K, V> def, CacheLoader<K, V> loader) {
- return CaffeinatedGuava.build(create(def), loader);
+ 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);
+ }
+
+ @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 = builder.removalListener(forwardingRemovalListenerFactory.create(def.name()));
+
+ com.google.common.cache.Weigher<K, V> weigher = def.weigher();
+ if (weigher == null) {
+ weigher = unitWeight();
+ }
+ builder.weigher(weigher);
+
+ Duration expireAfterWrite = def.expireAfterWrite();
+ if (has(def.configKey(), "maxAge")) {
+ builder.expireAfterWrite(
+ ConfigUtil.getTimeUnit(
+ cfg, "cache", def.configKey(), "maxAge", toSeconds(expireAfterWrite), SECONDS),
+ SECONDS);
+ } else if (expireAfterWrite != null) {
+ builder.expireAfterWrite(expireAfterWrite.toNanos(), NANOSECONDS);
+ }
+
+ Duration expireAfterAccess = def.expireFromMemoryAfterAccess();
+ if (has(def.configKey(), "expireFromMemoryAfterAccess")) {
+ builder.expireAfterAccess(
+ ConfigUtil.getTimeUnit(
+ cfg,
+ "cache",
+ def.configKey(),
+ "expireFromMemoryAfterAccess",
+ toSeconds(expireAfterAccess),
+ SECONDS),
+ SECONDS);
+ } else if (expireAfterAccess != null) {
+ builder.expireAfterAccess(expireAfterAccess.toNanos(), NANOSECONDS);
+ }
+
+ return builder;
}
private <K, V> Caffeine<K, V> create(CacheDef<K, V> def) {
@@ -103,6 +153,15 @@
}
@SuppressWarnings("unchecked")
+ private static <K, V> CacheBuilder<K, V> newLegacyCacheBuilder() {
+ return (CacheBuilder<K, V>) CacheBuilder.newBuilder();
+ }
+
+ private static <K, V> com.google.common.cache.Weigher<K, V> unitWeight() {
+ return (key, value) -> 1;
+ }
+
+ @SuppressWarnings("unchecked")
private static <K, V> Caffeine<K, V> newCacheBuilder() {
return (Caffeine<K, V>) Caffeine.newBuilder();
}
diff --git a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
index 6039fff..20f0f72 100644
--- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -22,6 +22,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
@@ -45,7 +46,9 @@
@Override
protected void configure() {
factory(PatchListLoader.Factory.class);
- persist(FILE_NAME, PatchListKey.class, PatchList.class)
+ // TODO(davido): Switch off using legacy cache backend, after fixing PatchListLoader
+ // to be recursion free.
+ persist(FILE_NAME, PatchListKey.class, PatchList.class, CacheBackend.GUAVA)
.maximumWeight(10 << 20)
.weigher(PatchListWeigher.class);
diff --git a/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java
index 1892566..424dbac 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java
@@ -39,7 +39,9 @@
import com.google.gerrit.server.patch.Text;
import com.google.inject.Inject;
import com.google.inject.name.Named;
+import java.lang.reflect.Field;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
@@ -66,6 +68,25 @@
private Cache<PatchListKey, PatchList> abstractPatchListCache;
@Test
+ public void ensureLegacyBackendIsUsedForFileCacheBackend() throws Exception {
+ Field fileCacheField = patchListCache.getClass().getDeclaredField("fileCache");
+ fileCacheField.setAccessible(true);
+ // Use the reflection to access "localCache" field that is only present in Guava backend.
+ assertThat(
+ Arrays.stream(fileCacheField.get(patchListCache).getClass().getDeclaredFields())
+ .anyMatch(f -> f.getName().equals("localCache")))
+ .isTrue();
+
+ // intraCache (and all other cache backends) should use Caffeine backend.
+ Field intraCacheField = patchListCache.getClass().getDeclaredField("intraCache");
+ intraCacheField.setAccessible(true);
+ assertThat(
+ Arrays.stream(intraCacheField.get(patchListCache).getClass().getDeclaredFields())
+ .noneMatch(f -> f.getName().equals("localCache")))
+ .isTrue();
+ }
+
+ @Test
public void listPatchesAgainstBase() throws Exception {
commitBuilder().add(FILE_D, "4").message(SUBJECT_1).create();
pushHead(testRepo, "refs/heads/master", false);
