diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java index aeeb2b3e63be8e..c8a2d013238910 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.ActionLookupValue; @@ -72,6 +73,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Set; import javax.annotation.Nullable; /** @@ -83,6 +85,17 @@ public class BuildDriverFunction implements SkyFunction { private final Supplier ruleContextConstraintSemantics; private final Supplier extraActionFilterSupplier; + // A set of BuildDriverKeys that have been checked for conflicts. + // This gets cleared after each build. + // We can't use SkyKeyComputeState here since it doesn't guarantee that the same state for + // a previously requested SkyKey is retrieved. This could cause a correctness issue: + // - we clear the conflict checking states and shut down the Executors after all the analysis + // work is done in the build + // - If the SkyKeyComputeState for this BuildDriverKey was cleared, an evaluation of this key + // would attempt again to check for conflicts => we redo the work, or a race condition with the + // shutting down of the Executors could lead to a RejectedExecutionException. + private Set checkedForConflicts = Sets.newConcurrentHashSet(); + BuildDriverFunction( TransitiveActionLookupValuesHelper transitiveActionLookupValuesHelper, Supplier incrementalArtifactConflictFinder, @@ -95,7 +108,6 @@ public class BuildDriverFunction implements SkyFunction { } private static class State implements SkyKeyComputeState { - private ImmutableMap actionConflicts; // It's only necessary to do this check once. private boolean checkedForCompatibility = false; private boolean checkedForPlatformCompatibility = false; @@ -129,28 +141,22 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } - // This code path should not be run during error bubbling for several reasons: - // 1. Correctness: to check for action conflicts, we need access to the transitive - // ConfiguredTargets, which will be null after AnalysisPhaseCompleteEvent in - // --discard_analysis_cache mode. - // 2. Performance: this method is CPU intensive, and it does not offer anything while error - // bubbling. - if (!env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) { - // Unconditionally check for action conflicts. - // TODO(b/214371092): Only check when necessary. - try (SilentCloseable c = - Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) { - if (state.actionConflicts == null) { - state.actionConflicts = - checkActionConflicts(actionLookupKey, buildDriverKey.strictActionConflictCheck()); - } - if (!state.actionConflicts.isEmpty()) { + // Unconditionally check for action conflicts. + // TODO(b/214371092): Only check when necessary. + try (SilentCloseable c = + Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) { + // We only check for action conflict once per BuildDriverKey. + if (checkedForConflicts.add(buildDriverKey)) { + ImmutableMap actionConflicts = + checkActionConflicts(actionLookupKey, buildDriverKey.strictActionConflictCheck()); + if (!actionConflicts.isEmpty()) { throw new BuildDriverFunctionException( new TopLevelConflictException( "Action conflict(s) detected while analyzing top-level target " + actionLookupKey.getLabel(), - state.actionConflicts)); + actionConflicts)); } + } } @@ -246,6 +252,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) return new BuildDriverValue(topLevelSkyValue, /*skipped=*/ false); } + public void resetActionConflictCheckingStatus() { + checkedForConflicts = Sets.newConcurrentHashSet(); + } + private static void postTopLevelTargetAnalyzedEvent( Environment env, ConfiguredTargetValue configuredTargetValue, 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 c60636a2055534..5e58f4df7beccc 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 @@ -636,6 +636,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( // We unconditionally reset the states here instead of in #analysisFinishedCallback since // in case of --nokeep_going & analysis error, the analysis phase is never finished. skyframeExecutor.resetIncrementalArtifactConflictFindingStates(); + skyframeExecutor.resetBuildDriverFunction(); Set additionalArtifacts = new HashSet<>(); 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 bcdbf1f33ff539..11c737eecee88a 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 @@ -335,6 +335,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur new AtomicReference<>(); protected final SkyframeActionExecutor skyframeActionExecutor; private ActionExecutionFunction actionExecutionFunction; + private BuildDriverFunction buildDriverFunction; private GlobFunction globFunction; protected SkyframeProgressReceiver progressReceiver; private CyclesReporter cyclesReporter = null; @@ -660,8 +661,7 @@ private ImmutableMap skyFunctions() { map.put( SkyFunctions.ARTIFACT_NESTED_SET, ArtifactNestedSetFunction.createInstance(valueBasedChangePruningEnabled())); - map.put( - SkyFunctions.BUILD_DRIVER, + BuildDriverFunction buildDriverFunction = new BuildDriverFunction( new TransitiveActionLookupValuesHelper() { @Override @@ -676,7 +676,10 @@ public void registerConflictFreeKeys(ImmutableSet keys) { }, this::getIncrementalArtifactConflictFinder, this::getRuleContextConstraintSemantics, - this::getExtraActionFilter)); + this::getExtraActionFilter); + map.put(SkyFunctions.BUILD_DRIVER, buildDriverFunction); + this.buildDriverFunction = buildDriverFunction; + map.putAll(extraSkyFunctions); return ImmutableMap.copyOf(map); } @@ -2398,6 +2401,10 @@ void resetActionConflictsStoredInSkyframe() { SkyFunctionName.functionIs(SkyFunctions.ACTION_LOOKUP_CONFLICT_FINDING)); } + public void resetBuildDriverFunction() { + buildDriverFunction.resetActionConflictCheckingStatus(); + } + /** Resets the incremental artifact conflict finder to ensure incremental correctness. */ public void resetIncrementalArtifactConflictFindingStates() throws InterruptedException { incrementalArtifactConflictFinder.shutdown(); diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java index e743559726f32c..5b6aa3b1136d54 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java @@ -318,10 +318,6 @@ default void injectVersionForNonHermeticFunction(Version version) {} *

Such a SkyFunction cannot unconditionally return a value, since in --nokeep_going mode it * may be called upon to transform a lower-level exception. This method can tell it whether to * transform a dependency's exception or ignore it and return a value as usual. - * - *

An exception is with {@link - * com.google.devtools.build.lib.skyframe.BuildDriverFunction#checkActionConflicts}. See the - * documentation at the method for more details. */ boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors();