Optimize sticky approval check for large changes When evaluating sticky approvals, CodeOwnerApprovalCheck was performing a sequential scan over all changed files of previously approved patch sets for every file in the current patch set. This resulted in O(N*M) complexity. For changes with a large number of files (e.g., 100k+), this caused extreme latency and timeouts. Optimize this by caching the touched paths of previous patch sets in a Set for O(1) lookup, reducing the complexity to O(N+M). Release-Notes: skip Change-Id: I142d288606c8b5f63fe55caef64a80418bdde6c5
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java index 511a90c..362946e 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
@@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot; @@ -26,6 +27,7 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.io.IOException; +import java.nio.file.Path; import java.util.HashMap; import java.util.Map; @@ -50,6 +52,7 @@ private final ChangeNotes changeNotes; private Map<PatchSet.Id, ImmutableList<ChangedFile>> cache = new HashMap<>(); + private Map<PatchSet.Id, ImmutableSet<Path>> pathsCache = new HashMap<>(); @Inject public ChangedFilesByPatchSetCache( @@ -65,6 +68,20 @@ return cache.computeIfAbsent(patchSetId, this::compute); } + public ImmutableSet<Path> getPaths(PatchSet.Id patchSetId) { + return pathsCache.computeIfAbsent(patchSetId, this::computePaths); + } + + private ImmutableSet<Path> computePaths(PatchSet.Id patchSetId) { + ImmutableList<ChangedFile> files = get(patchSetId); + ImmutableSet.Builder<Path> builder = ImmutableSet.builder(); + for (ChangedFile file : files) { + file.newPath().ifPresent(builder::add); + file.oldPath().ifPresent(builder::add); + } + return builder.build(); + } + private ImmutableList<ChangedFile> compute(PatchSet.Id patchSetId) { checkState( patchSetId.changeId().equals(changeNotes.getChange().getId()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index c7d0cb5..473d279 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -929,10 +929,7 @@ for (PatchSet.Id patchSetId : input.previouslyApprovedPatchSetsInReverseOrder()) { // changedFilesByPatchSetCache doesn't detect renames. That's fine since we only check whether // the path has been code-owner approved in a previous patch set. - if (changedFilesByPatchSetCache.get(patchSetId).stream() - .anyMatch( - changedFile -> - changedFile.hasNewPath(absolutePath) || changedFile.hasOldPath(absolutePath))) { + if (changedFilesByPatchSetCache.getPaths(patchSetId).contains(absolutePath)) { logger.atFine().log( "previously approved patch set %d contains path %s", patchSetId.get(), absolutePath); Optional<Account.Id> approver =