Don't aggressively flush objects when merging
This is a long story. We originally tried this about a year ago with
Ia85c5114, but that was doomed (reverted in Id691318a) as we couldn't
guarantee PatchSet objects wouldn't be written before the inserter was
flushed. That experience directly led to the introduction of
BatchUpdate in I0771f5e8, which finally culminated in converting
MergeOp to BatchUpdate in I1d9883a5[1].
Now, we can close the door on this chapter and finally avoid flushing
each new commit as soon as it's created. It is now easy to prove that
no PatchSets are written pointing to new SHA-1s before they are
flushed, since commits are created in updateRepo and patch sets are
updated in updateChange.
The lone exception is in CherryPickChange, where the BatchUpdate
creation can't easily be hoisted higher, because we have to parse
objects from the repo in order to determine which Change.Id the
operation applies to. But here we can ensure flushing happens by
passing the same ObjectInserter to any BatchUpdates.
This should substantially improve performance of all merge strategies
that create one commit per submitted change (MergeAlways, CherryPick,
RebaseIfNecessary), particularly on slow backends.
[1] The astute reader will notice I have also justified the existence
of BatchUpdate as making the notedb transition easier by being one
place to create ChangeUpdates, enforce ordering, eventually have
retry logic, etc. That is all true, but it turns out it's not the
original motivation.
Change-Id: Icba6262383088fe24b81ec7159511d80ae31724e
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
index b3659798..50efbb6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
@@ -132,7 +132,12 @@
String destinationBranch = RefNames.shortName(ref);
IdentifiedUser identifiedUser = user.get();
try (Repository git = gitManager.openRepository(project);
- CodeReviewRevWalk revWalk = CodeReviewCommit.newRevWalk(git)) {
+ // This inserter and revwalk *must* be passed to any BatchUpdates
+ // created later on, to ensure the cherry-picked commit is flushed
+ // before patch sets are updated.
+ ObjectInserter oi = git.newObjectInserter();
+ CodeReviewRevWalk revWalk =
+ CodeReviewCommit.newRevWalk(oi.newReader())) {
Ref destRef = git.getRefDatabase().exactRef(ref);
if (destRef == null) {
throw new InvalidChangeOperationException(String.format(
@@ -156,7 +161,7 @@
ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n';
CodeReviewCommit cherryPickCommit;
- try (ObjectInserter oi = git.newObjectInserter()) {
+ try {
ProjectState projectState = refControl.getProjectControl().getProjectState();
cherryPickCommit =
mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index aa2f9de..eefba9d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -77,6 +77,7 @@
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
@@ -126,19 +127,21 @@
ProjectState project;
BatchUpdate update;
+ private final ObjectReader reader;
private final Map<Branch.NameKey, OpenBranch> branches;
OpenRepo(Repository repo, ProjectState project) {
this.repo = repo;
this.project = project;
- rw = CodeReviewCommit.newRevWalk(repo);
+ ins = repo.newObjectInserter();
+ reader = ins.newReader();
+ rw = CodeReviewCommit.newRevWalk(reader);
rw.sort(RevSort.TOPO);
rw.sort(RevSort.COMMIT_TIME_DESC, true);
rw.setRetainBody(false);
canMergeFlag = rw.newFlag("CAN_MERGE");
rw.retainOnReset(canMergeFlag);
- ins = repo.newObjectInserter();
branches = Maps.newHashMapWithExpectedSize(1);
}
@@ -167,8 +170,9 @@
if (update != null) {
update.close();
}
- ins.close();
rw.close();
+ reader.close();
+ ins.close();
repo.close();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
index df307e7..586fed9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
@@ -74,7 +74,6 @@
import java.io.IOException;
import java.io.InputStream;
-import java.io.UnsupportedEncodingException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -83,6 +82,15 @@
import java.util.List;
import java.util.Set;
+/**
+ * Utility methods used during the merge process.
+ * <p>
+ * <strong>Note:</strong> Unless otherwise specified, the methods in this class
+ * <strong>do not</strong> flush {@link ObjectInserter}s. Callers that want to
+ * read back objects before flushing should use {@link
+ * ObjectInserter#newReader()}. This is already the default behavior of {@code
+ * BatchUpdate}.
+ */
public class MergeUtil {
private static final Logger log = LoggerFactory.getLogger(MergeUtil.class);
private static final String R_HEADS_MASTER =
@@ -191,7 +199,7 @@
mergeCommit.setAuthor(originalCommit.getAuthorIdent());
mergeCommit.setCommitter(cherryPickCommitterIdent);
mergeCommit.setMessage(commitMsg);
- return rw.parseCommit(commit(inserter, mergeCommit));
+ return rw.parseCommit(inserter.insert(mergeCommit));
} else {
throw new MergeConflictException("merge conflict");
}
@@ -540,7 +548,7 @@
mergeCommit.setMessage(msgbuf.toString());
CodeReviewCommit mergeResult =
- rw.parseCommit(commit(inserter, mergeCommit));
+ rw.parseCommit(inserter.insert(mergeCommit));
mergeResult.setControl(n.getControl());
return mergeResult;
}
@@ -631,14 +639,6 @@
return (ThreeWayMerger) m;
}
- public ObjectId commit(final ObjectInserter inserter,
- final CommitBuilder mergeCommit) throws IOException,
- UnsupportedEncodingException {
- ObjectId id = inserter.insert(mergeCommit);
- inserter.flush();
- return id;
- }
-
public void markCleanMerges(final RevWalk rw,
final RevFlag canMergeFlag, final CodeReviewCommit mergeTip,
final Set<RevCommit> alreadyAccepted) throws IntegrationException {
