SkyframeBuildView: move action conflict checking Bazel will now delay checking for action conflicts until it has collected "good" Aspects and CTs and has created the PackageRoots, because doing so will let us move the collection and creation logic into a SkyFunction and so reduce code outside of Skyframe between Analysis and Execution, and thereby allow us to maybe merge the two phases some time in the future. I believe the collection and creation logic does not actually depend on the result of action conflict checking, making this change safe. I believe this because the collection logic does not actually use the 'badActions' map, and while it looks up SkyValues from the EvaluationResult, those are supposed to be immutable, so the conflict checker could not be storing anything in them, therefore the collection logic cannot possibly be filtering CTs with bad actions. Therefore "good" in the variable names "goodCts" and "goodAspects" is misleading, and also the comment that claimed the "goodCts" collection logic was filtering out CTs with bad actions was wrong, so I renamed these variables and I removed the comment. I also fixed a minor bug: the 'aspects' list (previously: 'goodAspects') was initialized with an expected size of 'ctKeys.size()', now it is initialized with the 'aspectKeys.size()'. PiperOrigin-RevId: 288687661
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 6d33809..b87a2b1 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
@@ -368,7 +368,7 @@ */ public SkyframeAnalysisResult configureTargets( ExtendedEventHandler eventHandler, - List<ConfiguredTargetKey> values, + List<ConfiguredTargetKey> ctKeys, List<AspectValueKey> aspectKeys, Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier, EventBus eventBus, @@ -380,15 +380,48 @@ try (SilentCloseable c = Profiler.instance().profile("skyframeExecutor.configureTargets")) { result = skyframeExecutor.configureTargets( - eventHandler, values, aspectKeys, keepGoing, numThreads); + eventHandler, ctKeys, aspectKeys, keepGoing, numThreads); } finally { enableAnalysis(false); } + + Collection<AspectValue> aspects = Lists.newArrayListWithCapacity(aspectKeys.size()); + Root singleSourceRoot = skyframeExecutor.getForcedSingleSourceRootIfNoExecrootSymlinkCreation(); + NestedSetBuilder<Package> packages = + singleSourceRoot == null ? NestedSetBuilder.stableOrder() : null; + for (AspectValueKey aspectKey : aspectKeys) { + AspectValue value = (AspectValue) result.get(aspectKey); + if (value == null) { + // Skip aspects that couldn't be applied to targets. + continue; + } + aspects.add(value); + if (packages != null) { + packages.addTransitive(value.getTransitivePackagesForPackageRootResolution()); + } + } + + Collection<ConfiguredTarget> cts = Lists.newArrayListWithCapacity(ctKeys.size()); + for (ConfiguredTargetKey value : ctKeys) { + ConfiguredTargetValue ctValue = (ConfiguredTargetValue) result.get(value); + if (ctValue == null) { + continue; + } + cts.add(ctValue.getConfiguredTarget()); + if (packages != null) { + packages.addTransitive(ctValue.getTransitivePackagesForPackageRootResolution()); + } + } + PackageRoots packageRoots = + singleSourceRoot == null + ? new MapAsPackageRoots(collectPackageRoots(packages.build().toList())) + : new PackageRootsNoSymlinkCreation(singleSourceRoot); + try (SilentCloseable c = Profiler.instance().profile("skyframeExecutor.findArtifactConflicts")) { ImmutableSet<SkyKey> newKeys = - ImmutableSet.<SkyKey>builderWithExpectedSize(values.size() + aspectKeys.size()) - .addAll(values) + ImmutableSet.<SkyKey>builderWithExpectedSize(ctKeys.size() + aspectKeys.size()) + .addAll(ctKeys) .addAll(aspectKeys) .build(); if (someConfiguredTargetEvaluated @@ -401,54 +434,19 @@ // some way -- either we analyzed a new target or we invalidated an old one or are building // targets together that haven't been built before. skyframeActionExecutor.findAndStoreArtifactConflicts( - skyframeExecutor.getActionLookupValuesInBuild(values, aspectKeys)); + skyframeExecutor.getActionLookupValuesInBuild(ctKeys, aspectKeys)); someConfiguredTargetEvaluated = false; } } ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions = skyframeActionExecutor.badActions(); - - Collection<AspectValue> goodAspects = Lists.newArrayListWithCapacity(values.size()); - Root singleSourceRoot = skyframeExecutor.getForcedSingleSourceRootIfNoExecrootSymlinkCreation(); - NestedSetBuilder<Package> packages = - singleSourceRoot == null ? NestedSetBuilder.stableOrder() : null; - for (AspectValueKey aspectKey : aspectKeys) { - AspectValue value = (AspectValue) result.get(aspectKey); - if (value == null) { - // Skip aspects that couldn't be applied to targets. - continue; - } - goodAspects.add(value); - if (packages != null) { - packages.addTransitive(value.getTransitivePackagesForPackageRootResolution()); - } - } - - // Filter out all CTs that have a bad action and convert to a list of configured targets. This - // code ensures that the resulting list of configured targets has the same order as the incoming - // list of values, i.e., that the order is deterministic. - Collection<ConfiguredTarget> goodCts = Lists.newArrayListWithCapacity(values.size()); - for (ConfiguredTargetKey value : values) { - ConfiguredTargetValue ctValue = (ConfiguredTargetValue) result.get(value); - if (ctValue == null) { - continue; - } - goodCts.add(ctValue.getConfiguredTarget()); - if (packages != null) { - packages.addTransitive(ctValue.getTransitivePackagesForPackageRootResolution()); - } - } - PackageRoots packageRoots = - singleSourceRoot == null - ? new MapAsPackageRoots(collectPackageRoots(packages.build().toList())) - : new PackageRootsNoSymlinkCreation(singleSourceRoot); - if (!result.hasError() && badActions.isEmpty()) { return new SkyframeAnalysisResult( - /*hasLoadingError=*/false, /*hasAnalysisError=*/false, - ImmutableList.copyOf(goodCts), + /*hasLoadingError=*/ false, + /*hasAnalysisError=*/ false, + ImmutableList.copyOf(cts), result.getWalkableGraph(), - ImmutableList.copyOf(goodAspects), + ImmutableList.copyOf(aspects), packageRoots); } @@ -496,14 +494,14 @@ // In order to determine the set of configured targets transitively error free from action // conflict issues, we run a post-processing update() that uses the bad action map. EvaluationResult<PostConfiguredTargetValue> actionConflictResult = - skyframeExecutor.postConfigureTargets(eventHandler, values, keepGoing, badActions); + skyframeExecutor.postConfigureTargets(eventHandler, ctKeys, keepGoing, badActions); - goodCts = Lists.newArrayListWithCapacity(values.size()); - for (ConfiguredTargetKey value : values) { + cts = Lists.newArrayListWithCapacity(ctKeys.size()); + for (ConfiguredTargetKey value : ctKeys) { PostConfiguredTargetValue postCt = actionConflictResult.get(PostConfiguredTargetValue.key(value)); if (postCt != null) { - goodCts.add(postCt.getCt()); + cts.add(postCt.getCt()); } } } @@ -511,9 +509,9 @@ return new SkyframeAnalysisResult( errors.first, result.hasError() || !badActions.isEmpty(), - ImmutableList.copyOf(goodCts), + ImmutableList.copyOf(cts), result.getWalkableGraph(), - ImmutableList.copyOf(goodAspects), + ImmutableList.copyOf(aspects), packageRoots); }