From e560c490422b0a794b0c8b330598b9ddec2b4d50 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 8 Dec 2023 07:20:27 -0800 Subject: [PATCH] Automated rollback of commit ffa75bf996ce52067fae8f530921a1db16fb46b2. *** Reason for rollback *** Does not handle the case where a flaky action is rewound and evaluates to an error (toValue returns null). This situation was observed in b/315301248. *** Original change description *** Replace the big map in `ArtifactNestedSetFunction` with direct Skyframe graph access. This is fundamentally equivalent, but saves memory (~0.4% on an example build) from the `ConcurrentHashMap` structure, though all of the keys and values are typically still retained in Skyframe nodes. Memory savings may be greater on certain incremental builds due to Skyframe GC of dirty nodes (by default, we delete all dirty nodes at the end of a build, see `--version_window_for_dirty_node_gc`). Previously,... *** PiperOrigin-RevId: 589123293 Change-Id: I71e20d93c4d2d0c2c384cb9e7e9edbb0203d69d0 --- .../lib/skyframe/ActionExecutionFunction.java | 48 ++++---------- .../skyframe/ArtifactNestedSetFunction.java | 65 ++++++++++++++++++- .../build/lib/skyframe/SkyframeExecutor.java | 4 +- .../skyframe/TimestampBuilderTestCase.java | 8 +-- 4 files changed, 80 insertions(+), 45 deletions(-) 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());