Cache change /meta ref SHA1 for each change indexing task

Interactive indexing tasks of changes are executed synchronously
with the incoming API: multiple lookups of change /meta
ref name to SHA1 during the reindexing can compromise the
performance of the API. The refs lookups could be as slow
as 400 ms per lookup for a 500k refs repository on a shared
NFS volume.

Leverage the existing RepoRefCache associated with the PerThreadCache
by creating a read-only cache during the execution of the indexing
task.

NOTE: The indexing task may be part of a mutable API
(e.g. review a change); however, the operation itself is readonly
because it is executed once the mutation has already happened and
no further modifications to the change are done during the reindexing.

Bug: Issue 15798
Release-Notes: cache the resolution of change /meta ref upon interactive indexing
Change-Id: If137693293e3a6a5aa51babb5576690878d827d3

3 files changed, 44 insertions, 22 deletions

diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
index 609fce7b25..fe27bfb597 100644
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ b/java/com/google/gerrit/server/cache/PerThreadCache.java
@@ -21,7 +21,6 @@ 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;
 
@@ -60,10 +59,10 @@ 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.
+   * True when the current thread is associated with an incoming API request that is not changing
+   * any state.
    */
-  private final Optional<HttpServletRequest> httpRequest;
+  private final boolean readOnlyRequest;
 
   /**
    * Unique key for key-value mappings stored in PerThreadCache. The key is based on the value's
@@ -110,9 +109,29 @@ public class PerThreadCache implements AutoCloseable {
     }
   }
 
+  /**
+   * Creates a thread-local cache associated to an incoming HTTP request.
+   *
+   * <p>The request is considered as read-only if the associated method is GET or HEAD.
+   *
+   * @param httpRequest HTTP request associated with the thread-local cache
+   * @return thread-local cache
+   */
   public static PerThreadCache create(@Nullable HttpServletRequest httpRequest) {
     checkState(CACHE.get() == null, "called create() twice on the same request");
-    PerThreadCache cache = new PerThreadCache(httpRequest);
+    PerThreadCache cache = new PerThreadCache(httpRequest, false);
+    CACHE.set(cache);
+    return cache;
+  }
+
+  /**
+   * Creates a thread-local cache associated to an incoming read-only request.
+   *
+   * @return thread-local cache
+   */
+  public static PerThreadCache createReadOnly() {
+    checkState(CACHE.get() == null, "called create() twice on the same request");
+    PerThreadCache cache = new PerThreadCache(null, true);
     CACHE.set(cache);
     return cache;
   }
@@ -129,8 +148,12 @@ public class PerThreadCache implements AutoCloseable {
 
   private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
 
-  private PerThreadCache(@Nullable HttpServletRequest req) {
-    httpRequest = Optional.ofNullable(req);
+  private PerThreadCache(@Nullable HttpServletRequest req, boolean readOnly) {
+    readOnlyRequest =
+        readOnly
+            || (req != null
+                && (req.getMethod().equalsIgnoreCase("GET")
+                    || req.getMethod().equalsIgnoreCase("HEAD")));
   }
 
   /**
@@ -149,17 +172,9 @@ 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 */
+  /** Returns true if the associated request is read-only */
   public boolean hasReadonlyRequest() {
-    return httpRequest
-        .map(HttpServletRequest::getMethod)
-        .filter(m -> m.equalsIgnoreCase("GET") || m.equalsIgnoreCase("HEAD"))
-        .isPresent();
+    return readOnlyRequest;
   }
 
   @Override
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index 51a68b392f..affad63a39 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.cache.PerThreadCache;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.index.IndexExecutor;
 import com.google.gerrit.server.index.IndexUtils;
@@ -402,8 +403,10 @@ public class ChangeIndexer {
     @Override
     public Void callImpl(Provider<ReviewDb> db) throws Exception {
       remove();
-      ChangeData cd = newChangeData(db.get(), project, id);
-      index(cd);
+      try (PerThreadCache perThreadCache = PerThreadCache.createReadOnly()) {
+        ChangeData cd = newChangeData(db.get(), project, id);
+        index(cd);
+      }
       return null;
     }
 
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
index f476d0bed9..5a69401df8 100644
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
@@ -92,7 +92,6 @@ public class PerThreadCacheTest {
   public void isAssociatedWithHttpReadonlyRequest() {
     HttpServletRequest getRequest = new FakeHttpServletRequest();
     try (PerThreadCache cache = PerThreadCache.create(getRequest)) {
-      assertThat(cache.getHttpRequest()).hasValue(getRequest);
       assertThat(cache.hasReadonlyRequest()).isTrue();
     }
   }
@@ -107,7 +106,6 @@ public class PerThreadCacheTest {
           }
         };
     try (PerThreadCache cache = PerThreadCache.create(putRequest)) {
-      assertThat(cache.getHttpRequest()).hasValue(putRequest);
       assertThat(cache.hasReadonlyRequest()).isFalse();
     }
   }
@@ -115,12 +113,18 @@ public class PerThreadCacheTest {
   @Test
   public void isNotAssociatedWithHttpRequest() {
     try (PerThreadCache cache = PerThreadCache.create(null)) {
-      assertThat(cache.getHttpRequest()).isEmpty();
       assertThat(cache.hasReadonlyRequest()).isFalse();
     }
   }
 
   @Test
+  public void isAssociatedWithReadonlyRequest() {
+    try (PerThreadCache cache = PerThreadCache.createReadOnly()) {
+      assertThat(cache.hasReadonlyRequest()).isTrue();
+    }
+  }
+
+  @Test
   public void enforceMaxSize() {
     try (PerThreadCache cache = PerThreadCache.create(null)) {
       // Fill the cache