opendev/gerrit
Code Issues Proposed changes

ChangeSet: No longer be an AutoValue

Browse Source 
Originally this class was designed as to be an AutoValue such that it
can be hashed and used for identification in the MergeQueue. The MergeQueue
is gone and we do not need to have the hashing in ChangeSet any more.

We rather want to carry the set of changes around, such that we do not
need to reconstruct the changes again and again. So this commit ought to
be a performance improvement.

Change-Id: I8b2a683d6f02e9cef1cd41b23e351f5384a4480d
This commit is contained in:
Stefan Beller 2015-08-24 14:28:39 -07:00 committed by Shawn Pearce
parent 3b2aa8aace
commit 53167f8095
8 changed files with 138 additions and 115 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
View File

@ -19,13 +19,12 @@ import com.google.common.hash.Hashing;
import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.restapi.ETagView;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.MergeSuperSet;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.inject.Inject;
@ -69,11 +68,9 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
rsrc.getChangeResource().prepareETag(h, user);
h.putBoolean(Submit.wholeTopicEnabled(config));
ReviewDb db = dbProvider.get();
ChangeSet cs = mergeSuperSet.completeChangeSet(db,
ChangeSet.create(rsrc.getChange()));
ProjectControl ctl = rsrc.getControl().getProjectControl();
for (Change c : cs.changes()) {
new ChangeResource(ctl.controlFor(c)).prepareETag(h, user);
ChangeSet cs = mergeSuperSet.completeChangeSet(db, rsrc.getChange());
for (ChangeData cd : cs.changes()) {
new ChangeResource(cd.changeControl()).prepareETag(h, user);
}
} catch (IOException | OrmException e) {
throw new OrmRuntimeException(e);

7
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@ -182,11 +182,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
rsrc.getPatchSet().getRevision().get()));
}
ChangeSet submittedChanges = ChangeSet.create(change);
try {
ReviewDb db = dbProvider.get();
mergeOpProvider.get().merge(db, submittedChanges, caller, true);
mergeOpProvider.get().merge(db, change, caller, true);
change = db.changes().get(change.getId());
} catch (NoSuchChangeException e) {
throw new OrmException("Submission failed", e);
@ -306,8 +304,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ChangeSet cs;
try {
cs = mergeSuperSet.completeChangeSet(db,
ChangeSet.create(cd.change()));
cs = mergeSuperSet.completeChangeSet(db, cd.change());
} catch (OrmException | IOException e) {
throw new OrmRuntimeException("Could not determine complete set of " +
"changes to be submitted", e);

4
gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java
View File

@ -60,8 +60,8 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
ResourceConflictException, Exception {
try {
ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(),
ChangeSet.create(resource.getChange()));
if (cs.ids().size() > 1) {
resource.getChange());
if (cs.size() > 1) {
return json.create(EnumSet.of(
ListChangesOption.CURRENT_REVISION,
ListChangesOption.CURRENT_COMMIT))

129
gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
View File

@ -14,70 +14,103 @@
package com.google.gerrit.server.git;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
/** A set of changes grouped together to be submitted atomically.*/
@AutoValue
public abstract class ChangeSet {
public static ChangeSet create(Iterable<Change> changes) {
ImmutableSet.Builder<Project.NameKey> pb = ImmutableSet.builder();
ImmutableSet.Builder<Branch.NameKey> bb = ImmutableSet.builder();
ImmutableSet.Builder<Change.Id> ib = ImmutableSet.builder();
ImmutableSet.Builder<PatchSet.Id> psb = ImmutableSet.builder();
ImmutableSetMultimap.Builder<Project.NameKey, Branch.NameKey> pbb =
ImmutableSetMultimap.builder();
ImmutableSetMultimap.Builder<Project.NameKey, Change.Id> pcb =
ImmutableSetMultimap.builder();
ImmutableSetMultimap.Builder<Branch.NameKey, Change.Id> cbb =
ImmutableSetMultimap.builder();
ImmutableSet.Builder<Change> cb = ImmutableSet.builder();
import java.util.HashSet;
import java.util.Set;
for (Change c : changes) {
Branch.NameKey branch = c.getDest();
Project.NameKey project = branch.getParentKey();
pb.add(project);
bb.add(branch);
ib.add(c.getId());
psb.add(c.currentPatchSetId());
pbb.put(project, branch);
pcb.put(project, c.getId());
cbb.put(branch, c.getId());
cb.add(c);
/**
* A set of changes grouped together to be submitted atomically.
* <p>
* This class is not thread safe.
*/
public class ChangeSet {
private final ImmutableCollection<ChangeData> changeData;
public ChangeSet(Iterable<ChangeData> changes) {
Set<Change.Id> ids = new HashSet<>();
ImmutableSet.Builder<ChangeData> cdb = ImmutableSet.builder();
for (ChangeData cd : changes) {
if (ids.add(cd.getId())) {
cdb.add(cd);
}
}
return new AutoValue_ChangeSet(pb.build(), bb.build(), ib.build(),
psb.build(), pbb.build(), pcb.build(), cbb.build(), cb.build());
changeData = cdb.build();
}
public static ChangeSet create(Change change) {
return create(ImmutableList.of(change));
public ChangeSet(ChangeData change) {
this(ImmutableList.of(change));
}
public abstract ImmutableSet<Project.NameKey> projects();
public abstract ImmutableSet<Branch.NameKey> branches();
public abstract ImmutableSet<Change.Id> ids();
public abstract ImmutableSet<PatchSet.Id> patchIds();
public abstract ImmutableSetMultimap<Project.NameKey, Branch.NameKey>
branchesByProject();
public abstract ImmutableSetMultimap<Project.NameKey, Change.Id>
changesByProject();
public abstract ImmutableSetMultimap<Branch.NameKey, Change.Id>
changesByBranch();
public abstract ImmutableSet<Change> changes();
public ImmutableSet<Change.Id> ids() {
ImmutableSet.Builder<Change.Id> ret = ImmutableSet.builder();
for (ChangeData cd : changeData) {
ret.add(cd.getId());
}
return ret.build();
}
@Override
public int hashCode() {
return ids().hashCode();
public Set<PatchSet.Id> patchIds() throws OrmException {
Set<PatchSet.Id> ret = new HashSet<>();
for (ChangeData cd : changeData) {
ret.add(cd.change().currentPatchSetId());
}
return ret;
}
public SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject()
throws OrmException {
SetMultimap<Project.NameKey, Branch.NameKey> ret =
HashMultimap.create();
for (ChangeData cd : changeData) {
ret.put(cd.change().getProject(), cd.change().getDest());
}
return ret;
}
public Multimap<Project.NameKey, Change.Id> changesByProject()
throws OrmException {
ListMultimap<Project.NameKey, Change.Id> ret =
ArrayListMultimap.create();
for (ChangeData cd : changeData) {
ret.put(cd.change().getProject(), cd.getId());
}
return ret;
}
public Multimap<Branch.NameKey, Change.Id> changesByBranch()
throws OrmException {
ListMultimap<Branch.NameKey, Change.Id> ret =
ArrayListMultimap.create();
for (ChangeData cd : changeData) {
ret.put(cd.change().getDest(), cd.getId());
}
return ret;
}
public ImmutableCollection<ChangeData> changes() {
return changeData;
}
public int size() {
return ids().size();
return changeData.size();
}
@Override
public String toString() {
return getClass().getSimpleName() + ids();
}
}

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

@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Table;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.TimeUtil;
@ -331,14 +332,14 @@ public class MergeOp {
}
}
public void merge(ReviewDb db, ChangeSet changes, IdentifiedUser caller,
public void merge(ReviewDb db, Change change, IdentifiedUser caller,
boolean checkSubmitRules) throws NoSuchChangeException,
OrmException, ResourceConflictException {
logPrefix = String.format("[%s]: ", String.valueOf(changes.hashCode()));
logPrefix = String.format("[%s]: ", String.valueOf(change.hashCode()));
this.db = db;
logDebug("Beginning merge of {}", changes);
logDebug("Beginning integration of {}", change);
try {
ChangeSet cs = mergeSuperSet.completeChangeSet(db, changes);
ChangeSet cs = mergeSuperSet.completeChangeSet(db, change);
logDebug("Calculated to merge {}", cs);
if (checkSubmitRules) {
logDebug("Checking submit rules and state");
@ -361,15 +362,17 @@ public class MergeOp {
logDebug("Beginning merge attempt on {}", cs);
Map<Branch.NameKey, ListMultimap<SubmitType, ChangeData>> toSubmit =
new HashMap<>();
logDebug("Perform the merges");
try {
logDebug("Perform the merges");
for (Project.NameKey project : cs.projects()) {
Multimap<Project.NameKey, Branch.NameKey> br = cs.branchesByProject();
Multimap<Branch.NameKey, Change.Id> cbb = cs.changesByBranch();
for (Project.NameKey project : br.keySet()) {
openRepository(project);
for (Branch.NameKey branch : cs.branchesByProject().get(project)) {
for (Branch.NameKey branch : br.get(project)) {
setDestProject(branch);
List<ChangeData> cds = new ArrayList<>();
for (Change.Id id : cs.changesByBranch().get(branch)) {
for (Change.Id id : cbb.get(branch)) {
cds.add(changeDataFactory.create(db, id));
}
ListMultimap<SubmitType, ChangeData> submitting =
@ -393,9 +396,9 @@ public class MergeOp {
}
logDebug("Write out the new branch tips");
SubmoduleOp subOp = subOpProvider.get();
for (Project.NameKey project : cs.projects()) {
for (Project.NameKey project : br.keySet()) {
openRepository(project);
for (Branch.NameKey branch : cs.branchesByProject().get(project)) {
for (Branch.NameKey branch : br.get(project)) {
RefUpdate update = updateBranch(branch);
pendingRefUpdates.remove(branch);
@ -413,7 +416,8 @@ public class MergeOp {
}
closeRepository();
}
updateSuperProjects(subOp, cs.branches());
updateSuperProjects(subOp, br.values());
checkState(pendingRefUpdates.isEmpty(), "programmer error: "
+ "pending ref update list not emptied");
} catch (NoSuchProjectException noProject) {
@ -906,7 +910,7 @@ public class MergeOp {
}
private void updateSuperProjects(SubmoduleOp subOp,
Set<Branch.NameKey> branches) {
Collection<Branch.NameKey> branches) {
logDebug("Updating superprojects");
try {
subOp.updateSuperProjects(db, branches);

39
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java
View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import com.google.common.base.Strings;
import com.google.common.collect.Multimap;
import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Branch;
@ -80,25 +81,27 @@ public class MergeSuperSet {
this.repoManager = repoManager;
}
public ChangeSet completeChangeSet(ReviewDb db, ChangeSet changes)
public ChangeSet completeChangeSet(ReviewDb db, Change change)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
OrmException {
ChangeData cd = changeDataFactory.create(db, change.getId());
if (Submit.wholeTopicEnabled(cfg)) {
return completeChangeSetIncludingTopics(db, changes);
return completeChangeSetIncludingTopics(db, new ChangeSet(cd));
} else {
return completeChangeSetWithoutTopic(db, changes);
return completeChangeSetWithoutTopic(db, new ChangeSet(cd));
}
}
private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
OrmException {
List<Change> ret = new ArrayList<>();
List<ChangeData> ret = new ArrayList<>();
for (Project.NameKey project : changes.projects()) {
Multimap<Project.NameKey, Change.Id> pc = changes.changesByProject();
for (Project.NameKey project : pc.keySet()) {
try (Repository repo = repoManager.openRepository(project);
RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
for (Change.Id cId : changes.changesByProject().get(project)) {
for (Change.Id cId : pc.get(project)) {
ChangeData cd = changeDataFactory.create(db, cId);
SubmitTypeRecord r = new SubmitRuleEvaluator(cd).getSubmitType();
@ -106,7 +109,7 @@ public class MergeSuperSet {
logErrorAndThrow("Failed to get submit type for " + cd.getId());
}
if (r.type == SubmitType.CHERRY_PICK) {
ret.add(cd.change());
ret.add(cd);
continue;
}
@ -137,17 +140,15 @@ public class MergeSuperSet {
// Merged changes are ok to exclude
Iterable<ChangeData> destChanges = queryProvider.get()
.byCommitsOnBranchNotMerged(cd.change().getDest(), hashes);
for (ChangeData chd : destChanges) {
Change chg = chd.change();
ret.add(chg);
ret.add(chd);
}
}
}
}
}
return ChangeSet.create(ret);
return new ChangeSet(ret);
}
private ChangeSet completeChangeSetIncludingTopics(
@ -157,24 +158,18 @@ public class MergeSuperSet {
boolean done = false;
ChangeSet newCs = completeChangeSetWithoutTopic(db, changes);
while (!done) {
List<Change> chgs = new ArrayList<>();
List<ChangeData> chgs = new ArrayList<>();
done = true;
for (Change.Id cId : newCs.ids()) {
// TODO(sbeller): Cache the change data here and in completeChangeSet
// There is no need to reread it a few times.
ChangeData cd = changeDataFactory.create(db, cId);
chgs.add(cd.change());
for (ChangeData cd : newCs.changes()) {
chgs.add(cd);
String topic = cd.change().getTopic();
if (!Strings.isNullOrEmpty(topic) && !topicsTraversed.contains(topic)) {
for (ChangeData addCd : queryProvider.get().byTopicOpen(topic)) {
chgs.add(addCd.change());
}
chgs.addAll(queryProvider.get().byTopicOpen(topic));
done = false;
topicsTraversed.add(topic);
}
}
changes = ChangeSet.create(chgs);
changes = new ChangeSet(chgs);
newCs = completeChangeSetWithoutTopic(db, changes);
}
return newCs;

33
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -1787,31 +1787,28 @@ public class ReceiveCommits {
throws OrmException, ResourceConflictException {
Submit submit = submitProvider.get();
RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
List<Change> changes = Lists.newArrayList(rsrc.getChange());
try {
mergeOpProvider.get().merge(db, ChangeSet.create(changes),
mergeOpProvider.get().merge(db, rsrc.getChange(),
(IdentifiedUser) changeCtl.getCurrentUser(), false);
} catch (NoSuchChangeException e) {
throw new OrmException(e);
}
addMessage("");
for (Change c : changes) {
c = db.changes().get(c.getId());
switch (c.getStatus()) {
case MERGED:
addMessage("Change " + c.getChangeId() + " merged.");
Change c = db.changes().get(rsrc.getChange().getId());
switch (c.getStatus()) {
case MERGED:
addMessage("Change " + c.getChangeId() + " merged.");
break;
case NEW:
ChangeMessage msg = submit.getConflictMessage(rsrc);
if (msg != null) {
addMessage("Change " + c.getChangeId() + ": " + msg.getMessage());
break;
case NEW:
ChangeMessage msg = submit.getConflictMessage(rsrc);
if (msg != null) {
addMessage("Change " + c.getChangeId() + ": " + msg.getMessage());
break;
}
//$FALL-THROUGH$
default:
addMessage("change " + c.getChangeId() + " is "
+ c.getStatus().name().toLowerCase());
}
}
//$FALL-THROUGH$
default:
addMessage("change " + c.getChangeId() + " is "
+ c.getStatus().name().toLowerCase());
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
View File

@ -180,7 +180,7 @@ public class SubmoduleOp {
}
protected void updateSuperProjects(ReviewDb db,
Set<Branch.NameKey> updatedBranches) throws SubmoduleException {
Collection<Branch.NameKey> updatedBranches) throws SubmoduleException {
try {
// These (repo/branch) will be updated later with all the given
// individual submodule subscriptions