Avoid computing changed files for automerge commits using PatchListCache
ChangeFiles has 2 methods to compute changed files:
1. computeByComparingAgainstAutoMerge:
This method uses the PatchListCache for computing the changed files,
but PatchListCache can be slow (> 30s). Once the result is cached any
further access is fast.
The primary reason for using PatchListCache was that it takes care of
creating the auto merge commit.
2. computeByComparingAgainstFirstParent:
This method uses DiffFormatter (without rename detection) for
computing the changed files. This seems to be fast also for changes
that touch many files.
This change drops the usage of PatchListCache and uses DiffFormatter
also for case 1. This means that ChangedFiles now needs to take care of
creating the auto merge commit, but it's only a matter of invoking
AutoMerger.
Alternatively we could have used the new file diff cache for case 1, but
we do not because:
- This cache is very new and we do not know for sure that it's always
fast (e.g. it may do some kind of rename detection).
- We cannot use the new file diff cache for case 2 as in this case we
must use the provided RevWalk for loading commits and we cannot pass the
RevWalk into the file diff cache. Hence for case 2 we would need to
continue using DiffFormatter. Then we can also use this existing code
for case 1 and avoid having a differentiation between computing
changed files against first parent and against auto merge.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia5e2afae345e7af5eb5c5c73a068c934c1e685b0
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index b691e21..2f34306 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -14,26 +14,27 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Patch;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.git.InMemoryInserter;
+import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.patch.AutoMerger;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
@@ -42,7 +43,10 @@
import org.eclipse.jgit.diff.RawTextComparator;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.merge.ThreeWayMergeStrategy;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.io.DisabledOutputStream;
@@ -63,19 +67,24 @@
private final GitRepositoryManager repoManager;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
- private final PatchListCache patchListCache;
+ private final Provider<AutoMerger> autoMergerProvider;
private final CodeOwnerMetrics codeOwnerMetrics;
+ private final ThreeWayMergeStrategy mergeStrategy;
+ private final boolean saveAutoMergeCommits;
@Inject
public ChangedFiles(
+ @GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
- PatchListCache patchListCache,
+ Provider<AutoMerger> autoMergerProvider,
CodeOwnerMetrics codeOwnerMetrics) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
- this.patchListCache = patchListCache;
+ this.autoMergerProvider = autoMergerProvider;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
+ this.saveAutoMergeCommits = AutoMerger.cacheAutomerge(cfg);
}
/**
@@ -121,7 +130,7 @@
public ImmutableList<ChangedFile> compute(
Project.NameKey project, Config repoConfig, RevWalk revWalk, RevCommit revCommit)
- throws IOException, PatchListNotAvailableException {
+ throws IOException {
return compute(
project,
repoConfig,
@@ -136,7 +145,7 @@
RevWalk revWalk,
RevCommit revCommit,
MergeCommitStrategy mergeCommitStrategy)
- throws IOException, PatchListNotAvailableException {
+ throws IOException {
requireNonNull(project, "project");
requireNonNull(repoConfig, "repoConfig");
requireNonNull(revWalk, "revWalk");
@@ -148,58 +157,47 @@
if (revCommit.getParentCount() > 1
&& MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
- return computeByComparingAgainstAutoMerge(project, revCommit);
+ RevCommit autoMergeCommit = getAutoMergeCommit(project, revCommit);
+ return compute(repoConfig, revWalk, revCommit, autoMergeCommit);
}
- return computeByComparingAgainstFirstParent(repoConfig, revWalk, revCommit);
+ RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
+ return compute(repoConfig, revWalk, revCommit, baseCommit);
}
- /**
- * Computes the changed files by comparing the given merge commit against the auto merge.
- *
- * <p>Since computing the auto merge is expensive, we do not compute it and diff against it on our
- * own, but rather ask the patch list cache for it.
- *
- * @param project the project that contains the merge commit
- * @param mergeCommit the merge commit for which the changed files should be computed
- * @return the changed files for the given merge commit
- */
- private ImmutableList<ChangedFile> computeByComparingAgainstAutoMerge(
- Project.NameKey project, RevCommit mergeCommit) throws PatchListNotAvailableException {
- checkState(
- mergeCommit.getParentCount() > 1, "expected %s to be a merge commit", mergeCommit.name());
-
- try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFilesAgainstAutoMerge.start()) {
- // for merge commits the default base is the auto merge commit
- PatchListKey patchListKey =
- PatchListKey.againstDefaultBase(mergeCommit, Whitespace.IGNORE_NONE);
-
- return patchListCache.get(patchListKey, project).getPatches().stream()
- .filter(
- patchListEntry ->
- patchListEntry.getNewName() == null
- || !Patch.isMagic(patchListEntry.getNewName()))
- .map(ChangedFile::create)
- .collect(toImmutableList());
+ private RevCommit getAutoMergeCommit(Project.NameKey project, RevCommit mergeCommit)
+ throws IOException {
+ try (Timer0.Context ctx = codeOwnerMetrics.getAutoMerge.start();
+ Repository repository = repoManager.openRepository(project);
+ ObjectInserter inserter =
+ saveAutoMergeCommits
+ ? repository.newObjectInserter()
+ : new InMemoryInserter(repository);
+ ObjectReader reader = inserter.newReader();
+ RevWalk revWalk = new RevWalk(reader)) {
+ return autoMergerProvider
+ .get()
+ .merge(repository, revWalk, inserter, mergeCommit, mergeStrategy);
}
}
/**
- * Computes the changed files by comparing the given commit against its first parent.
+ * Computes the changed files by comparing the given commit against the given base commit.
*
* <p>The computation also works if the commit doesn't have any parent.
*
* @param repoConfig the repository configuration
* @param revWalk the rev walk
- * @param revCommit the commit for which the changed files should be computed
+ * @param commit the commit for which the changed files should be computed
+ * @param baseCommit the base commit against which the given commit should be compared, {@code
+ * null} if the commit doesn't have any parent commit
* @return the changed files for the given commit, sorted alphabetically by path
*/
- private ImmutableList<ChangedFile> computeByComparingAgainstFirstParent(
- Config repoConfig, RevWalk revWalk, RevCommit revCommit) throws IOException {
- try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFilesAgainstFirstParent.start()) {
- RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
- logger.atFine().log("baseCommit = %s", baseCommit != null ? baseCommit.name() : "n/a");
-
+ private ImmutableList<ChangedFile> compute(
+ Config repoConfig, RevWalk revWalk, RevCommit commit, @Nullable RevCommit baseCommit)
+ throws IOException {
+ logger.atFine().log("baseCommit = %s", baseCommit != null ? baseCommit.name() : "n/a");
+ try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFiles.start()) {
// Detecting renames is expensive (since it requires Git to load and compare file contents of
// added and deleted files) and can significantly increase the latency for changes that touch
// large files. To avoid this latency we do not enable the rename detection on the
@@ -208,7 +206,7 @@
try (DiffFormatter diffFormatter = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
diffFormatter.setReader(revWalk.getObjectReader(), repoConfig);
diffFormatter.setDiffComparator(RawTextComparator.DEFAULT);
- List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, revCommit);
+ List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, commit);
ImmutableList<ChangedFile> changedFiles =
diffEntries.stream().map(ChangedFile::create).collect(toImmutableList());
if (changedFiles.size() <= MAX_CHANGED_FILES_TO_LOG) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
index 85933df..c6181b5 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -26,7 +26,6 @@
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.ValidationMessage;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectLevelConfig;
import com.google.gerrit.server.project.ProjectState;
@@ -98,7 +97,7 @@
exceptionMessage(fileName, cfg.getRevision()), validationMessages);
}
return ImmutableList.of();
- } catch (IOException | ConfigInvalidException | PatchListNotAvailableException e) {
+ } catch (IOException | ConfigInvalidException e) {
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
@@ -125,7 +124,7 @@
* @param fileName the name of the file
*/
private boolean isFileChanged(CommitReceivedEvent receiveEvent, String fileName)
- throws IOException, PatchListNotAvailableException {
+ throws IOException {
return changedFiles
.compute(
receiveEvent.project.getNameKey(),
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 22de56d..a04a078 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -30,12 +30,12 @@
public class CodeOwnerMetrics {
// latency metrics
public final Timer0 addChangeMessageOnAddReviewer;
- public final Timer0 computeChangedFilesAgainstAutoMerge;
- public final Timer0 computeChangedFilesAgainstFirstParent;
+ public final Timer0 computeChangedFiles;
public final Timer0 computeFileStatus;
public final Timer0 computeFileStatuses;
public final Timer0 computeOwnedPaths;
public final Timer0 extendChangeMessageOnPostReview;
+ public final Timer0 getAutoMerge;
public final Timer0 prepareFileStatusComputation;
public final Timer0 prepareFileStatusComputationForAccount;
public final Timer0 resolveCodeOwnerConfig;
@@ -68,14 +68,8 @@
"add_change_message_on_add_reviewer",
"Latency for adding a change message with the owned path when a code owner is added as"
+ " a reviewer");
- this.computeChangedFilesAgainstAutoMerge =
- createLatencyTimer(
- "compute_changed_files_against_auto_merge",
- "Latency for computing changed files against auto merge");
- this.computeChangedFilesAgainstFirstParent =
- createLatencyTimer(
- "compute_changed_files_against_first_parent",
- "Latency for computing changed files against first parent");
+ this.computeChangedFiles =
+ createLatencyTimer("compute_changed_files", "Latency for computing changed files");
this.computeFileStatus =
createLatencyTimer(
"compute_file_status", "Latency for computing the file status of one file");
@@ -92,6 +86,9 @@
"extend_change_message_on_post_review",
"Latency for extending the change message with the owned path when a code owner"
+ " approval is applied");
+ this.getAutoMerge =
+ createLatencyTimer(
+ "get_auto_merge", "Latency for getting the auto merge commit of a merge commit");
this.prepareFileStatusComputation =
createLatencyTimer(
"prepare_file_status_computation", "Latency for preparing the file status computation");
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 80eece9..eee9cfb 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -61,7 +61,6 @@
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
@@ -341,11 +340,10 @@
// For merge commits, always do the comparison against the destination branch
// (MergeCommitStrategy.ALL_CHANGED_FILES). Doing the comparison against the auto-merge
- // (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION) is not possible because the auto-merge
- // is loaded via the PatchListCache to which we cannot pass the rev walk which should be used
- // to load the newly created merge commit and hence trying to load it from PatchListCache
- // would fail with a missing object exception. This is why we use
- // MergeCommitStrategy.ALL_CHANGED_FILES here even if
+ // (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION) is not possible because loading the
+ // auto-merge cannot reuse the rev walk that can see newly created merge commits and hence
+ // trying to get the auto merge would fail with a missing object exception. This is why we
+ // use MergeCommitStrategy.ALL_CHANGED_FILES here even if
// MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
ImmutableList<ChangedFile> modifiedCodeOwnerConfigFiles =
changedFiles
@@ -405,7 +403,7 @@
"code-owners plugin configuration is invalid,"
+ " cannot validate code owner config files",
ValidationMessage.Type.WARNING)));
- } catch (IOException | PatchListNotAvailableException e) {
+ } catch (IOException e) {
String errorMessage =
String.format(
"failed to validate code owner config files in revision %s"
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index b4512f2..1c565f0 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -8,10 +8,8 @@
* `add_change_message_on_add_reviewer`:
Latency for adding a change message with the owned path when a code owner is
added as a reviewer.
-* `compute_changed_files_against_auto_merge`:
- Latency for computing changed files against auto merge.
-* `compute_changed_files_against_first_parent`:
- Latency for computing changed files against first parent.
+* `compute_changed_files`:
+ Latency for computing changed files.
* `compute_file_status`:
Latency for computing the file status for one file.
* `compute_file_statuses`:
@@ -23,6 +21,8 @@
* `extend_change_message_on_post_review`:
Latency for extending the change message with the owned path when a code owner
approval is applied.
+* `get_auto_merge`:
+ Latency for getting the auto merge commit of a merge commit.
* `prepare_file_status_computation`:
Latency for preparing the file status computation.
* `prepare_file_status_computation_for_account`:
