Cache change /meta ref SHA1 for each REST API request

Some Gerrit REST APIs may cause multiple lookups of change /meta
ref name to SHA1 during the same request, which could be as slow
as 400 ms per lookup for a 500k refs repository on a shared
NFS volume.

Make sure that a lookup of a /meta ref happens only once per GET/HEAD
API request by caching the result in a PerThreadCache scoped to the
execution of the RestApiServlet.

The measured improvement for a change reindex API (repo with 500k
refs stored on an external NFS volume with trustfolderstat=false)
is from 5s down to 500ms (10 times faster).
For other APIs or local storage with trustfolderstat=true,
the benefits are much more limited.

NOTE: The improvement on all other APIs (PUT, POST, DELETE, PATCH)
is outside the scope of this change, because it would imply a more
complex eviction mechanism due to the mutation of the /meta ref
during the potential NoteDb manipulation performed.

Bug: Issue 15798
Release-Notes: cache the resolution of change /meta ref on REST API
Change-Id: If41377b78cb0bbbde0fb22489a47f7634248e6f6

6 files changed, 93 insertions, 12 deletions

diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 623d31d7d7..28939f75fe 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -289,7 +289,7 @@ public class RestApiServlet extends HttpServlet {
     ViewData viewData = null;
 
     try (TraceContext traceContext = enableTracing(req, res)) {
-      try (PerThreadCache ignored = PerThreadCache.create()) {
+      try (PerThreadCache ignored = PerThreadCache.create(req)) {
         logger.atFinest().log(
             "Received REST request: %s %s (parameters: %s)",
             req.getMethod(), req.getRequestURI(), getParameterNames(req));
diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
index b4f79d10ab..609fce7b25 100644
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ b/java/com/google/gerrit/server/cache/PerThreadCache.java
@@ -21,7 +21,9 @@ 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.Optional;
 import java.util.function.Supplier;
+import javax.servlet.http.HttpServletRequest;
 
 /**
  * Caches object instances for a request as {@link ThreadLocal} in the serving thread.
@@ -58,6 +60,12 @@ public class PerThreadCache implements AutoCloseable {
   private static final int PER_THREAD_CACHE_SIZE = 25;
 
   /**
+   * Optional HTTP request associated with the per-thread cache, should the thread be associated
+   * with the incoming HTTP thread pool.
+   */
+  private final Optional<HttpServletRequest> httpRequest;
+
+  /**
    * 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.
@@ -102,9 +110,9 @@ public class PerThreadCache implements AutoCloseable {
     }
   }
 
-  public static PerThreadCache create() {
+  public static PerThreadCache create(@Nullable HttpServletRequest httpRequest) {
     checkState(CACHE.get() == null, "called create() twice on the same request");
-    PerThreadCache cache = new PerThreadCache();
+    PerThreadCache cache = new PerThreadCache(httpRequest);
     CACHE.set(cache);
     return cache;
   }
@@ -121,7 +129,9 @@ public class PerThreadCache implements AutoCloseable {
 
   private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
 
-  private PerThreadCache() {}
+  private PerThreadCache(@Nullable HttpServletRequest req) {
+    httpRequest = Optional.ofNullable(req);
+  }
 
   /**
    * Returns an instance of {@code T} that was either loaded from the cache or obtained from the
@@ -139,6 +149,19 @@ public class PerThreadCache implements AutoCloseable {
     return value;
   }
 
+  /** Returns the optional HTTP request associated with the local thread cache. */
+  public Optional<HttpServletRequest> getHttpRequest() {
+    return httpRequest;
+  }
+
+  /** Returns true if there is an HTTP request associated and is a GET or HEAD */
+  public boolean hasReadonlyRequest() {
+    return httpRequest
+        .map(HttpServletRequest::getMethod)
+        .filter(m -> m.equalsIgnoreCase("GET") || m.equalsIgnoreCase("HEAD"))
+        .isPresent();
+  }
+
   @Override
   public void close() {
     CACHE.remove();
diff --git a/java/com/google/gerrit/server/git/RepoRefCache.java b/java/com/google/gerrit/server/git/RepoRefCache.java
index 6b2493a9f1..9086da7fb5 100644
--- a/java/com/google/gerrit/server/git/RepoRefCache.java
+++ b/java/com/google/gerrit/server/git/RepoRefCache.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.git;
 
+import com.google.gerrit.server.cache.PerThreadCache;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
@@ -29,6 +30,17 @@ public class RepoRefCache implements RefCache {
   private final RefDatabase refdb;
   private final Map<String, Optional<ObjectId>> ids;
 
+  public static Optional<RefCache> getOptional(Repository repo) {
+    PerThreadCache cache = PerThreadCache.get();
+    if (cache != null && cache.hasReadonlyRequest()) {
+      return Optional.of(
+          cache.get(
+              PerThreadCache.Key.create(RepoRefCache.class, repo), () -> new RepoRefCache(repo)));
+    }
+
+    return Optional.empty();
+  }
+
   public RepoRefCache(Repository repo) {
     this.refdb = repo.getRefDatabase();
     this.ids = new HashMap<>();
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index d548bf3fa9..3d97651969 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -725,7 +725,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   @Override
   protected ObjectId readRef(Repository repo) throws IOException {
-    return refs != null ? refs.get(getRefName()).orElse(null) : super.readRef(repo);
+    Optional<RefCache> refsCache =
+        Optional.ofNullable(refs).map(Optional::of).orElse(RepoRefCache.getOptional(repo));
+    return refsCache.isPresent()
+        ? refsCache.get().get(getRefName()).orElse(null)
+        : super.readRef(repo);
   }
 
   @Override
@@ -754,7 +758,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
         // ReviewDb claims NoteDb state exists, but meta ref isn't present: fall through and
         // auto-rebuild if necessary.
       }
-      RefCache refs = this.refs != null ? this.refs : new RepoRefCache(repo);
+      RefCache refs =
+          this.refs != null
+              ? this.refs
+              : RepoRefCache.getOptional(repo).orElse(new RepoRefCache(repo));
       if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {
         return rebuildAndOpen(repo, id);
       }
diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD
index 495026615b..84c1f18024 100644
--- a/javatests/com/google/gerrit/server/cache/BUILD
+++ b/javatests/com/google/gerrit/server/cache/BUILD
@@ -5,7 +5,10 @@ junit_tests(
     srcs = glob(["*.java"]),
     deps = [
         "//java/com/google/gerrit/server",
+        "//javatests/com/google/gerrit/util/http/testutil",
         "//lib:junit",
         "//lib/truth",
+        "//lib/truth:truth-java8-extension",
+        "@servlet-api-3_1//jar",
     ],
 )
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
index cfb5f3f361..f476d0bed9 100644
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
@@ -15,8 +15,12 @@
 package com.google.gerrit.server.cache;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 
+import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
 import java.util.function.Supplier;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -42,7 +46,7 @@ public class PerThreadCacheTest {
 
   @Test
   public void endToEndCache() {
-    try (PerThreadCache ignored = PerThreadCache.create()) {
+    try (PerThreadCache ignored = PerThreadCache.create(null)) {
       PerThreadCache cache = PerThreadCache.get();
       PerThreadCache.Key<String> key1 = PerThreadCache.Key.create(String.class);
 
@@ -60,7 +64,7 @@ public class PerThreadCacheTest {
   @Test
   public void cleanUp() {
     PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class);
-    try (PerThreadCache ignored = PerThreadCache.create()) {
+    try (PerThreadCache ignored = PerThreadCache.create(null)) {
       PerThreadCache cache = PerThreadCache.get();
       String value1 = cache.get(key, () -> "value1");
       assertThat(value1).isEqualTo("value1");
@@ -68,7 +72,7 @@ public class PerThreadCacheTest {
 
     // 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()) {
+    try (PerThreadCache ignored = PerThreadCache.create(null)) {
       PerThreadCache cache = PerThreadCache.get();
       String value1 = cache.get(key, () -> "value2");
       assertThat(value1).isEqualTo("value2");
@@ -77,16 +81,48 @@ public class PerThreadCacheTest {
 
   @Test
   public void doubleInstantiationFails() {
-    try (PerThreadCache ignored = PerThreadCache.create()) {
+    try (PerThreadCache ignored = PerThreadCache.create(null)) {
       exception.expect(IllegalStateException.class);
       exception.expectMessage("called create() twice on the same request");
-      PerThreadCache.create();
+      PerThreadCache.create(null);
+    }
+  }
+
+  @Test
+  public void isAssociatedWithHttpReadonlyRequest() {
+    HttpServletRequest getRequest = new FakeHttpServletRequest();
+    try (PerThreadCache cache = PerThreadCache.create(getRequest)) {
+      assertThat(cache.getHttpRequest()).hasValue(getRequest);
+      assertThat(cache.hasReadonlyRequest()).isTrue();
+    }
+  }
+
+  @Test
+  public void isAssociatedWithHttpWriteRequest() {
+    HttpServletRequest putRequest =
+        new HttpServletRequestWrapper(new FakeHttpServletRequest()) {
+          @Override
+          public String getMethod() {
+            return "PUT";
+          }
+        };
+    try (PerThreadCache cache = PerThreadCache.create(putRequest)) {
+      assertThat(cache.getHttpRequest()).hasValue(putRequest);
+      assertThat(cache.hasReadonlyRequest()).isFalse();
+    }
+  }
+
+  @Test
+  public void isNotAssociatedWithHttpRequest() {
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
+      assertThat(cache.getHttpRequest()).isEmpty();
+      assertThat(cache.hasReadonlyRequest()).isFalse();
     }
   }
 
   @Test
   public void enforceMaxSize() {
-    try (PerThreadCache cache = PerThreadCache.create()) {
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
       // Fill the cache
       for (int i = 0; i < 50; i++) {
         PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, i);