From 81f312102ff25b89e3afadf05620785ace319ae0 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 27 Mar 2023 07:41:24 -0700 Subject: [PATCH] [Skymeld] Don't use SkyKeyComputeState to manage conflict checking in BuildDriverFunction. When all analysis work in the build is finished, we clear the various intermediate states and shut down the executors for conflict checking. We don't expect any further conflict checking beyond this point, and until now have been using SkyKeyComputeState for that. This is a poor fit because SkyKeyComputeState doesn't guarantee that the same instance is returned when calling #compute on the same SkyKey and should only be used as a performance optimization. A bug may then occur: some conflict checking tasks may be sent to the executors still after they were shut down and resulted in a RejectedExecutionException. This CL fixes that issue by using a dedicated Set in BuildDriverFunction that tracks whether a BuildDriverKey has been checked for conflicts or not. This set is reset after each build for consistency. PiperOrigin-RevId: 519718108 Change-Id: I1cc8d883815514f3a8ba5f37f365ebabca9215d3 --- .../lib/skyframe/BuildDriverFunction.java | 46 +++++++++++-------- .../build/lib/skyframe/SkyframeBuildView.java | 1 + .../build/lib/skyframe/SkyframeExecutor.java | 13 ++++-- .../devtools/build/skyframe/SkyFunction.java | 4 -- 4 files changed, 39 insertions(+), 25 deletions(-) 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();