Skip to content

Commit

Permalink
Automated rollback of commit ffa75bf.
Browse files Browse the repository at this point in the history 
*** 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
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Dec 8, 2023
1 parent 65da015 commit e560c49
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 45 deletions.
48 changes: 11 additions & 37 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<MemoizingEvaluator> evaluator;

private final BlazeDirectories directories;
private final Supplier<TimestampGranularityMonitor> tsgm;
private final BugReporter bugReporter;
Expand All @@ -167,15 +159,13 @@ public final class ActionExecutionFunction implements SkyFunction {
public ActionExecutionFunction(
ActionRewindStrategy actionRewindStrategy,
SkyframeActionExecutor skyframeActionExecutor,
Supplier<MemoizingEvaluator> evaluator,
BlazeDirectories directories,
Supplier<TimestampGranularityMonitor> tsgm,
BugReporter bugReporter,
Supplier<ConsumedArtifactsTracker> consumedArtifactsTrackerSupplier,
Supplier<Boolean> 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);
Expand Down Expand Up @@ -1389,14 +1379,13 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(

@CanIgnoreReturnValue
@Nullable
private SkyValue getAndCheckInputSkyValue(
private static SkyValue getAndCheckInputSkyValue(
Action action,
Artifact input,
Predicate<Artifact> 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();
Expand Down Expand Up @@ -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}.
*
* <p>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 <T> boolean findPathToKey(
NestedSet<T> start, T target, Consumer<NestedSet<T>> receiver, Set<NestedSet.Node> seen) {
if (start.getLeaves().contains(target)) {
Expand Down Expand Up @@ -1658,7 +1625,12 @@ private final class ActionExecutionFunctionExceptionHandler {
* <p>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 {
Expand All @@ -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) {
Expand Down
65 changes: 64 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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}.
*
* <p>Question: Why don't we clear artifactSkyKeyToSkyValue after each build?
*
* <p>The map maintains an invariant: if an ArtifactNestedSetKey exists on Skyframe, the SkyValues
* of its member Artifacts are available in artifactSkyKeyToSkyValue.
*
* <p>Example: Action A has as input NestedSet X, where X = (X1, X2), where X1 & X2 are 2
* transitive NestedSets.
*
* <p>Run 0: Establish dependency from A to X and from X to X1 & X2. Artifacts from X1 & X2 have
* entries in artifactSkyKeyToSkyValue.
*
* <p>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.
*
* <p>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<SkyKey, SkyValue> artifactSkyKeyToSkyValue = new ConcurrentHashMap<>();

private final Supplier<ConsumedArtifactsTracker> consumedArtifactsTrackerSupplier;

ArtifactNestedSetFunction(Supplier<ConsumedArtifactsTracker> consumedArtifactsTrackerSupplier) {
private ArtifactNestedSetFunction(
Supplier<ConsumedArtifactsTracker> consumedArtifactsTrackerSupplier) {
this.consumedArtifactsTrackerSupplier = consumedArtifactsTrackerSupplier;
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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<ConsumedArtifactsTracker> 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 {

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -774,7 +774,6 @@ protected SkyFunction newActionExecutionFunction() {
return new ActionExecutionFunction(
actionRewindStrategy,
skyframeActionExecutor,
() -> memoizingEvaluator,
directories,
tsgm::get,
bugReporter,
Expand Down Expand Up @@ -970,6 +969,7 @@ public void handleAnalysisInvalidatingChange() {
analysisCacheInvalidated = true;
skyframeBuildView.clearInvalidatedActionLookupKeys();
skyframeBuildView.clearLegacyData();
ArtifactNestedSetFunction.getInstance().resetArtifactNestedSetFunctionMaps();
}

/** Used with dump --rules. */
Expand Down
8 changes: 3 additions & 5 deletions src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -237,7 +236,6 @@ protected BuilderWithResult createBuilder(
skyframeActionExecutor.configure(
cache, ActionInputPrefetcher.NONE, DiscoveredModulesPruner.DEFAULT);

AtomicReference<MemoizingEvaluator> evaluatorRef = new AtomicReference<>();
InMemoryMemoizingEvaluator evaluator =
new InMemoryMemoizingEvaluator(
ImmutableMap.<SkyFunctionName, SkyFunction>builder()
Expand All @@ -254,7 +252,6 @@ protected BuilderWithResult createBuilder(
new ActionExecutionFunction(
new ActionRewindStrategy(),
skyframeActionExecutor,
evaluatorRef::get,
directories,
() -> tsgm,
BugReporter.defaultInstance(),
Expand Down Expand Up @@ -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,
Expand All @@ -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());
Expand Down

0 comments on commit e560c49

Please sign in to comment.