ChangeJson: Project control cache is dead; remove it
ChangeJson tries to use Cache<Project.NameKey, ProjectControl> to
optimize construction of ChangeControl in control() method:
if (cd.hasChangeControl()) {
ctrl = cd.changeControl().forUser(userProvider.get());
} else {
ctrl = projectControls.get(cd.change().getProject())
.controlFor(cd.change());
}
However during rendering of multiple changes as result of change query
execution, the condition:
if (cd.hasChangeControl()) {
ctrl = cd.changeControl().forUser(userProvider.get());
}
is always true and the cache is never used. That's because is_visible()
predicate is always get added to the query passed by user in:
public List<List<ChangeData>> queryChanges(List<String> queries) {
final Predicate<ChangeData> visibleToMe = queryBuilder.is_visible();
[...]
Predicate<ChangeData> q = parseQuery(query, visibleToMe);
Predicate<ChangeData> s = queryRewriter.rewrite(q, start);
And the is_visible() predicate creates ChangeControl and set it into
ChangeData instance by calling cacheVisibleTo() method:
void cacheVisibleTo(ChangeControl ctl) {
visibleTo = ctl.getCurrentUser();
changeControl = ctl;
}
Change-Id: I596c1be8f5fcd6060797a9d601d00c982689b6bd
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 4f4f1d9..5c1ffdf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -33,9 +33,6 @@
import com.google.common.base.Joiner;
import com.google.common.base.Objects;
import com.google.common.base.Optional;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
@@ -74,7 +71,6 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.PatchSetInfo.ParentInfo;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
@@ -88,7 +84,6 @@
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -100,7 +95,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.IOException;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.Collections;
@@ -111,7 +105,6 @@
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
-import java.util.concurrent.ExecutionException;
public class ChangeJson {
private static final Logger log = LoggerFactory.getLogger(ChangeJson.class);
@@ -137,7 +130,6 @@
private final Provider<CurrentUser> userProvider;
private final AnonymousUser anonymous;
private final IdentifiedUser.GenericFactory userFactory;
- private final ProjectControl.GenericFactory projectControlFactory;
private final ChangeData.Factory changeDataFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final ChangesCollection changes;
@@ -152,7 +144,6 @@
private AccountInfo.Loader accountLoader;
private ChangeControl lastControl;
private Set<Change.Id> reviewed;
- private LoadingCache<Project.NameKey, ProjectControl> projectControls;
private Provider<WebLinks> webLinks;
@Inject
@@ -178,7 +169,6 @@
this.userProvider = user;
this.anonymous = au;
this.userFactory = uf;
- this.projectControlFactory = pcf;
this.changeDataFactory = cdf;
this.patchSetInfoFactory = psi;
this.changes = changes;
@@ -191,15 +181,6 @@
this.webLinks = webLinks;
options = EnumSet.noneOf(ListChangesOption.class);
- projectControls = CacheBuilder.newBuilder()
- .concurrencyLevel(1)
- .build(new CacheLoader<Project.NameKey, ProjectControl>() {
- @Override
- public ProjectControl load(Project.NameKey key)
- throws NoSuchProjectException, IOException {
- return projectControlFactory.controlFor(key, userProvider.get());
- }
- });
}
public ChangeJson addOption(ListChangesOption o) {
@@ -369,19 +350,7 @@
&& cd.getId().equals(lastControl.getChange().getId())) {
return lastControl;
}
- ChangeControl ctrl;
- try {
- if (cd.hasChangeControl()) {
- ctrl = cd.changeControl().forUser(userProvider.get());
- } else {
- ctrl = projectControls.get(cd.change().getProject())
- .controlFor(cd.change());
- }
- } catch (ExecutionException e) {
- throw new OrmException(e);
- }
- lastControl = ctrl;
- return ctrl;
+ return lastControl = cd.changeControl().forUser(userProvider.get());
}
private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
