diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index b4cfa3dcadbf6d..df9eec4808c018 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -346,7 +346,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:top_level_aspects_value", "//src/main/java/com/google/devtools/build/lib/skyframe:workspace_info", "//src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2:actiongraph_v2", - "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding", + "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:action_rewound_event", "//src/main/java/com/google/devtools/build/lib/unix", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:TestType", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index f2febeffd4166f..4b6bb085982b27 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -314,6 +314,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper", "//src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2:actiongraph_v2", "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding", + "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:action_rewound_event", + "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:rewindable_graph_inconsistency_receiver", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/util", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 6bada69eee4d5e..27a46b17cd48d6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -80,6 +80,7 @@ import com.google.devtools.build.lib.skyframe.FilesystemValueChecker.ImmutableBatchDirtyResult; import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; +import com.google.devtools.build.lib.skyframe.rewinding.RewindableGraphInconsistencyReceiver; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.ExitCode; @@ -166,6 +167,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { private Duration outputTreeDiffCheckingDuration = Duration.ofSeconds(-1L); private final WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver; + private GraphInconsistencyReceiver inconsistencyReceiver = GraphInconsistencyReceiver.THROWING; private SequencedSkyframeExecutor( Consumer skyframeExecutorConsumerOnInit, @@ -230,7 +232,7 @@ protected InMemoryMemoizingEvaluator createEvaluator( skyFunctions, recordingDiffer, progressReceiver, - GraphInconsistencyReceiver.THROWING, + inconsistencyReceiver, eventFilter, emittedEventState, trackIncrementalState); @@ -271,6 +273,11 @@ public WorkspaceInfoFromDiff sync( OptionsProvider options) throws InterruptedException, AbruptExitException { if (evaluatorNeedsReset) { + // Rewinding is only supported with no incremental state and no action cache. + inconsistencyReceiver = + trackIncrementalState || useActionCache(options) + ? GraphInconsistencyReceiver.THROWING + : new RewindableGraphInconsistencyReceiver(); // Recreate MemoizingEvaluator so that graph is recreated with correct edge-clearing status, // or if the graph doesn't have edges, so that a fresh graph can be used. resetEvaluator(); @@ -293,6 +300,11 @@ public WorkspaceInfoFromDiff sync( return workspaceInfo; } + private static boolean useActionCache(OptionsProvider options) { + BuildRequestOptions buildRequestOptions = options.getOptions(BuildRequestOptions.class); + return buildRequestOptions != null && buildRequestOptions.useActionCache; + } + /** * The value types whose builders have direct access to the package locator, rather than accessing * it via an explicit Skyframe dependency. They need to be invalidated if the package locator diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/BUILD index cb08b65c4916ce..86662c0b429a75 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/BUILD @@ -10,12 +10,20 @@ filegroup( visibility = ["//src:__subpackages__"], ) +java_library( + name = "action_rewound_event", + srcs = ["ActionRewoundEvent.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/events", + ], +) + java_library( name = "rewinding", srcs = [ "ActionRewindStrategy.java", "ActionRewindingStats.java", - "ActionRewoundEvent.java", ], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", @@ -39,3 +47,34 @@ java_library( "//third_party:jsr305", ], ) + +java_library( + name = "rewindable_graph_inconsistency_receiver", + srcs = ["RewindableGraphInconsistencyReceiver.java"], + deps = [ + ":rewinding_inconsistency_utils", + "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:flogger", + "//third_party:guava", + "//third_party:jsr305", + ], +) + +java_library( + name = "rewinding_inconsistency_utils", + srcs = ["RewindingInconsistencyUtils.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/skyframe:action_template_expansion_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:artifact_nested_set_key", + "//src/main/java/com/google/devtools/build/lib/skyframe:aspect_completion_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:fileset_entry_key", + "//src/main/java/com/google/devtools/build/lib/skyframe:recursive_filesystem_traversal", + "//src/main/java/com/google/devtools/build/lib/skyframe:target_completion_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:test_completion_value", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + ], +) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java new file mode 100644 index 00000000000000..1e4e24e397fdd8 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java @@ -0,0 +1,117 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe.rewinding; + +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.common.collect.ImmutableList; +import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.skyframe.GraphInconsistencyReceiver; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.proto.GraphInconsistency.Inconsistency; +import java.util.Collection; +import java.util.function.Predicate; +import javax.annotation.Nullable; + +/** + * {@link GraphInconsistencyReceiver} for evaluations operating on graphs that support rewinding (no + * reverse dependencies, no action cache). + * + *

Action rewinding results in various kinds of inconsistencies which this receiver tolerates. + */ +public final class RewindableGraphInconsistencyReceiver implements GraphInconsistencyReceiver { + + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + + private boolean rewindingInitiated = false; + + @Override + public void noteInconsistencyAndMaybeThrow( + SkyKey key, @Nullable Collection otherKeys, Inconsistency inconsistency) { + String childrenAsString = + otherKeys != null ? GraphInconsistencyReceiver.listChildren(otherKeys) : "null"; + + // RESET_REQUESTED and PARENT_FORCE_REBUILD_OF_CHILD may be the first inconsistencies seen with + // rewinding. BUILDING_PARENT_FOUND_UNDONE_CHILD may also be seen, but it will not be the first. + switch (inconsistency) { + case RESET_REQUESTED: + checkState( + RewindingInconsistencyUtils.isTypeThatDependsOnRewindableNodes(key), + "Unexpected reset requested for: %s", + key); + logger.atInfo().log("Reset requested for: %s", key); + rewindingInitiated = true; + return; + + case PARENT_FORCE_REBUILD_OF_CHILD: + boolean parentMayForceRebuildChildren = + RewindingInconsistencyUtils.mayForceRebuildChildren(key); + ImmutableList unrewindableRebuildChildren = + otherKeys.stream() + .filter(Predicate.not(RewindingInconsistencyUtils::isRewindable)) + .collect(toImmutableList()); + checkState( + parentMayForceRebuildChildren && unrewindableRebuildChildren.isEmpty(), + "Unexpected force rebuild, parent = %s, children = %s", + key, + parentMayForceRebuildChildren + ? GraphInconsistencyReceiver.listChildren(unrewindableRebuildChildren) + : childrenAsString); + logger.atInfo().log( + "Parent force rebuild of children: parent = %s, children = %s", key, childrenAsString); + rewindingInitiated = true; + return; + + case BUILDING_PARENT_FOUND_UNDONE_CHILD: + boolean parentDependsOnRewindableNodes = + RewindingInconsistencyUtils.isTypeThatDependsOnRewindableNodes(key); + ImmutableList unrewindableUndoneChildren = + otherKeys.stream() + .filter(Predicate.not(RewindingInconsistencyUtils::isRewindable)) + .collect(toImmutableList()); + checkState( + rewindingInitiated + && parentDependsOnRewindableNodes + && unrewindableUndoneChildren.isEmpty(), + "Unexpected undone children: parent = %s, children = %s", + key, + rewindingInitiated && parentDependsOnRewindableNodes + ? GraphInconsistencyReceiver.listChildren(unrewindableUndoneChildren) + : childrenAsString); + logger.atInfo().log( + "Building parent found undone children: parent = %s, children = %s", + key, childrenAsString); + return; + + case PARENT_FORCE_REBUILD_OF_MISSING_CHILD: + case DIRTY_PARENT_HAD_MISSING_CHILD: + case ALREADY_DECLARED_CHILD_MISSING: + throw new IllegalStateException( + String.format( + "Unexpected inconsistency %s, key = %s, otherKeys = %s ", + inconsistency, key, childrenAsString)); + default: // Needed because protobuf creates additional enum values. + throw new IllegalStateException( + String.format( + "Unknown inconsistency %s, key = %s, otherKeys = %s ", + inconsistency, key, childrenAsString)); + } + } + + @Override + public boolean restartPermitted() { + return true; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingInconsistencyUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingInconsistencyUtils.java new file mode 100644 index 00000000000000..33d5b0f6ab917e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingInconsistencyUtils.java @@ -0,0 +1,65 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe.rewinding; + +import com.google.devtools.build.lib.actions.ActionLookupData; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey; +import com.google.devtools.build.lib.skyframe.ArtifactNestedSetKey; +import com.google.devtools.build.lib.skyframe.AspectCompletionValue.AspectCompletionKey; +import com.google.devtools.build.lib.skyframe.FilesetEntryKey; +import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue; +import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey; +import com.google.devtools.build.lib.skyframe.TestCompletionValue.TestCompletionKey; +import com.google.devtools.build.skyframe.SkyKey; + +/** + * Centralizes rewinding-related logic used by {@link + * com.google.devtools.build.skyframe.GraphInconsistencyReceiver} policies. + */ +public final class RewindingInconsistencyUtils { + + private RewindingInconsistencyUtils() {} + + public static boolean mayForceRebuildChildren(SkyKey key) { + return key instanceof ActionLookupData || key instanceof ArtifactNestedSetKey; + } + + /** Returns whether the key specifies a node which may be rewound by a failed action. */ + public static boolean isRewindable(SkyKey key) { + return key instanceof ActionLookupData + || key instanceof ArtifactNestedSetKey + || key instanceof Artifact + || key instanceof FilesetEntryKey + || key instanceof RecursiveFilesystemTraversalValue.TraversalRequest; + } + + /** + * Returns whether the key specifies a node which depends on nodes which may be rewound. + * + *

Such a node may discover, while in-flight, that a dependency of theirs transitioned from + * done to undone. + */ + public static boolean isTypeThatDependsOnRewindableNodes(SkyKey key) { + return key instanceof ActionLookupData + || key instanceof ArtifactNestedSetKey + || key instanceof ActionTemplateExpansionKey + || key instanceof Artifact + || key instanceof TargetCompletionKey + || key instanceof TestCompletionKey + || key instanceof AspectCompletionKey + || key instanceof RecursiveFilesystemTraversalValue.TraversalRequest + || key instanceof FilesetEntryKey; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index e836283648932b..611c13916ad79c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -69,10 +69,12 @@ import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.actions.util.TestAction.DummyAction; import com.google.devtools.build.lib.analysis.AnalysisOptions; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.OutputGroupInfo; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; @@ -129,8 +131,11 @@ import com.google.devtools.build.skyframe.Differencer.Diff; import com.google.devtools.build.skyframe.EvaluationContext; import com.google.devtools.build.skyframe.EvaluationResult; +import com.google.devtools.build.skyframe.GraphTester; import com.google.devtools.build.skyframe.NotifyingHelper; import com.google.devtools.build.skyframe.NotifyingHelper.EventType; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.TaggedEvents; @@ -140,14 +145,17 @@ import com.google.devtools.common.options.OptionsProvider; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; import java.io.Serializable; import java.lang.ref.WeakReference; import java.util.ArrayList; -import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.Callable; @@ -161,10 +169,9 @@ import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; /** Tests for {@link SequencedSkyframeExecutor}. */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public final class SequencedSkyframeExecutorTest extends BuildViewTestCase { private static final DetailedExitCode USER_DETAILED_EXIT_CODE = @@ -178,26 +185,40 @@ public final class SequencedSkyframeExecutorTest extends BuildViewTestCase { .setCrash(Crash.newBuilder().setCode(Crash.Code.CRASH_UNKNOWN)) .build()); + private final OptionsParser options = + OptionsParser.builder() + .optionsClasses( + AnalysisOptions.class, + BuildLanguageOptions.class, + BuildRequestOptions.class, + CoreOptions.class, + KeepGoingOption.class, + PackageOptions.class) + .build(); + private final Map extraSkyFunctions = new HashMap<>(); private QueryTransitivePackagePreloader visitor; - private OptionsParser options; @Before - public void createSkyframeExecutorAndVisitor() throws Exception { - skyframeExecutor = getSkyframeExecutor(); + public void createVisitorAndParseOptions() throws Exception { visitor = skyframeExecutor.getQueryTransitivePackagePreloader(); - options = - OptionsParser.builder() - .optionsClasses( - AnalysisOptions.class, - BuildLanguageOptions.class, - BuildRequestOptions.class, - CoreOptions.class, - KeepGoingOption.class, - PackageOptions.class) - .build(); options.parse("--jobs=20"); } + @Override + protected AnalysisMock getAnalysisMock() { + AnalysisMock delegate = super.getAnalysisMock(); + return new AnalysisMock.Delegate(delegate) { + @Override + public ImmutableMap getSkyFunctions( + BlazeDirectories directories) { + return ImmutableMap.builder() + .putAll(delegate.getSkyFunctions(directories)) + .putAll(extraSkyFunctions) + .buildOrThrow(); + } + }; + } + @Test public void testChangeFile() throws Exception { analysisMock.pySupport().setup(mockToolsConfig); @@ -591,7 +612,7 @@ public void testPackageFunctionHandlesExceptionFromDependencies() throws Excepti .getPackage(reporter, PackageIdentifier.createInMainRepo("bad"))); } - private Collection dirtyValues() throws InterruptedException { + private ImmutableList dirtyValues() throws InterruptedException { Diff diff = new FilesystemValueChecker( new TimestampGranularityMonitor(BlazeClock.instance()), @@ -2455,14 +2476,45 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution eventCollector, Pattern.compile(".*after scanning.*\n.*Scanning.*\n.*Test dir/top.*")); } - private void syncSkyframeExecutor() throws AbruptExitException, InterruptedException { + @Test + public void usesCorrectGraphInconsistencyReceiver( + @TestParameter boolean trackIncrementalState, @TestParameter boolean useActionCache) + throws Exception { + extraSkyFunctions.put( + SkyFunctionName.FOR_TESTING, + (key, env) -> { + if (trackIncrementalState || useActionCache) { + assertThat(env.restartPermitted()).isFalse(); + } else { + assertThat(env.restartPermitted()).isTrue(); + } + return new SkyValue() {}; + }); + initializeSkyframeExecutor(); + options.parse("--use_action_cache=" + useActionCache); + + skyframeExecutor.setActive(false); + skyframeExecutor.decideKeepIncrementalState( + /*batch=*/ false, + /*keepStateAfterBuild=*/ true, + trackIncrementalState, + /*discardAnalysisCache=*/ false, + reporter); + skyframeExecutor.setActive(true); + syncSkyframeExecutor(); + + EvaluationResult result = evaluate(ImmutableList.of(GraphTester.skyKey("key"))); + assertThat(result.hasError()).isFalse(); + } + + private void syncSkyframeExecutor() throws InterruptedException, AbruptExitException { skyframeExecutor.sync( reporter, skyframeExecutor.getPackageLocator().get(), UUID.randomUUID(), /*clientEnv=*/ ImmutableMap.of(), /*repoEnvOption=*/ ImmutableMap.of(), - new TimestampGranularityMonitor(BlazeClock.instance()), + tsgm, options); } }