From 4801bf52d60161953b2d0e28c1b51262153d74c0 Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Mon, 14 Jun 2021 20:12:04 -0700 Subject: [PATCH] Improve performance of artifact conflict detection. * Use the action count to presize data structures that grow with the number of outputs. * Short-circuit output registration when detecting that actions can be shared - there is no need to check for each output, since shareable actions must have identical outputs. * Instead of using `ConcurrentSkipListMap` to order and deduplicate artifacts, aggregate all artifacts in a list and use `Arrays.parallelSort` followed by a length check to skip calling `startsWith` on equal exec paths. PiperOrigin-RevId: 379403122 --- .../devtools/build/lib/actions/Actions.java | 202 +++++++----------- .../lib/actions/MapBasedActionGraph.java | 22 +- .../ActionTemplateExpansionFunction.java | 10 +- .../lib/skyframe/ArtifactConflictFinder.java | 85 ++++---- .../build/lib/skyframe/SkyframeBuildView.java | 5 +- .../build/lib/skyframe/SkyframeExecutor.java | 4 + 6 files changed, 147 insertions(+), 181 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index 10b1d19959570e..1055946dfc4962 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java @@ -19,6 +19,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterators; import com.google.common.escape.Escaper; import com.google.common.escape.Escapers; import com.google.common.flogger.GoogleLogger; @@ -28,6 +29,7 @@ import com.google.devtools.build.lib.skyframe.SkyframeAwareAction; import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.HashMap; @@ -35,7 +37,6 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Optional; -import java.util.SortedMap; import javax.annotation.Nullable; /** Utility class for actions. */ @@ -83,36 +84,17 @@ public static boolean dependsOnBuildId(ActionAnalysisMetadata action) { public static boolean canBeShared( ActionKeyContext actionKeyContext, ActionAnalysisMetadata a, ActionAnalysisMetadata b) throws InterruptedException { - if (!a.isShareable() || !b.isShareable()) { - return false; - } - - if (!a.getMnemonic().equals(b.getMnemonic())) { - return false; - } - - // Non-Actions cannot be shared. - if (!(a instanceof Action) || !(b instanceof Action)) { - return false; - } - - Action actionA = (Action) a; - Action actionB = (Action) b; - if (!actionA - .getKey(actionKeyContext, /*artifactExpander=*/ null) - .equals(actionB.getKey(actionKeyContext, /*artifactExpander=*/ null))) { - return false; - } - // Don't bother to check input and output counts first; the expected result for these tests is - // to always be true (i.e., that this method returns true). - if (!artifactsEqualWithoutOwner( - actionA.getMandatoryInputs().toList(), actionB.getMandatoryInputs().toList())) { - return false; - } - if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) { - return false; - } - return true; + return a.isShareable() + && b.isShareable() + && a.getMnemonic().equals(b.getMnemonic()) + // Non-Actions cannot be shared. + && a instanceof Action + && b instanceof Action + && a.getKey(actionKeyContext, /*artifactExpander=*/ null) + .equals(b.getKey(actionKeyContext, /*artifactExpander=*/ null)) + && artifactsEqualWithoutOwner( + a.getMandatoryInputs().toList(), b.getMandatoryInputs().toList()) + && artifactsEqualWithoutOwner(a.getOutputs(), b.getOutputs()); } /** @@ -128,45 +110,36 @@ static boolean canBeSharedLogForPotentialFalsePositives( ActionAnalysisMetadata actionA, ActionAnalysisMetadata actionB) throws InterruptedException { - boolean canBeShared = canBeShared(actionKeyContext, actionA, actionB); - if (canBeShared) { - Optional treeArtifactInput = - actionA.getMandatoryInputs().toList().stream() - .filter(Artifact::isTreeArtifact) - .findFirst(); - treeArtifactInput.ifPresent( - treeArtifact -> - logger.atInfo().atMostEvery(5, MINUTES).log( - "Shared action: %s has a tree artifact input: %s -- shared actions" - + " detection is overly permissive in this case and may allow" - + " sharing of different actions", - actionA, treeArtifact)); + if (!canBeShared(actionKeyContext, actionA, actionB)) { + return false; } - return canBeShared; + Optional treeArtifactInput = + actionA.getMandatoryInputs().toList().stream().filter(Artifact::isTreeArtifact).findFirst(); + treeArtifactInput.ifPresent( + treeArtifact -> + logger.atInfo().atMostEvery(5, MINUTES).log( + "Shared action: %s has a tree artifact input: %s -- shared actions" + + " detection is overly permissive in this case and may allow" + + " sharing of different actions", + actionA, treeArtifact)); + return true; } private static boolean artifactsEqualWithoutOwner( - Iterable iterable1, Iterable iterable2) { - if (iterable1 instanceof Collection && iterable2 instanceof Collection) { - Collection collection1 = (Collection) iterable1; - Collection collection2 = (Collection) iterable2; - if (collection1.size() != collection2.size()) { - return false; - } + Collection collection1, Collection collection2) { + if (collection1.size() != collection2.size()) { + return false; } - Iterator iterator1 = iterable1.iterator(); - Iterator iterator2 = iterable2.iterator(); + Iterator iterator1 = collection1.iterator(); + Iterator iterator2 = collection2.iterator(); while (iterator1.hasNext()) { - if (!iterator2.hasNext()) { - return false; - } Artifact artifact1 = iterator1.next(); Artifact artifact2 = iterator2.next(); if (!artifact1.equalsWithoutOwner(artifact2)) { return false; } } - return !iterator2.hasNext(); + return true; } /** @@ -341,96 +314,78 @@ private static GeneratingActions assignOwnersAndMaybeFilterSharedActionsAndThrow artifactsByOutputLabel != null ? artifactsByOutputLabel.build() : ImmutableMap.of()); } - /** - * Returns a comparator for use with {@link #findArtifactPrefixConflicts(ActionGraph, SortedMap, - * boolean)}. - */ - public static Comparator comparatorForPrefixConflicts() { - return PathFragmentPrefixComparator.INSTANCE; - } - - private static class PathFragmentPrefixComparator implements Comparator { - private static final PathFragmentPrefixComparator INSTANCE = new PathFragmentPrefixComparator(); - - @Override - public int compare(PathFragment lhs, PathFragment rhs) { - // We need to use the OS path policy in case the OS is case insensitive. - OsPathPolicy os = OsPathPolicy.getFilePathOs(); - String str1 = lhs.getPathString(); - String str2 = rhs.getPathString(); - int len1 = str1.length(); - int len2 = str2.length(); - int n = Math.min(len1, len2); - for (int i = 0; i < n; ++i) { - char c1 = str1.charAt(i); - char c2 = str2.charAt(i); - int res = os.compare(c1, c2); - if (res != 0) { - if (c1 == PathFragment.SEPARATOR_CHAR) { - return -1; - } else if (c2 == PathFragment.SEPARATOR_CHAR) { - return 1; + private static final Comparator EXEC_PATH_PREFIX_COMPARATOR = + (lhs, rhs) -> { + // We need to use the OS path policy in case the OS is case insensitive. + OsPathPolicy os = OsPathPolicy.getFilePathOs(); + String str1 = lhs.getExecPathString(); + String str2 = rhs.getExecPathString(); + int len1 = str1.length(); + int len2 = str2.length(); + int n = Math.min(len1, len2); + for (int i = 0; i < n; ++i) { + char c1 = str1.charAt(i); + char c2 = str2.charAt(i); + int res = os.compare(c1, c2); + if (res != 0) { + if (c1 == PathFragment.SEPARATOR_CHAR) { + return -1; + } else if (c2 == PathFragment.SEPARATOR_CHAR) { + return 1; + } + return res; } - return res; } - } - return len1 - len2; - } - } + return len1 - len2; + }; /** * Finds Artifact prefix conflicts between generated artifacts. An artifact prefix conflict - * happens if one action generates an artifact whose path is a prefix of another artifact's path. - * Those two artifacts cannot exist simultaneously in the output tree. + * happens if one action generates an artifact whose path is a strict prefix of another artifact's + * path. Those two artifacts cannot exist simultaneously in the output tree. * * @param actionGraph the {@link ActionGraph} to query for artifact conflicts - * @param artifactPathMap a map mapping generated artifacts to their exec paths. The map must be - * sorted using the comparator from {@link #comparatorForPrefixConflicts()}. - * @param strictConflictChecks report path prefix conflicts, regardless of - * shouldReportPathPrefixConflict(). + * @param artifacts all generated artifacts in the build + * @param strictConflictChecks report path prefix conflicts, regardless of {@link + * ActionAnalysisMetadata#shouldReportPathPrefixConflict} * @return An immutable map between actions that generated the conflicting artifacts and their - * associated {@link ArtifactPrefixConflictException}. + * associated {@link ArtifactPrefixConflictException} */ public static ImmutableMap findArtifactPrefixConflicts( - ActionGraph actionGraph, - SortedMap artifactPathMap, - boolean strictConflictChecks) { - // You must construct the sorted map using this comparator for the algorithm to work. - // The algorithm requires subdirectories to immediately follow parent directories, - // before any files in that directory. - // Example: "foo", "foo.obj", foo/bar" must be sorted - // "foo", "foo/bar", foo.obj" - Preconditions.checkArgument( - artifactPathMap.comparator() instanceof PathFragmentPrefixComparator, - "artifactPathMap must be sorted with PathFragmentPrefixComparator"); + ActionGraph actionGraph, Collection artifacts, boolean strictConflictChecks) { // No actions in graph -- currently happens only in tests. Special-cased because .next() call // below is unconditional. - if (artifactPathMap.isEmpty()) { - return ImmutableMap.of(); + if (artifacts.isEmpty()) { + return ImmutableMap.of(); } + Artifact[] artifactArray = artifacts.toArray(new Artifact[0]); + Arrays.parallelSort(artifactArray, EXEC_PATH_PREFIX_COMPARATOR); + // Keep deterministic ordering of bad actions. Map badActions = new LinkedHashMap<>(); - Iterator iter = artifactPathMap.keySet().iterator(); + Iterator iter = Iterators.forArray(artifactArray); - // Report an error for every derived artifact which is a prefix of another. + // Report an error for every derived artifact which is a strict prefix of another. // If x << y << z (where x << y means "y starts with x"), then we only report (x,y), (x,z), but // not (y,z). - for (PathFragment pathJ = iter.next(); iter.hasNext(); ) { + for (Artifact artifactJ = iter.next(); iter.hasNext(); ) { // For each comparison, we have a prefix candidate (pathI) and a suffix candidate (pathJ). // At the beginning of the loop, we set pathI to the last suffix candidate, since it has not // yet been tested as a prefix candidate, and then set pathJ to the paths coming after pathI, // until we come to one that does not contain pathI as a prefix. pathI is then verified not to // be the prefix of any path, so we start the next run of the loop. - PathFragment pathI = pathJ; + Artifact artifactI = artifactJ; + PathFragment pathI = artifactI.getExecPath(); // Compare pathI to the paths coming after it. while (iter.hasNext()) { - pathJ = iter.next(); - if (pathJ.startsWith(pathI)) { // prefix conflict. - Artifact artifactI = Preconditions.checkNotNull(artifactPathMap.get(pathI), pathI); - Artifact artifactJ = Preconditions.checkNotNull(artifactPathMap.get(pathJ), pathJ); - + artifactJ = iter.next(); + PathFragment pathJ = artifactJ.getExecPath(); + // Check length first so that we only detect strict prefix conflicts. Equal exec paths are + // possible from shared actions. + if (pathJ.getPathString().length() > pathI.getPathString().length() + && pathJ.startsWith(pathI)) { // TODO(b/159733792): Test this check with compressed tree artifact input. // We ignore the artifact prefix conflict between a TreeFileArtifact and its parent // TreeArtifact. @@ -449,8 +404,9 @@ public int compare(PathFragment lhs, PathFragment rhs) { ActionAnalysisMetadata actionJ = Preconditions.checkNotNull(actionGraph.getGeneratingAction(artifactJ), artifactJ); if (strictConflictChecks || actionI.shouldReportPathPrefixConflict(actionJ)) { - ArtifactPrefixConflictException exception = new ArtifactPrefixConflictException(pathI, - pathJ, actionI.getOwner().getLabel(), actionJ.getOwner().getLabel()); + ArtifactPrefixConflictException exception = + new ArtifactPrefixConflictException( + pathI, pathJ, actionI.getOwner().getLabel(), actionJ.getOwner().getLabel()); badActions.put(actionI, exception); badActions.put(actionJ, exception); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java index 0217e33232d9e9..c23ee08688a3c4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java +++ b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java @@ -25,11 +25,15 @@ public final class MapBasedActionGraph implements MutableActionGraph { private final ActionKeyContext actionKeyContext; - private final ConcurrentMap - generatingActionMap = new ConcurrentHashMap<>(); + private final ConcurrentMap generatingActionMap; public MapBasedActionGraph(ActionKeyContext actionKeyContext) { + this(actionKeyContext, /*sizeHint=*/ 16); + } + + public MapBasedActionGraph(ActionKeyContext actionKeyContext, int sizeHint) { this.actionKeyContext = actionKeyContext; + this.generatingActionMap = new ConcurrentHashMap<>(sizeHint); } @Override @@ -42,13 +46,13 @@ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { public void registerAction(ActionAnalysisMetadata action) throws ActionConflictException, InterruptedException { for (Artifact artifact : action.getOutputs()) { - OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact); - ActionAnalysisMetadata previousAction = generatingActionMap.putIfAbsent(wrapper, action); - if (previousAction != null - && previousAction != action - && !Actions.canBeSharedLogForPotentialFalsePositives( - actionKeyContext, action, previousAction)) { - generatingActionMap.remove(wrapper, action); + ActionAnalysisMetadata previousAction = + generatingActionMap.putIfAbsent(new OwnerlessArtifactWrapper(artifact), action); + if (previousAction != null && previousAction != action) { + if (Actions.canBeSharedLogForPotentialFalsePositives( + actionKeyContext, action, previousAction)) { + return; // All outputs can be shared. No need to register the remaining outputs. + } throw new ActionConflictException(actionKeyContext, artifact, previousAction, action); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java index f5639bf24f8041..97b0d8345cc6a5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; @@ -43,7 +42,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.TreeMap; import javax.annotation.Nullable; /** @@ -198,15 +196,9 @@ private static ImmutableMap getMapForConsisten */ private static Map findArtifactPrefixConflicts(Map generatingActions) { - TreeMap artifactPathMap = - new TreeMap<>(Actions.comparatorForPrefixConflicts()); - for (Artifact artifact : generatingActions.keySet()) { - artifactPathMap.put(artifact.getExecPath(), artifact); - } - return Actions.findArtifactPrefixConflicts( new MapBasedImmutableActionGraph(generatingActions), - artifactPathMap, + generatingActions.keySet(), /*strictConflictChecks=*/ true); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java index 611eae52917155..a14ff19f8b2726 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java @@ -33,12 +33,12 @@ import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.ConcurrentNavigableMap; -import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import javax.annotation.Nullable; @@ -64,19 +64,25 @@ private ArtifactConflictFinder() {} */ static ActionConflictsAndStats findAndStoreArtifactConflicts( Sharder actionLookupValues, + int actionCount, boolean strictConflictChecks, ActionKeyContext actionKeyContext) throws InterruptedException { ConcurrentMap temporaryBadActionMap = new ConcurrentHashMap<>(); - MapBasedActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext); - ConcurrentNavigableMap artifactPathMap = - new ConcurrentSkipListMap<>(Actions.comparatorForPrefixConflicts()); - constructActionGraphAndPathMap( - actionGraph, artifactPathMap, actionLookupValues, temporaryBadActionMap); + + // Use the action count to presize - all actions have at least one output artifact. + MapBasedActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext, actionCount); + List artifacts = new ArrayList<>(actionCount); + + constructActionGraphAndArtifactList( + actionGraph, + Collections.synchronizedList(artifacts), + actionLookupValues, + temporaryBadActionMap); Map actionsWithArtifactPrefixConflict = - Actions.findArtifactPrefixConflicts(actionGraph, artifactPathMap, strictConflictChecks); + Actions.findArtifactPrefixConflicts(actionGraph, artifacts, strictConflictChecks); for (Map.Entry actionExceptionPair : actionsWithArtifactPrefixConflict.entrySet()) { temporaryBadActionMap.put( @@ -92,9 +98,9 @@ static ActionConflictsAndStats findAndStoreArtifactConflicts( * PathFragment}s to their respective {@link Artifact}s. We do this in a threadpool to save around * 1.5 seconds on a mid-sized build versus a single-threaded operation. */ - private static void constructActionGraphAndPathMap( + private static void constructActionGraphAndArtifactList( MutableActionGraph actionGraph, - ConcurrentNavigableMap artifactPathMap, + List artifacts, Sharder actionShards, ConcurrentMap badActionMap) throws InterruptedException { @@ -108,7 +114,7 @@ private static void constructActionGraphAndPathMap( new ThreadFactoryBuilder().setNameFormat("ActionLookupValue Processor %d").build()); for (List shard : actionShards) { executor.execute( - wrapper.wrap(actionRegistration(shard, actionGraph, artifactPathMap, badActionMap))); + wrapper.wrap(() -> actionRegistration(shard, actionGraph, artifacts, badActionMap))); } boolean interrupted = ExecutorUtil.interruptibleShutdown(executor); Throwable firstThrownError = wrapper.getFirstThrownError(); @@ -121,36 +127,37 @@ private static void constructActionGraphAndPathMap( } } - private static Runnable actionRegistration( - final List values, - final MutableActionGraph actionGraph, - final ConcurrentMap artifactPathMap, - final ConcurrentMap badActionMap) { - return () -> { - for (ActionLookupValue value : values) { - for (ActionAnalysisMetadata action : value.getActions()) { - try { - actionGraph.registerAction(action); - } catch (ActionConflictException e) { - // It may be possible that we detect a conflict for the same action more than once, if - // that action belongs to multiple aspect values. In this case we will harmlessly - // overwrite the badActionMap entry. - badActionMap.put(action, new ConflictException(e)); - // We skip the rest of the loop, and do not add the path->artifact mapping for this - // artifact below -- we don't need to check it since this action is already in - // error. - continue; - } catch (InterruptedException e) { - // Bail. - Thread.currentThread().interrupt(); - return; - } - for (Artifact output : action.getOutputs()) { - artifactPathMap.put(output.getExecPath(), output); - } + private static void actionRegistration( + List values, + MutableActionGraph actionGraph, + List allArtifacts, + ConcurrentMap badActionMap) { + // Accumulated and added to the shared list at the end to reduce contention. + List myArtifacts = new ArrayList<>(values.size()); + + for (ActionLookupValue value : values) { + for (ActionAnalysisMetadata action : value.getActions()) { + try { + actionGraph.registerAction(action); + } catch (ActionConflictException e) { + // It may be possible that we detect a conflict for the same action more than once, if + // that action belongs to multiple aspect values. In this case we will harmlessly + // overwrite the badActionMap entry. + badActionMap.put(action, new ConflictException(e)); + // We skip the rest of the loop, and do not add the path->artifact mapping for this + // artifact below -- we don't need to check it since this action is already in + // error. + continue; + } catch (InterruptedException e) { + // Bail. + Thread.currentThread().interrupt(); + return; } + myArtifacts.addAll(action.getOutputs()); } - }; + } + + allArtifacts.addAll(myArtifacts); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index af7665c99b185e..7b4007d6054781 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -454,7 +454,10 @@ public SkyframeAnalysisResult configureTargets( skyframeExecutor.getActionLookupValuesInBuild(ctKeys, aspectKeys); ArtifactConflictFinder.ActionConflictsAndStats conflictsAndStats = ArtifactConflictFinder.findAndStoreArtifactConflicts( - analysisTraversalResult.getActionShards(), strictConflictChecks, actionKeyContext); + analysisTraversalResult.getActionShards(), + analysisTraversalResult.getActionCount(), + strictConflictChecks, + actionKeyContext); BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics buildGraphMetrics = analysisTraversalResult .getMetrics() diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index bf141061d647b2..83c7596542bdea 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -3126,6 +3126,10 @@ Sharder getActionShards() { return actionShards; } + int getActionCount() { + return actionCount; + } + BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics.Builder getMetrics() { return BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics.newBuilder() .setActionLookupValueCount(configuredObjectCount)