diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 7cb8d15d189f06..d079d50636d723 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -100,8 +100,6 @@ import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; -import com.google.devtools.build.skyframe.MemoizingEvaluator; -import com.google.devtools.build.skyframe.NodeEntry; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunctionException; @@ -150,12 +148,6 @@ public final class ActionExecutionFunction implements SkyFunction { private final ActionRewindStrategy actionRewindStrategy; private final SkyframeActionExecutor skyframeActionExecutor; - - // Direct access to the MemoizingEvaluator should typically not be allowed in SkyFunctions. We - // allow it here as an optimization for accessing inputs that are under an ArtifactNestedSet node - // without adding a direct Skyframe edge on the input or its generating action. - private final Supplier evaluator; - private final BlazeDirectories directories; private final Supplier tsgm; private final BugReporter bugReporter; @@ -167,7 +159,6 @@ public final class ActionExecutionFunction implements SkyFunction { public ActionExecutionFunction( ActionRewindStrategy actionRewindStrategy, SkyframeActionExecutor skyframeActionExecutor, - Supplier evaluator, BlazeDirectories directories, Supplier tsgm, BugReporter bugReporter, @@ -175,7 +166,6 @@ public ActionExecutionFunction( Supplier clearNestedSetAfterActionExecution) { this.actionRewindStrategy = checkNotNull(actionRewindStrategy); this.skyframeActionExecutor = checkNotNull(skyframeActionExecutor); - this.evaluator = checkNotNull(evaluator); this.directories = checkNotNull(directories); this.tsgm = checkNotNull(tsgm); this.bugReporter = checkNotNull(bugReporter); @@ -1389,14 +1379,13 @@ private R accumulateInputs( @CanIgnoreReturnValue @Nullable - private SkyValue getAndCheckInputSkyValue( + private static SkyValue getAndCheckInputSkyValue( Action action, Artifact input, Predicate isMandatoryInput, ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler, - ActionLookupData actionLookupDataForError) - throws InterruptedException { - SkyValue value = lookupInput(input); + ActionLookupData actionLookupDataForError) { + SkyValue value = ArtifactNestedSetFunction.getInstance().getValueForKey(Artifact.key(input)); if (value == null) { if (isMandatoryInput.test(input)) { StringBuilder errorMessage = new StringBuilder(); @@ -1458,28 +1447,6 @@ private SkyValue getAndCheckInputSkyValue( return value; } - /** - * Looks up the value for an input using {@link - * MemoizingEvaluator#getExistingEntryAtCurrentlyEvaluatingVersion} to avoid establishing a direct - * dependency on the {@link Artifact#key}. - * - *

Must only be called after a proper direct dependency was established and is done. The direct - * dependency may be declared on either the {@link Artifact#key} or an {@link - * ArtifactNestedSetKey} containing the input. - */ - @Nullable - private SkyValue lookupInput(Artifact input) throws InterruptedException { - NodeEntry entry = - evaluator.get().getExistingEntryAtCurrentlyEvaluatingVersion(Artifact.key(input)); - if (entry == null) { - return null; - } - // Use toValue() so that in case the input's generating action was rewound, we still get some - // value. It might end up being a lost input when we execute the consuming action, but it may be - // available if its generating action was rewound due to losing a different output. - return entry.toValue(); - } - private static boolean findPathToKey( NestedSet start, T target, Consumer> receiver, Set seen) { if (start.getLeaves().contains(target)) { @@ -1658,7 +1625,12 @@ private final class ActionExecutionFunctionExceptionHandler { *

Also updates ArtifactNestedSetFunction#skyKeyToSkyValue if an Artifact's value is * non-null. * - * @throws ActionExecutionException if the eval of any mandatory artifact threw an exception + * @throws ActionExecutionException if the eval of any mandatory artifact threw an exception. + * This may not be the most complete exception because it may lack missing input file causes + * that did not throw exceptions, but were present as "missing file artifact values" in the + * global {@link ArtifactNestedSetFunction#artifactSkyKeyToSkyValue} map. Unfortunately, + * that map is not trustworthy in the exceptional case, since it may not have been populated + * with all data from this build before an exception shut the build down. * @return true if there is at least one input artifact that is missing */ boolean accumulateAndMaybeThrowExceptions() throws ActionExecutionException { @@ -1684,6 +1656,8 @@ boolean accumulateAndMaybeThrowExceptions() throws ActionExecutionException { if (value instanceof MissingArtifactValue) { someInputsMissing = true; } + + ArtifactNestedSetFunction.getInstance().updateValueForKey(key, value); } catch (SourceArtifactException e) { handleSourceArtifactExceptionFromSkykey(key, e); } catch (ActionExecutionException e) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetFunction.java index 66f3b3f341263e..7b920ca157d10f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetFunction.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.collect.nestedset.ArtifactNestedSetKey; @@ -26,6 +28,8 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -49,9 +53,38 @@ */ final class ArtifactNestedSetFunction implements SkyFunction { + private static ArtifactNestedSetFunction singleton = null; + + /** + * A concurrent map from Artifacts' SkyKeys to their SkyValue, for Artifacts that are part of + * NestedSets which were evaluated as {@link ArtifactNestedSetKey}. + * + *

Question: Why don't we clear artifactSkyKeyToSkyValue after each build? + * + *

The map maintains an invariant: if an ArtifactNestedSetKey exists on Skyframe, the SkyValues + * of its member Artifacts are available in artifactSkyKeyToSkyValue. + * + *

Example: Action A has as input NestedSet X, where X = (X1, X2), where X1 & X2 are 2 + * transitive NestedSets. + * + *

Run 0: Establish dependency from A to X and from X to X1 & X2. Artifacts from X1 & X2 have + * entries in artifactSkyKeyToSkyValue. + * + *

Run 1 (incremental): Some changes were made to an Artifact in X1 such that X1, X and A's + * SkyKeys are marked as dirty. A's ActionLookupData has to be re-evaluated. This involves asking + * Skyframe to compute SkyValues for its inputs. + * + *

However, X2 is not dirty, so Skyframe won't re-run ArtifactNestedSetFunction#compute for X2, + * therefore not populating artifactSkyKeyToSkyValue with X2's member Artifacts. Hence if we clear + * artifactSkyKeyToSkyValue between build 0 and 1, X2's member artifacts' SkyValues would not be + * available in the map. TODO(leba): Make this weak-keyed. + */ + private ConcurrentMap artifactSkyKeyToSkyValue = new ConcurrentHashMap<>(); + private final Supplier consumedArtifactsTrackerSupplier; - ArtifactNestedSetFunction(Supplier consumedArtifactsTrackerSupplier) { + private ArtifactNestedSetFunction( + Supplier consumedArtifactsTrackerSupplier) { this.consumedArtifactsTrackerSupplier = consumedArtifactsTrackerSupplier; } @@ -98,6 +131,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (value instanceof MissingArtifactValue) { result = ArtifactNestedSetValue.SOME_MISSING; } + + artifactSkyKeyToSkyValue.put(key, value); } catch (SourceArtifactException e) { // SourceArtifactException is never catastrophic. transitiveExceptionsBuilder.add(Pair.of(key, e)); @@ -131,6 +166,34 @@ public SkyValue compute(SkyKey skyKey, Environment env) return result; } + static ArtifactNestedSetFunction getInstance() { + return checkNotNull(singleton); + } + + /** + * Creates a new instance. Should only be used in {@code SkyframeExecutor#skyFunctions}. Keeping + * this method separated from {@code #getInstance} since sometimes we need to overwrite the + * existing instance. + */ + static ArtifactNestedSetFunction createInstance( + Supplier consumedArtifactsTrackerSupplier) { + singleton = new ArtifactNestedSetFunction(consumedArtifactsTrackerSupplier); + return singleton; + } + + /** Reset the various state-keeping maps of ArtifactNestedSetFunction. */ + void resetArtifactNestedSetFunctionMaps() { + artifactSkyKeyToSkyValue = new ConcurrentHashMap<>(); + } + + SkyValue getValueForKey(SkyKey skyKey) { + return artifactSkyKeyToSkyValue.get(skyKey); + } + + void updateValueForKey(SkyKey skyKey, SkyValue skyValue) { + artifactSkyKeyToSkyValue.put(skyKey, skyValue); + } + /** Mainly used for error bubbling when evaluating direct/transitive children. */ private static final class ArtifactNestedSetFunctionException extends SkyFunctionException { 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 bda2ee2a5ce3e4..4cd67f9967cbdd 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 @@ -737,7 +737,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) { new PlatformMappingFunction(ruleClassProvider.getFragmentRegistry().getOptionsClasses())); map.put( SkyFunctions.ARTIFACT_NESTED_SET, - new ArtifactNestedSetFunction(this::getConsumedArtifactsTracker)); + ArtifactNestedSetFunction.createInstance(this::getConsumedArtifactsTracker)); BuildDriverFunction buildDriverFunction = new BuildDriverFunction( new TransitiveActionLookupValuesHelper() { @@ -774,7 +774,6 @@ protected SkyFunction newActionExecutionFunction() { return new ActionExecutionFunction( actionRewindStrategy, skyframeActionExecutor, - () -> memoizingEvaluator, directories, tsgm::get, bugReporter, @@ -970,6 +969,7 @@ public void handleAnalysisInvalidatingChange() { analysisCacheInvalidated = true; skyframeBuildView.clearInvalidatedActionLookupKeys(); skyframeBuildView.clearLegacyData(); + ArtifactNestedSetFunction.getInstance().resetArtifactNestedSetFunctionMaps(); } /** Used with dump --rules. */ diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 6dc530ce703ded..07ae6f06483c64 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -112,7 +112,6 @@ import com.google.devtools.build.skyframe.EventFilter; import com.google.devtools.build.skyframe.GraphInconsistencyReceiver; import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator; -import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.RecordingDifferencer; import com.google.devtools.build.skyframe.SequencedRecordingDifferencer; import com.google.devtools.build.skyframe.SkyFunction; @@ -237,7 +236,6 @@ protected BuilderWithResult createBuilder( skyframeActionExecutor.configure( cache, ActionInputPrefetcher.NONE, DiscoveredModulesPruner.DEFAULT); - AtomicReference evaluatorRef = new AtomicReference<>(); InMemoryMemoizingEvaluator evaluator = new InMemoryMemoizingEvaluator( ImmutableMap.builder() @@ -254,7 +252,6 @@ protected BuilderWithResult createBuilder( new ActionExecutionFunction( new ActionRewindStrategy(), skyframeActionExecutor, - evaluatorRef::get, directories, () -> tsgm, BugReporter.defaultInstance(), @@ -298,7 +295,9 @@ protected BuilderWithResult createBuilder( .put( SkyFunctions.ACTION_TEMPLATE_EXPANSION, new DelegatingActionTemplateExpansionFunction()) - .put(SkyFunctions.ARTIFACT_NESTED_SET, new ArtifactNestedSetFunction(() -> null)) + .put( + SkyFunctions.ARTIFACT_NESTED_SET, + ArtifactNestedSetFunction.createInstance(() -> null)) .buildOrThrow(), differencer, evaluationProgressReceiver, @@ -307,7 +306,6 @@ protected BuilderWithResult createBuilder( new EmittedEventState(), /* keepEdges= */ true, /* usePooledInterning= */ true); - evaluatorRef.set(evaluator); PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID()); PrecomputedValue.ACTION_ENV.set(differencer, ImmutableMap.of()); PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());