opendev/gerrit
Code Issues Proposed changes

Remove PerThreadCache

Browse Source 
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
This commit is contained in:
Patrick Hiesel
2020-10-02 12:55:03 +02:00
parent b89332d7c7
commit 00b214c96d
4 changed files with 2 additions and 257 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
java/com/google/gerrit/httpd/restapi/RestApiServlet.java
View File

@@ -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));

146
java/com/google/gerrit/server/cache/PerThreadCache.java vendored
View File

@@ -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();
}
}

7
java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
View File

@@ -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(