opendev/gerrit
Code Issues Proposed changes

Reuse cached RefControl data in FunctionState

Browse Source 
Currently the FunctionState code is run for every change in a
change listing in the web UI, or the user dashboard. Looking
up the relevant permissions for each reviewer is costly, but
can typically be cached and reused off the request's cache,
as most changes are not on username specific references.

Change-Id: Idff421e1eecf651db11549d6591484578cd9543c
This commit is contained in:
Shawn O. Pearce
2011-06-22 17:39:44 -07:00
parent bee0aeafa1
commit a429b1a2db
12 changed files with 85 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
View File

@@ -154,8 +154,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
db.patchSetApprovals().byChange(changeId).toList();
if (detail.getChange().getStatus().isOpen()) {
final FunctionState fs =
functionState.create(detail.getChange(), psId, allApprovals);
final FunctionState fs = functionState.create(control, psId, allApprovals);
for (final ApprovalType at : approvalTypes.getApprovalTypes()) {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);

4
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
View File

@@ -182,7 +182,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
final Map<ApprovalCategory.Id, PatchSetApproval> psas =
new HashMap<ApprovalCategory.Id, PatchSetApproval>();
final FunctionState fs =
functionStateFactory.create(change, ps_id, psas.values());
functionStateFactory.create(cc, ps_id, psas.values());
for (final PatchSetApproval ca : db.patchSetApprovals()
.byPatchSetUser(ps_id, aid)) {
@@ -229,7 +229,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
final Map<ApprovalCategory.Id, PatchSetApproval> psas =
new HashMap<ApprovalCategory.Id, PatchSetApproval>();
final FunctionState fs =
functionStateFactory.create(change, ps_id, psas.values());
functionStateFactory.create(cc, ps_id, psas.values());
for (PatchSetApproval ca : db.patchSetApprovals().byPatchSet(ps_id)) {
final ApprovalCategory.Id category = ca.getCategoryId();

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
View File

@@ -40,6 +40,7 @@ import com.google.gerrit.server.patch.AddReviewer;
import com.google.gerrit.server.patch.PublishComments;
import com.google.gerrit.server.patch.RemoveReviewer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.PerRequestProjectControlCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeQueryRewriter;
@@ -58,6 +59,7 @@ public class GerritRequestModule extends FactoryModule {
bind(ChangeQueryRewriter.class);
bind(AnonymousUser.class).in(RequestScoped.class);
bind(PerRequestProjectControlCache.class).in(RequestScoped.class);
bind(ChangeControl.Factory.class).in(SINGLETON);
bind(GroupControl.Factory.class).in(SINGLETON);
bind(ProjectControl.Factory.class).in(SINGLETON);

13
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -40,6 +40,8 @@ import com.google.gerrit.server.mail.MergeFailSender;
import com.google.gerrit.server.mail.MergedSender;
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.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.workflow.CategoryFunction;
@@ -137,6 +139,7 @@ public class MergeOp {
private final ApprovalTypes approvalTypes;
private final PatchSetInfoFactory patchSetInfoFactory;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final ChangeControl.GenericFactory changeControlFactory;
private final MergeQueue mergeQueue;
private final PersonIdent myIdent;
@@ -167,6 +170,7 @@ public class MergeOp {
@CanonicalWebUrl @Nullable final Provider<String> cwu,
final ApprovalTypes approvalTypes, final PatchSetInfoFactory psif,
final IdentifiedUser.GenericFactory iuf,
final ChangeControl.GenericFactory changeControlFactory,
@GerritPersonIdent final PersonIdent myIdent,
final MergeQueue mergeQueue, @Assisted final Branch.NameKey branch,
final ChangeHookRunner hooks, final AccountCache accountCache,
@@ -183,6 +187,7 @@ public class MergeOp {
this.approvalTypes = approvalTypes;
patchSetInfoFactory = psif;
identifiedUserFactory = iuf;
this.changeControlFactory = changeControlFactory;
this.mergeQueue = mergeQueue;
this.hooks = hooks;
this.accountCache = accountCache;
@@ -1240,7 +1245,11 @@ public class MergeOp {
c.setStatus(Change.Status.MERGED);
final List<PatchSetApproval> approvals =
schema.patchSetApprovals().byChange(changeId).toList();
final FunctionState fs = functionState.create(c, merged, approvals);
final FunctionState fs = functionState.create(
changeControlFactory.controlFor(
c,
identifiedUserFactory.create(c.getOwner())),
merged, approvals);
for (ApprovalType at : approvalTypes.getApprovalTypes()) {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);
}
@@ -1256,6 +1265,8 @@ public class MergeOp {
a.cache(c);
}
schema.patchSetApprovals().update(approvals);
} catch (NoSuchChangeException err) {
log.warn("Cannot normalize approvals for change " + changeId, err);
} catch (OrmException err) {
log.warn("Cannot normalize approvals for change " + changeId, err);
}

6
gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
View File

@@ -120,7 +120,7 @@ public class PublishComments implements Callable<VoidResult> {
final boolean isCurrent = patchSetId.equals(change.currentPatchSetId());
if (isCurrent && change.getStatus().isOpen()) {
publishApprovals();
publishApprovals(ctl);
} else if (! approvals.isEmpty()) {
throw new InvalidChangeOperationException("Change is closed");
} else {
@@ -141,7 +141,7 @@ public class PublishComments implements Callable<VoidResult> {
db.patchComments().update(drafts);
}
private void publishApprovals() throws OrmException {
private void publishApprovals(ChangeControl ctl) throws OrmException {
ChangeUtil.updated(change);
final Set<ApprovalCategory.Id> dirty = new HashSet<ApprovalCategory.Id>();
@@ -169,7 +169,7 @@ public class PublishComments implements Callable<VoidResult> {
// Normalize all of the items the user is changing.
//
final FunctionState functionState =
functionStateFactory.create(change, patchSetId, all);
functionStateFactory.create(ctl, patchSetId, all);
for (final ApprovalCategoryValue.Id want : approvals) {
final PatchSetApproval a = mine.get(want.getParentKey());
final short o = a.getValue();

52
gerrit-server/src/main/java/com/google/gerrit/server/project/PerRequestProjectControlCache.java Normal file
View File

@@ -0,0 +1,52 @@
// Copyright (C) 2011 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.project;
import com.google.gerrit.reviewdb.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.inject.Inject;
import com.google.inject.servlet.RequestScoped;
import java.util.HashMap;
import java.util.Map;
/** Caches {@link ProjectControl} objects for the current user of the request. */
@RequestScoped
public class PerRequestProjectControlCache {
private final ProjectCache projectCache;
private final CurrentUser user;
private final Map<Project.NameKey, ProjectControl> controls;
@Inject
PerRequestProjectControlCache(ProjectCache projectCache,
CurrentUser userProvider) {
this.projectCache = projectCache;
this.user = userProvider;
this.controls = new HashMap<Project.NameKey, ProjectControl>();
}
ProjectControl get(Project.NameKey nameKey) throws NoSuchProjectException {
ProjectControl ctl = controls.get(nameKey);
if (ctl == null) {
ProjectState p = projectCache.get(nameKey);
if (p == null) {
throw new NoSuchProjectException(nameKey);
}
ctl = p.controlFor(user);
controls.put(nameKey, ctl);
}
return ctl;
}
}

14
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -80,22 +80,16 @@ public class ProjectControl {
}
public static class Factory {
private final ProjectCache projectCache;
private final Provider<CurrentUser> user;
private final Provider<PerRequestProjectControlCache> userCache;
@Inject
Factory(final ProjectCache pc, final Provider<CurrentUser> cu) {
projectCache = pc;
user = cu;
Factory(Provider<PerRequestProjectControlCache> uc) {
userCache = uc;
}
public ProjectControl controlFor(final Project.NameKey nameKey)
throws NoSuchProjectException {
final ProjectState p = projectCache.get(nameKey);
if (p == null) {
throw new NoSuchProjectException(nameKey);
}
return p.controlFor(user.get());
return userCache.get().get(nameKey);
}
public ProjectControl validateFor(final Project.NameKey nameKey)

7
gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
View File

@@ -76,11 +76,4 @@ public abstract class CategoryFunction {
* the valid status into.
*/
public abstract void run(ApprovalType at, FunctionState state);
public boolean isValid(final CurrentUser user, final ApprovalType at,
final FunctionState state) {
return !state.controlFor(user) //
.getRange(Permission.forLabel(at.getCategory().getLabelName())) //
.isEmpty();
}
}

22
gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
View File

@@ -27,10 +27,7 @@ import com.google.gerrit.reviewdb.ApprovalCategory.Id;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -44,7 +41,7 @@ import java.util.Map;
/** State passed through to a {@link CategoryFunction}. */
public class FunctionState {
public interface Factory {
FunctionState create(Change c, PatchSet.Id psId,
FunctionState create(ChangeControl c, PatchSet.Id psId,
Collection<PatchSetApproval> all);
}
@@ -55,20 +52,19 @@ public class FunctionState {
new HashMap<ApprovalCategory.Id, Collection<PatchSetApproval>>();
private final Map<ApprovalCategory.Id, Boolean> valid =
new HashMap<ApprovalCategory.Id, Boolean>();
private final ChangeControl callerChangeControl;
private final Change change;
private final ProjectState project;
@Inject
FunctionState(final ApprovalTypes approvalTypes,
final ProjectCache projectCache,
final IdentifiedUser.GenericFactory userFactory, final GroupCache egc,
@Assisted final Change c, @Assisted final PatchSet.Id psId,
@Assisted final ChangeControl c, @Assisted final PatchSet.Id psId,
@Assisted final Collection<PatchSetApproval> all) {
this.approvalTypes = approvalTypes;
this.userFactory = userFactory;
change = c;
project = projectCache.get(change.getProject());
callerChangeControl = c;
change = c.getChange();
for (final PatchSetApproval ca : all) {
if (psId.equals(ca.getPatchSetId())) {
@@ -147,10 +143,8 @@ public class FunctionState {
a.setValue((short) range.squash(a.getValue()));
}
RefControl controlFor(final CurrentUser user) {
ProjectControl pc = project.controlFor(user);
RefControl rc = pc.controlForRef(change.getDest().get());
return rc;
private ChangeControl controlFor(CurrentUser user) {
return callerChangeControl.forUser(user);
}
/** Run <code>applyTypeFloor</code>, <code>applyRightFloor</code>. */

7
gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoBlock.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.workflow;
import com.google.gerrit.common.data.ApprovalType;
import com.google.gerrit.server.CurrentUser;
/** A function that does nothing. */
public class NoBlock extends CategoryFunction {
@@ -25,10 +24,4 @@ public class NoBlock extends CategoryFunction {
public void run(final ApprovalType at, final FunctionState state) {
state.valid(at, true);
}
@Override
public boolean isValid(final CurrentUser user, final ApprovalType at,
final FunctionState state) {
return true;
}
}

7
gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoOpFunction.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.workflow;
import com.google.gerrit.common.data.ApprovalType;
import com.google.gerrit.server.CurrentUser;
/** A function that does nothing. */
public class NoOpFunction extends CategoryFunction {
@@ -24,10 +23,4 @@ public class NoOpFunction extends CategoryFunction {
@Override
public void run(final ApprovalType at, final FunctionState state) {
}
@Override
public boolean isValid(final CurrentUser user, final ApprovalType at,
final FunctionState state) {
return false;
}
}

2
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
View File

@@ -380,7 +380,7 @@ public class ReviewCommand extends BaseCommand {
new PatchSetApproval(new PatchSetApproval.Key(patchSetId, currentUser
.getAccountId(), ao.getCategoryId()), v);
final FunctionState fs =
functionStateFactory.create(changeControl.getChange(), patchSetId,
functionStateFactory.create(changeControl, patchSetId,
Collections.<PatchSetApproval> emptyList());
psa.setValue(v);
fs.normalize(approvalTypes.byId(psa.getCategoryId()), psa);