commit 00b214c96d9cb066af13d7f8a2100aaa8fd205e1
Author: Patrick Hiesel <hiesel@google.com>
Date:   Fri Oct 2 12:55:03 2020 +0200

    Remove PerThreadCache
    
    PerThreadCache was added Id5de21ed as a concept to cache state
    while processing a request. Some things have changed since then,
    most importantly, we have added more 'regular' caches to Gerrit.
    
    The only current use of this cache is to cache RefControls inside
    DefaultPermissionBackend#ForProject. We have instrumented the
    relevant code a while ago - PermissionCollection#filterLatency
    that is. The 99.99%-ile is 3ms, so extremely fast. This is a
    strong indication that we can just stop this caching without
    replacement.
    
    Change-Id: I1378ad083266f49484cc3f9b0584cd38d87c8211

diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index f743578..665cc33 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -108,7 +108,6 @@ import com.google.gerrit.server.OptionUtil;
 import com.google.gerrit.server.RequestInfo;
 import com.google.gerrit.server.RequestListener;
 import com.google.gerrit.server.audit.ExtendedHttpAuditEvent;
-import com.google.gerrit.server.cache.PerThreadCache;
 import com.google.gerrit.server.change.ChangeFinder;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.group.GroupAuditService;
@@ -328,7 +327,7 @@ public class RestApiServlet extends HttpServlet {
     try (TraceContext traceContext = enableTracing(req, res)) {
       List<IdString> path = splitPath(req);
 
-      try (PerThreadCache ignored = PerThreadCache.create()) {
+      try {
         RequestInfo requestInfo = createRequestInfo(traceContext, requestUri(req), path);
         globals.requestListeners.runEach(l -> l.onRequest(requestInfo));
 
diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
deleted file mode 100644
index b4f79d1..0000000
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ /dev/null
@@ -1,146 +0,0 @@
-// Copyright (C) 2018 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.base.Preconditions.checkState;
-
-import com.google.common.base.Objects;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Maps;
-import com.google.gerrit.common.Nullable;
-import java.util.Map;
-import java.util.function.Supplier;
-
-/**
- * Caches object instances for a request as {@link ThreadLocal} in the serving thread.
- *
- * <p>This class is intended to cache objects that have a high instantiation cost, are specific to
- * the current request and potentially need to be instantiated multiple times while serving a
- * request.
- *
- * <p>This is different from the key-value storage in {@code CurrentUser}: {@code CurrentUser}
- * offers a key-value storage by providing thread-safe {@code get} and {@code put} methods. Once the
- * value is retrieved through {@code get} there is not thread-safety anymore - apart from the
- * retrieved object guarantees. Depending on the implementation of {@code CurrentUser}, it might be
- * shared between the request serving thread as well as sub- or background treads.
- *
- * <p>In comparison to that, this class guarantees thread safety even on non-thread-safe objects as
- * its cache is tied to the serving thread only. While allowing to cache non-thread-safe objects, it
- * has the downside of not sharing any objects with background threads or executors.
- *
- * <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
-   * class and a list of identifiers that in combination uniquely set the object apart form others
-   * of the same class.
-   */
-  public static final class Key<T> {
-    private final Class<T> clazz;
-    private final ImmutableList<Object> identifiers;
-
-    /**
-     * Returns a key based on the value's class and an identifier that uniquely identify the value.
-     * The identifier needs to implement {@code equals()} and {@hashCode()}.
-     */
-    public static <T> Key<T> create(Class<T> clazz, Object identifier) {
-      return new Key<>(clazz, ImmutableList.of(identifier));
-    }
-
-    /**
-     * Returns a key based on the value's class and a set of identifiers that uniquely identify the
-     * value. Identifiers need to implement {@code equals()} and {@hashCode()}.
-     */
-    public static <T> Key<T> create(Class<T> clazz, Object... identifiers) {
-      return new Key<>(clazz, ImmutableList.copyOf(identifiers));
-    }
-
-    private Key(Class<T> clazz, ImmutableList<Object> identifiers) {
-      this.clazz = clazz;
-      this.identifiers = identifiers;
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hashCode(clazz, identifiers);
-    }
-
-    @Override
-    public boolean equals(Object o) {
-      if (!(o instanceof Key)) {
-        return false;
-      }
-      Key<?> other = (Key<?>) o;
-      return this.clazz == other.clazz && this.identifiers.equals(other.identifiers);
-    }
-  }
-
-  public static PerThreadCache create() {
-    checkState(CACHE.get() == null, "called create() twice on the same request");
-    PerThreadCache cache = new PerThreadCache();
-    CACHE.set(cache);
-    return cache;
-  }
-
-  @Nullable
-  public static PerThreadCache get() {
-    return CACHE.get();
-  }
-
-  public static <T> T getOrCompute(Key<T> key, Supplier<T> loader) {
-    PerThreadCache cache = get();
-    return cache != null ? cache.get(key, loader) : loader.get();
-  }
-
-  private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
-
-  private PerThreadCache() {}
-
-  /**
-   * Returns an instance of {@code T} that was either loaded from the cache or obtained from the
-   * provided {@link Supplier}.
-   */
-  public <T> T get(Key<T> key, Supplier<T> loader) {
-    @SuppressWarnings("unchecked")
-    T value = (T) cache.get(key);
-    if (value == null) {
-      value = loader.get();
-      if (cache.size() < PER_THREAD_CACHE_SIZE) {
-        cache.put(key, value);
-      }
-    }
-    return value;
-  }
-
-  @Override
-  public void close() {
-    CACHE.remove();
-  }
-}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index cf6a184..1b029b1 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -34,7 +34,6 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 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.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.inject.Inject;
@@ -105,11 +104,7 @@ public class DefaultPermissionBackend extends PermissionBackend {
     public ForProject project(Project.NameKey project) {
       try {
         ProjectState state = projectCache.get(project).orElseThrow(illegalState(project));
-        ProjectControl control =
-            PerThreadCache.getOrCompute(
-                PerThreadCache.Key.create(ProjectControl.class, project, user.getCacheKey()),
-                () -> projectControlFactory.create(user, state));
-        return control.asForProject();
+        return projectControlFactory.create(user, state).asForProject();
       } catch (Exception e) {
         Throwable cause = e.getCause() != null ? e.getCause() : e;
         return FailedPermissionBackend.project(
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
deleted file mode 100644
index 5d420d3..0000000
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ /dev/null
@@ -1,103 +0,0 @@
-// Copyright (C) 2018 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 static com.google.gerrit.testing.GerritJUnit.assertThrows;
-
-import java.util.function.Supplier;
-import org.junit.Test;
-
-public class PerThreadCacheTest {
-
-  @SuppressWarnings("TruthIncompatibleType")
-  @Test
-  public void key_respectsClass() {
-    assertThat(PerThreadCache.Key.create(String.class))
-        .isEqualTo(PerThreadCache.Key.create(String.class));
-    assertThat(PerThreadCache.Key.create(String.class))
-        .isNotEqualTo(
-            /* expected: Key<String>, actual: Key<Integer> */ PerThreadCache.Key.create(
-                Integer.class));
-  }
-
-  @Test
-  public void key_respectsIdentifiers() {
-    assertThat(PerThreadCache.Key.create(String.class, "id1"))
-        .isEqualTo(PerThreadCache.Key.create(String.class, "id1"));
-    assertThat(PerThreadCache.Key.create(String.class, "id1"))
-        .isNotEqualTo(PerThreadCache.Key.create(String.class, "id2"));
-  }
-
-  @Test
-  public void endToEndCache() {
-    try (PerThreadCache ignored = PerThreadCache.create()) {
-      PerThreadCache cache = PerThreadCache.get();
-      PerThreadCache.Key<String> key1 = PerThreadCache.Key.create(String.class);
-
-      String value1 = cache.get(key1, () -> "value1");
-      assertThat(value1).isEqualTo("value1");
-
-      Supplier<String> neverCalled =
-          () -> {
-            throw new IllegalStateException("this method must not be called");
-          };
-      assertThat(cache.get(key1, neverCalled)).isEqualTo("value1");
-    }
-  }
-
-  @Test
-  public void cleanUp() {
-    PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class);
-    try (PerThreadCache ignored = PerThreadCache.create()) {
-      PerThreadCache cache = PerThreadCache.get();
-      String value1 = cache.get(key, () -> "value1");
-      assertThat(value1).isEqualTo("value1");
-    }
-
-    // Create a second cache and assert that it is not connected to the first one.
-    // This ensures that the cleanup is actually working.
-    try (PerThreadCache ignored = PerThreadCache.create()) {
-      PerThreadCache cache = PerThreadCache.get();
-      String value1 = cache.get(key, () -> "value2");
-      assertThat(value1).isEqualTo("value2");
-    }
-  }
-
-  @Test
-  public void doubleInstantiationFails() {
-    try (PerThreadCache ignored = PerThreadCache.create()) {
-      IllegalStateException thrown =
-          assertThrows(IllegalStateException.class, () -> PerThreadCache.create());
-      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");
-    }
-  }
-}