Avoid computing file diffs when evalution code owner approvals
To evaluate code owner approvals on a change we need to know which files
have been changed. So far we retrieved the changed files from the diff
cache, which means file diffs are being computed when they are not
cached yet (or when their cache entry has expired). Computing files diff
for changes that touch many files (e.g. > 70k files) can take > 15min.
Since the code owner approval evaluation only needs to know which files
have changed, computing the file diffs seems unnecessary. DiffOperations
has a new method that allows to get the modified files without computing
the file diffs and with this change we are using it to get the changed
files instead of getting them from the diff cache. For the evalution of
code owner approvals detecting renames is important (because renamed
files require an approval on the old and the new path), hence the
rename detection must be enabled.
Bug: Google b/461456634
Change-Id: I45c7e61832be45676ef0ceae5044990cfdf469e0
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 07b42bc..f0723bc 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -32,12 +32,12 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOperationsForCommitValidation;
-import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
import java.util.Map;
import java.util.stream.Stream;
import org.eclipse.jgit.lib.ObjectId;
@@ -91,7 +91,7 @@
requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
try (Timer0.Context ctx = codeOwnerMetrics.getChangedFiles.start()) {
- Map<String, FileDiffOutput> fileDiffOutputs;
+ List<ModifiedFile> modifiedFiles;
if (mergeCommitStrategy.equals(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION)
|| isInitialCommit(project, revision)) {
// Use parentNum=0 to do the comparison against the default base.
@@ -99,9 +99,9 @@
// Initial commits are supported when using parentNum=0.
// For merge commits the default base is the auto-merge commit which should be used as base
// if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION.
- fileDiffOutputs =
- diffOperations.listModifiedFilesAgainstParent(
- project, revision, /* parentNum= */ 0, DiffOptions.DEFAULTS);
+ modifiedFiles =
+ diffOperations.getModifiedFiles(
+ project, revision, /* parentNum= */ 0, /* enableRenameDetection= */ true);
} else {
checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES));
// Always use parent 1 to do the comparison.
@@ -109,13 +109,12 @@
// handled above).
// For merge commits also the first parent should be used if the merge commit strategy is
// ALL_CHANGED_FILES.
- fileDiffOutputs =
- diffOperations.listModifiedFilesAgainstParent(
- project, revision, 1, DiffOptions.DEFAULTS);
+ modifiedFiles =
+ diffOperations.getModifiedFiles(
+ project, revision, /* parentNum= */ 1, /* enableRenameDetection= */ true);
}
- return fileDiffOutputToChangedFiles(
- filterOutMagicFilesFromFileDiffOutputAndSort(fileDiffOutputs))
+ return modifiedFilesToChangedFiles(filterOutMagicFilesFromModifiedFilesAndSort(modifiedFiles))
.collect(toImmutableList());
}
}
@@ -209,7 +208,8 @@
project, revision, 1, /* enableRenameDetection= */ true);
}
- return modifiedFilesToChangedFiles(filterOutMagicFilesFromModifiedFilesAndSort(modifiedFiles))
+ return modifiedFilesToChangedFiles(
+ filterOutMagicFilesFromModifiedFilesAndSort(modifiedFiles.values()))
.collect(toImmutableList());
}
}
@@ -221,27 +221,14 @@
}
}
- private Stream<Map.Entry<String, FileDiffOutput>> filterOutMagicFilesFromFileDiffOutputAndSort(
- Map<String, FileDiffOutput> fileDiffOutputs) {
- return fileDiffOutputs.entrySet().stream()
- .filter(e -> !Patch.isMagic(e.getKey()))
- .sorted(comparing(Map.Entry::getKey));
+ private Stream<ModifiedFile> filterOutMagicFilesFromModifiedFilesAndSort(
+ Collection<ModifiedFile> modifiedFiles) {
+ return modifiedFiles.stream()
+ .filter(modifiedFile -> !Patch.isMagic(modifiedFile.getDefaultPath()))
+ .sorted(comparing(ModifiedFile::getDefaultPath));
}
- private Stream<ChangedFile> fileDiffOutputToChangedFiles(
- Stream<Map.Entry<String, FileDiffOutput>> fileDiffOutputs) {
- return fileDiffOutputs.map(Map.Entry::getValue).map(ChangedFile::create);
- }
-
- private Stream<Map.Entry<String, ModifiedFile>> filterOutMagicFilesFromModifiedFilesAndSort(
- Map<String, ModifiedFile> modifiedFiles) {
- return modifiedFiles.entrySet().stream()
- .filter(e -> !Patch.isMagic(e.getKey()))
- .sorted(comparing(Map.Entry::getKey));
- }
-
- private Stream<ChangedFile> modifiedFilesToChangedFiles(
- Stream<Map.Entry<String, ModifiedFile>> modifiedFiles) {
- return modifiedFiles.map(Map.Entry::getValue).map(ChangedFile::create);
+ private Stream<ChangedFile> modifiedFilesToChangedFiles(Stream<ModifiedFile> modifiedFiles) {
+ return modifiedFiles.map(ChangedFile::create);
}
}
