Automated rollback of commit 9321316b34767b06c3071b2cf2a4de189874fcce. *** Reason for rollback *** Design change, 2 boolean flags instead of 1 enum flag *** Original change description *** Add --incremental_state_retention_strategy This option is intended to replace some of the uses of --batch. It lets users specify that builds should not be incremental, and how eagerly to discard the state that is kept around for incrementality. Note that for both values discard_eargerly and keep_for_life_of_build, the build graph is kept around until the next build. This may change. Will add tests for keep_for_life_of_build in a later change, for now it will warn that that feature is experimen... *** ROLLBACK_OF=178661777 RELNOTES: None. PiperOrigin-RevId: 178681472
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index d769c68..d193bb3 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
@@ -20,7 +20,6 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Converters.RangeConverter; -import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -405,51 +404,18 @@ ) public boolean useActionCache; - /** - * Timeline for retaining inmemory incremental state. - * - * <ol> - * <li>{@code KEEP_FOREVER} - the default behavior, maintains incrementality state across builds - * to minimize cost of subsequent builds. - * <li>{@code KEEP_FOR_LIFE_OF_BUILD} - during this build, maintains incrementality state like - * KEEP_FOREVER, but discards it at the end of the build. The following build will not be - * incremental. - * <li>{@code DISCARD_EAGERLY} - memory saving mode, eagerly discards state that would have been - * useful for subsequent builds but is not necessary for this one. The following build will - * not be incremental. - * </ol> - */ - public enum IncrementalStateRetentionStrategy { - KEEP_FOREVER, - KEEP_FOR_LIFE_OF_BUILD, - DISCARD_EAGERLY - } - - /** Option converter for {@link IncrementalStateRetentionStrategy}. */ - public static class IncrementalStateRetentionStrategyConverter - extends EnumConverter<IncrementalStateRetentionStrategy> { - public IncrementalStateRetentionStrategyConverter() { - super( - IncrementalStateRetentionStrategy.class, - "Timeline for retaining inmemory incremental state"); - } - } - @Option( - name = "incremental_state_retention_strategy", - defaultValue = "KEEP_FOREVER", + name = "keep_incrementality_data", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE}, - metadataTags = {OptionMetadataTag.EXPERIMENTAL}, - converter = IncrementalStateRetentionStrategyConverter.class, help = - "Controls how long Blaze-internal data that allows for invalidation and re-evaluation " - + "on incremental builds is kept. Default is keep_forever, which allows for " - + "incremental builds. Non-incremental options are keep_for_life_of_build, which " - + "discards state at the end of a build, and discard_eagerly, which saves memory " - + "during the build by discarding state eagerly." + "If false, discard Blaze-internal data that allows for invalidation and re-evaluation " + + "on incremental builds in order to save memory on this build. Subsequent builds " + + "will not have any incrementality with respect to this one. Usually you will want" + + "to specify the --batch startup option along with this one." ) - public IncrementalStateRetentionStrategy incrementalityStateRetentionStrategy; + public boolean keepIncrementalityData; /** Converter for jobs: [0, MAX_JOBS] or "auto". */ public static class JobsConverter extends RangeConverter {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 2e5f599..9f20f76 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -438,7 +438,7 @@ skyframeExecutor.drainChangedFiles(); if (request.getViewOptions().discardAnalysisCache - || !skyframeExecutor.hasIncrementalState()) { + || !request.getBuildOptions().keepIncrementalityData) { // Free memory by removing cache entries that aren't going to be needed. env.getSkyframeBuildView() .clearAnalysisCache(analysisResult.getTargetsToBuild(), analysisResult.getAspects()); @@ -666,7 +666,7 @@ return successfulTargets; } - /** Get action cache if present or reload it from the on-disk cache. */ + private ActionCache getActionCache() throws LocalEnvironmentException { try { return env.getPersistentActionCache();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index 2c11eac..54552a6 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -605,7 +605,7 @@ // Fail fast in the case where a Blaze command forgets to install the package path correctly. skyframeExecutor.setActive(false); - // Let skyframe figure out how much incremental state it will be keeping. + // Let skyframe figure out if it needs to store graph edges for this build. skyframeExecutor.decideKeepIncrementalState( runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).batch, options,
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 e6c4137..f3e4bf4 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
@@ -34,7 +34,6 @@ import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; -import com.google.devtools.build.lib.buildtool.BuildRequestOptions.IncrementalStateRetentionStrategy; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.Uninterruptibles; import com.google.devtools.build.lib.events.Event; @@ -108,14 +107,17 @@ private boolean lastAnalysisDiscarded = false; + private enum IncrementalState { + NORMAL, + CLEAR_EDGES_AND_ACTIONS + } + /** - * Assume that this build is incremental. If {@link - * IncrementalStateRetentionStrategy#DISCARD_EAGERLY}, the graph will not store edges, saving + * If {@link IncrementalState#CLEAR_EDGES_AND_ACTIONS}, the graph will not store edges, saving * memory but making subsequent builds not incremental. Also, each action will be dereferenced * once it is executed, saving memory. */ - private IncrementalStateRetentionStrategy incrementalState = - IncrementalStateRetentionStrategy.KEEP_FOREVER; + private IncrementalState incrementalState = IncrementalState.NORMAL; private boolean evaluatorNeedsReset = false; @@ -502,71 +504,52 @@ SkyFunctionName.functionIs(SkyFunctions.FILE_STATE))); } - /** - * {@inheritDoc} - * - * <p>Necessary conditions to not store graph edges are either - * - * <ol> - * <li>batch (since incremental builds are not possible) and discard_analysis_cache (since - * otherwise user isn't concerned about saving memory this way). - * <li>incremental_state_retention_strategy set to discard_eagerly. - * </ol> - */ @Override public void decideKeepIncrementalState( boolean batch, OptionsProvider options, EventHandler eventHandler) { Preconditions.checkState(!active); BuildView.Options viewOptions = options.getOptions(BuildView.Options.class); BuildRequestOptions requestOptions = options.getOptions(BuildRequestOptions.class); - if (requestOptions != null - && requestOptions.incrementalityStateRetentionStrategy.equals( - IncrementalStateRetentionStrategy.KEEP_FOR_LIFE_OF_BUILD)) { - eventHandler.handle( - Event.warn( - "Incremental state retention strategy keep_for_life_of_build is experimental, use " - + "with caution.")); - } - - // First check if the incrementality state should be kept around during the build. - IncrementalStateRetentionStrategy explicitStrategyOrDefault = - requestOptions != null - ? requestOptions.incrementalityStateRetentionStrategy - : IncrementalStateRetentionStrategy.KEEP_FOREVER; - boolean explicitlyRequestedNoIncrementalState = - explicitStrategyOrDefault.equals(IncrementalStateRetentionStrategy.DISCARD_EAGERLY); - boolean implicitlyRequestedNoIncrementalState = + boolean explicitlyRequestedNoIncrementalData = + requestOptions != null && !requestOptions.keepIncrementalityData; + boolean implicitlyRequestedNoIncrementalData = batch && viewOptions != null && viewOptions.discardAnalysisCache; boolean discardingEdges = - explicitlyRequestedNoIncrementalState || implicitlyRequestedNoIncrementalState; - if (!explicitlyRequestedNoIncrementalState && implicitlyRequestedNoIncrementalState) { - eventHandler.handle( - Event.warn( - "--batch and --discard_analysis_cache specified, but " - + "--incremental_state_retention_strategy not set to discard_eagerly: " - + "incrementality data is implicitly discarded, but you may need to specify " - + "--incremental_state_retention_strategy=discard_eagerly in the future if " - + "you want to maximize memory savings.")); + explicitlyRequestedNoIncrementalData || implicitlyRequestedNoIncrementalData; + if (explicitlyRequestedNoIncrementalData != implicitlyRequestedNoIncrementalData) { + if (requestOptions != null && !explicitlyRequestedNoIncrementalData) { + eventHandler.handle( + Event.warn( + "--batch and --discard_analysis_cache specified, but --nokeep_incrementality_data " + + "not specified: incrementality data is implicitly discarded, but you may need" + + " to specify --nokeep_incrementality_data in the future if you want to " + + "maximize memory savings.")); + } + if (!batch) { + eventHandler.handle( + Event.warn( + "--batch not specified with --nokeep_incrementality_data: the server will " + + "remain running, but the next build will not be incremental on this one.")); + } } - - // Now check if it is necessary to wipe the previous state. We do this if either the previous - // or current incrementalStateRetentionStrategy requires the build to have been isolated. - IncrementalStateRetentionStrategy oldState = incrementalState; + IncrementalState oldState = incrementalState; incrementalState = - discardingEdges - ? IncrementalStateRetentionStrategy.DISCARD_EAGERLY - : explicitStrategyOrDefault; - if (!incrementalState.equals(IncrementalStateRetentionStrategy.KEEP_FOREVER) - || !oldState.equals(IncrementalStateRetentionStrategy.KEEP_FOREVER)) { - logger.info("Old incremental state will be wiped, retention strategy is " + incrementalState); + discardingEdges ? IncrementalState.CLEAR_EDGES_AND_ACTIONS : IncrementalState.NORMAL; + if (oldState != incrementalState) { + logger.info("Set incremental state to " + incrementalState); evaluatorNeedsReset = true; - removeActionsAfterEvaluation.set(discardingEdges); + removeActionsAfterEvaluation.set( + incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS); + } else if (incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS) { + evaluatorNeedsReset = true; } } @Override public boolean hasIncrementalState() { - return !incrementalState.equals(IncrementalStateRetentionStrategy.DISCARD_EAGERLY); + // TODO(bazel-team): Combine this method with clearSkyframeRelevantCaches() once legacy + // execution is removed [skyframe-execution]. + return incrementalState == IncrementalState.NORMAL; } @Override
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 c91a693..da4d4ef 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
@@ -686,19 +686,21 @@ } /** - * Decides if graph edges should be stored during this evaluation and checks if the state from the - * last evaluation, if any, can be kept. + * Decides if graph edges should be stored for this build. If not, notes that the next evaluation + * on the graph should reset it first. Necessary conditions to not store graph edges are: * - * <p>If not, it will mark this state for deletion. The actual cleaning is put off until {@link - * #sync}, in case no evaluation was actually called for and the existing state can be kept for - * longer. + * <ol> + * <li>batch (since incremental builds are not possible); + * <li>keep-going (since otherwise bubbling errors up may require edges of done nodes); + * <li>discard_analysis_cache (since otherwise user isn't concerned about saving memory this + * way). + * </ol> */ public void decideKeepIncrementalState( boolean batch, OptionsProvider viewOptions, EventHandler eventHandler) { // Assume incrementality. } - /** Whether this executor tracks state for the purpose of improving incremental performance. */ public boolean hasIncrementalState() { return true; } @@ -1856,9 +1858,6 @@ return pkgFactory.getPackageBuilderHelperForTesting(); } - /** - * Initializes and syncs the graph with the given options, readying it for the next evaluation. - */ public void sync( ExtendedEventHandler eventHandler, PackageCacheOptions packageCacheOptions,
diff --git a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java index f1a002f..aaf3569 100644 --- a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java +++ b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
@@ -39,8 +39,8 @@ private List<EventHandler> handlers = new ArrayList<>(); /** - * Determine which events the {@link #collector()} created by this apparatus will collect. - * Default: {@link EventKind#ERRORS_WARNINGS_AND_INFO}. + * Determine which events the {@link #collector()} created by this apparatus + * will collect. Default: {@link EventKind#ERRORS_AND_WARNINGS}. */ public EventCollectionApparatus(Set<EventKind> mask) { eventCollector = new EventCollector(mask);
diff --git a/src/test/shell/integration/discard_graph_edges_lib.sh b/src/test/shell/integration/discard_graph_edges_lib.sh index e7f97f9..683b96b 100644 --- a/src/test/shell/integration/discard_graph_edges_lib.sh +++ b/src/test/shell/integration/discard_graph_edges_lib.sh
@@ -17,7 +17,7 @@ # discard_graph_edges_lib.sh: functions needed by discard_graph_edges_test.sh STARTUP_FLAGS="--batch" -BUILD_FLAGS="--discard_analysis_cache --incremental_state_retention_strategy=discard_eagerly" +BUILD_FLAGS="--discard_analysis_cache --nokeep_incrementality_data" function extract_histogram_count() { local histofile="$1"
diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh index dd80672..4b0311c 100755 --- a/src/test/shell/integration/discard_graph_edges_test.sh +++ b/src/test/shell/integration/discard_graph_edges_test.sh
@@ -334,22 +334,21 @@ # The following tests are not expected to exercise codepath -- make sure nothing bad happens. function test_no_batch() { - bazel $STARTUP_FLAGS --nobatch test $BUILD_FLAGS \ - --incremental_state_retention_strategy=keep_forever //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel $STARTUP_FLAGS --nobatch test $BUILD_FLAGS --keep_incrementality_data \ + //testing:mytest >& "$TEST_log" || fail "Expected success" } function test_no_discard_analysis_cache() { bazel $STARTUP_FLAGS test $BUILD_FLAGS --nodiscard_analysis_cache \ - --incremental_state_retention_strategy=keep_forever //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + --keep_incrementality_data //testing:mytest >& "$TEST_log" \ + || fail "Expected success" } function test_packages_cleared_nobatch() { readonly local old_startup_flags="$STARTUP_FLAGS" STARTUP_FLAGS="--nobatch" readonly local old_build_flags="$BUILD_FLAGS" - BUILD_FLAGS="--incremental_state_retention_strategy=discard_eagerly --discard_analysis_cache" + BUILD_FLAGS="--nokeep_incrementality_data --discard_analysis_cache" test_packages_cleared STARTUP_FLAGS="$old_startup_flags" BUILD_FLAGS="$old_build_flags" @@ -357,7 +356,7 @@ function test_packages_cleared_implicit_noincrementality_data() { readonly local old_build_flags="$BUILD_FLAGS" - BUILD_FLAGS="$BUILD_FLAGS --incremental_state_retention_strategy=keep_forever" + BUILD_FLAGS="$BUILD_FLAGS --keep_incrementality_data" test_packages_cleared BUILD_FLAGS="$old_build_flags" } @@ -366,22 +365,22 @@ readonly local old_startup_flags="$STARTUP_FLAGS" STARTUP_FLAGS="--nobatch" readonly local old_build_flags="$BUILD_FLAGS" - BUILD_FLAGS="--incremental_state_retention_strategy=discard_eagerly" + BUILD_FLAGS="--nokeep_incrementality_data" test_actions_deleted_after_execution STARTUP_FLAGS="$old_startup_flags" BUILD_FLAGS="$old_build_flags" } function test_dump_after_discard_incrementality_data() { - bazel build --incremental_state_retention_strategy=discard_eagerly \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel build --nokeep_incrementality_data //testing:mytest >& "$TEST_log" \ + || fail "Expected success" bazel dump --skyframe=detailed >& "$TEST_log" || fail "Expected success" expect_log "//testing:mytest" } function test_query_after_discard_incrementality_data() { - bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --nokeep_incrementality_data //testing:mytest \ + >& "$TEST_log" || fail "Expected success" bazel query --noexperimental_ui --output=label_kind //testing:mytest \ >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" @@ -391,8 +390,8 @@ function test_shutdown_after_discard_incrementality_data() { readonly local server_pid="$(bazel info server_pid 2> /dev/null)" [[ -z "$server_pid" ]] && fail "Couldn't get server pid" - bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --nokeep_incrementality_data //testing:mytest \ + >& "$TEST_log" || fail "Expected success" bazel shutdown || fail "Expected success" readonly local new_server_pid="$(bazel info server_pid 2> /dev/null)" [[ "$server_pid" != "$new_server_pid" ]] \ @@ -400,22 +399,20 @@ } function test_clean_after_discard_incrementality_data() { - bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --nokeep_incrementality_data //testing:mytest \ + >& "$TEST_log" || fail "Expected success" bazel clean >& "$TEST_log" || fail "Expected success" } function test_switch_back_and_forth() { readonly local server_pid="$(bazel info \ - --incremental_state_retention_strategy=discard_eagerly server_pid 2> /dev/null)" + --nokeep_incrementality_data server_pid 2> /dev/null)" [[ -z "$server_pid" ]] && fail "Couldn't get server pid" - bazel test --noexperimental_ui \ - --incremental_state_retention_strategy=discard_eagerly //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui --nokeep_incrementality_data \ + //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" - bazel test --noexperimental_ui \ - --incremental_state_retention_strategy=discard_eagerly //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui --nokeep_incrementality_data \ + //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" bazel test --noexperimental_ui //testing:mytest >& "$TEST_log" \ || fail "Expected success" @@ -423,9 +420,8 @@ bazel test --noexperimental_ui //testing:mytest >& "$TEST_log" \ || fail "Expected success" expect_not_log "Loading package: testing" - bazel test --noexperimental_ui \ - --incremental_state_retention_strategy=discard_eagerly //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui --nokeep_incrementality_data \ + //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" readonly local new_server_pid="$(bazel info server_pid 2> /dev/null)" [[ "$server_pid" == "$new_server_pid" ]] \ @@ -435,8 +431,10 @@ function test_warns_on_unexpected_combos() { bazel --batch build --nobuild --discard_analysis_cache >& "$TEST_log" \ || fail "Expected success" - expect_log \ - "--batch and --discard_analysis_cache specified, but --incremental_state_retention_strategy not set to discard_eagerly" + expect_log "--batch and --discard_analysis_cache specified, but --nokeep_incrementality_data not specified" + bazel build --nobuild --discard_analysis_cache --nokeep_incrementality_data \ + >& "$TEST_log" || fail "Expected success" + expect_log "--batch not specified with --nokeep_incrementality_data" } run_suite "test for --discard_graph_edges"