diff options
8 files changed, 138 insertions, 84 deletions
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 d193bb3064..d769c68ece 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,6 +20,7 @@ import com.google.devtools.build.lib.util.OptionsUtils; 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; @@ -404,18 +405,51 @@ public class BuildRequestOptions extends OptionsBase { ) 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 = "keep_incrementality_data", - defaultValue = "true", + name = "incremental_state_retention_strategy", + defaultValue = "KEEP_FOREVER", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE}, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + converter = IncrementalStateRetentionStrategyConverter.class, help = - "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." + "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." ) - public boolean keepIncrementalityData; + public IncrementalStateRetentionStrategy incrementalityStateRetentionStrategy; /** 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 9f20f76622..2e5f599692 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 @@ public class ExecutionTool { skyframeExecutor.drainChangedFiles(); if (request.getViewOptions().discardAnalysisCache - || !request.getBuildOptions().keepIncrementalityData) { + || !skyframeExecutor.hasIncrementalState()) { // Free memory by removing cache entries that aren't going to be needed. env.getSkyframeBuildView() .clearAnalysisCache(analysisResult.getTargetsToBuild(), analysisResult.getAspects()); @@ -666,7 +666,7 @@ public class ExecutionTool { 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 54552a6c5f..2c11eac9e0 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 @@ public final class CommandEnvironment { // Fail fast in the case where a Blaze command forgets to install the package path correctly. skyframeExecutor.setActive(false); - // Let skyframe figure out if it needs to store graph edges for this build. + // Let skyframe figure out how much incremental state it will be keeping. 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 f3e4bf4da0..e6c4137327 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,6 +34,7 @@ import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory; 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; @@ -107,17 +108,14 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { private boolean lastAnalysisDiscarded = false; - private enum IncrementalState { - NORMAL, - CLEAR_EDGES_AND_ACTIONS - } - /** - * If {@link IncrementalState#CLEAR_EDGES_AND_ACTIONS}, the graph will not store edges, saving + * Assume that this build is incremental. If {@link + * IncrementalStateRetentionStrategy#DISCARD_EAGERLY}, 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 IncrementalState incrementalState = IncrementalState.NORMAL; + private IncrementalStateRetentionStrategy incrementalState = + IncrementalStateRetentionStrategy.KEEP_FOREVER; private boolean evaluatorNeedsReset = false; @@ -504,52 +502,71 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { 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); - boolean explicitlyRequestedNoIncrementalData = - requestOptions != null && !requestOptions.keepIncrementalityData; - boolean implicitlyRequestedNoIncrementalData = + 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 = batch && viewOptions != null && viewOptions.discardAnalysisCache; boolean discardingEdges = - 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.")); - } + 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.")); } - IncrementalState oldState = incrementalState; + + // 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 = - discardingEdges ? IncrementalState.CLEAR_EDGES_AND_ACTIONS : IncrementalState.NORMAL; - if (oldState != incrementalState) { - logger.info("Set incremental state to " + incrementalState); - evaluatorNeedsReset = true; - removeActionsAfterEvaluation.set( - incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS); - } else if (incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS) { + 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); evaluatorNeedsReset = true; + removeActionsAfterEvaluation.set(discardingEdges); } } @Override public boolean hasIncrementalState() { - // TODO(bazel-team): Combine this method with clearSkyframeRelevantCaches() once legacy - // execution is removed [skyframe-execution]. - return incrementalState == IncrementalState.NORMAL; + return !incrementalState.equals(IncrementalStateRetentionStrategy.DISCARD_EAGERLY); } @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 da4d4efef1..c91a693029 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,21 +686,19 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } /** - * 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: + * Decides if graph edges should be stored during this evaluation and checks if the state from the + * last evaluation, if any, can be kept. * - * <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> + * <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. */ 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; } @@ -1858,6 +1856,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 aaf3569686..f1a002fec1 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 @@ public final class EventCollectionApparatus { private List<EventHandler> handlers = new ArrayList<>(); /** - * Determine which events the {@link #collector()} created by this apparatus - * will collect. Default: {@link EventKind#ERRORS_AND_WARNINGS}. + * Determine which events the {@link #collector()} created by this apparatus will collect. + * Default: {@link EventKind#ERRORS_WARNINGS_AND_INFO}. */ 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 683b96b4dd..e7f97f9bbc 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 --nokeep_incrementality_data" +BUILD_FLAGS="--discard_analysis_cache --incremental_state_retention_strategy=discard_eagerly" 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 4b0311c6a7..dd806725bc 100755 --- a/src/test/shell/integration/discard_graph_edges_test.sh +++ b/src/test/shell/integration/discard_graph_edges_test.sh @@ -334,21 +334,22 @@ EOF # 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 --keep_incrementality_data \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel $STARTUP_FLAGS --nobatch test $BUILD_FLAGS \ + --incremental_state_retention_strategy=keep_forever //testing:mytest \ + >& "$TEST_log" || fail "Expected success" } function test_no_discard_analysis_cache() { bazel $STARTUP_FLAGS test $BUILD_FLAGS --nodiscard_analysis_cache \ - --keep_incrementality_data //testing:mytest >& "$TEST_log" \ - || fail "Expected success" + --incremental_state_retention_strategy=keep_forever //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="--nokeep_incrementality_data --discard_analysis_cache" + BUILD_FLAGS="--incremental_state_retention_strategy=discard_eagerly --discard_analysis_cache" test_packages_cleared STARTUP_FLAGS="$old_startup_flags" BUILD_FLAGS="$old_build_flags" @@ -356,7 +357,7 @@ function test_packages_cleared_nobatch() { function test_packages_cleared_implicit_noincrementality_data() { readonly local old_build_flags="$BUILD_FLAGS" - BUILD_FLAGS="$BUILD_FLAGS --keep_incrementality_data" + BUILD_FLAGS="$BUILD_FLAGS --incremental_state_retention_strategy=keep_forever" test_packages_cleared BUILD_FLAGS="$old_build_flags" } @@ -365,22 +366,22 @@ function test_actions_deleted_after_execution_nobatch_keep_analysis () { readonly local old_startup_flags="$STARTUP_FLAGS" STARTUP_FLAGS="--nobatch" readonly local old_build_flags="$BUILD_FLAGS" - BUILD_FLAGS="--nokeep_incrementality_data" + BUILD_FLAGS="--incremental_state_retention_strategy=discard_eagerly" test_actions_deleted_after_execution STARTUP_FLAGS="$old_startup_flags" BUILD_FLAGS="$old_build_flags" } function test_dump_after_discard_incrementality_data() { - bazel build --nokeep_incrementality_data //testing:mytest >& "$TEST_log" \ - || fail "Expected success" + bazel build --incremental_state_retention_strategy=discard_eagerly \ + //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 --nokeep_incrementality_data //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ + //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" @@ -390,8 +391,8 @@ function test_query_after_discard_incrementality_data() { 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 --nokeep_incrementality_data //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ + //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" ]] \ @@ -399,20 +400,22 @@ function test_shutdown_after_discard_incrementality_data() { } function test_clean_after_discard_incrementality_data() { - bazel build --nobuild --nokeep_incrementality_data //testing:mytest \ - >& "$TEST_log" || fail "Expected success" + bazel build --nobuild --incremental_state_retention_strategy=discard_eagerly \ + //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 \ - --nokeep_incrementality_data server_pid 2> /dev/null)" + --incremental_state_retention_strategy=discard_eagerly server_pid 2> /dev/null)" [[ -z "$server_pid" ]] && fail "Couldn't get server pid" - bazel test --noexperimental_ui --nokeep_incrementality_data \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui \ + --incremental_state_retention_strategy=discard_eagerly //testing:mytest \ + >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" - bazel test --noexperimental_ui --nokeep_incrementality_data \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui \ + --incremental_state_retention_strategy=discard_eagerly //testing:mytest \ + >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" bazel test --noexperimental_ui //testing:mytest >& "$TEST_log" \ || fail "Expected success" @@ -420,8 +423,9 @@ function test_switch_back_and_forth() { bazel test --noexperimental_ui //testing:mytest >& "$TEST_log" \ || fail "Expected success" expect_not_log "Loading package: testing" - bazel test --noexperimental_ui --nokeep_incrementality_data \ - //testing:mytest >& "$TEST_log" || fail "Expected success" + bazel test --noexperimental_ui \ + --incremental_state_retention_strategy=discard_eagerly //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" ]] \ @@ -431,10 +435,8 @@ function test_switch_back_and_forth() { 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 --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" + expect_log \ + "--batch and --discard_analysis_cache specified, but --incremental_state_retention_strategy not set to discard_eagerly" } run_suite "test for --discard_graph_edges" |