diff options
author | Luca Milanesio <luca.milanesio@gmail.com> | 2022-04-11 01:02:19 +0100 |
---|---|---|
committer | Luca Milanesio <luca.milanesio@gmail.com> | 2022-04-11 10:02:33 +0100 |
commit | 4c44557ae2f62bcce0f0ab993815905fb1e9959f (patch) | |
tree | bc671cd9f69928b1315e723dda39dc58b071b5db | |
parent | 23db8a546b800b2c91629d80b6f5117ff635238a (diff) |
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 |