diff options
9 files changed, 75 insertions, 66 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..57f0936a51 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 @@ -405,17 +405,18 @@ public class BuildRequestOptions extends OptionsBase { public boolean useActionCache; @Option( - name = "keep_incrementality_data", + name = "track_incremental_state", + oldName = "keep_incrementality_data", defaultValue = "true", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE}, help = - "If false, discard Blaze-internal data that allows for invalidation and re-evaluation " + "If false, Blaze will not persist 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." + + "will not have any incrementality with respect to this one. Usually you will want " + + "to specify --batch when setting this to false." ) - public boolean keepIncrementalityData; + public boolean trackIncrementalState; /** 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/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 107510fa67..2b22d606bf 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java @@ -239,7 +239,7 @@ public final class BuildTool { // graph beforehand if this option is specified, or add another option to wipe if desired // (SkyframeExecutor#handleConfiguredTargetChange should be sufficient). if (request.getBuildOptions().queryExpression != null) { - if (!env.getSkyframeExecutor().hasIncrementalState()) { + if (!env.getSkyframeExecutor().tracksStateForIncrementality()) { throw new ConfiguredTargetQueryCommandLineException( "Configured query is not allowed if incrementality state is not being kept"); } 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..006281e5e6 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.tracksStateForIncrementality()) { // 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/BuildSummaryStatsModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java index 6423f94fa2..2a1df16bf6 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java @@ -67,7 +67,7 @@ public class BuildSummaryStatsModule extends BlazeModule { @Override public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { enabled = env.getOptions().getOptions(ExecutionOptions.class).enableCriticalPathProfiling; - discardActions = !env.getSkyframeExecutor().hasIncrementalState(); + discardActions = !env.getSkyframeExecutor().tracksStateForIncrementality(); } @Subscribe 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 64bf1c2d74..34a2ebfcdd 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 @@ -620,7 +620,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..3aa1ce8da1 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 @@ -107,17 +107,13 @@ 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 - * memory but making subsequent builds not incremental. Also, each action will be dereferenced - * once it is executed, saving memory. + * If false, the graph will not store state useful for incremental builds, saving memory but + * leaving the graph un-reusable. Subsequent builds will therefore not be incremental. + * + * <p>Avoids storing edges entirely and dereferences each action after execution. */ - private IncrementalState incrementalState = IncrementalState.NORMAL; + private boolean trackIncrementalState = true; private boolean evaluatorNeedsReset = false; @@ -504,52 +500,63 @@ 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>track_incremental_state set to false. + * </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 oldState = trackIncrementalState; + + // First check if the incrementality state should be kept around during the build. boolean explicitlyRequestedNoIncrementalData = - requestOptions != null && !requestOptions.keepIncrementalityData; + requestOptions != null && !requestOptions.trackIncrementalState; boolean implicitlyRequestedNoIncrementalData = batch && viewOptions != null && viewOptions.discardAnalysisCache; - boolean discardingEdges = - explicitlyRequestedNoIncrementalData || implicitlyRequestedNoIncrementalData; + trackIncrementalState = + !explicitlyRequestedNoIncrementalData && !implicitlyRequestedNoIncrementalData; if (explicitlyRequestedNoIncrementalData != implicitlyRequestedNoIncrementalData) { if (requestOptions != null && !explicitlyRequestedNoIncrementalData) { eventHandler.handle( Event.warn( - "--batch and --discard_analysis_cache specified, but --nokeep_incrementality_data " + "--batch and --discard_analysis_cache specified, but --notrack_incremental_state " + "not specified: incrementality data is implicitly discarded, but you may need" - + " to specify --nokeep_incrementality_data in the future if you want to " + + " to specify --notrack_incremental_state 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 " + "--batch not specified with --notrack_incremental_state: the server will " + "remain running, but the next build will not be incremental on this one.")); } } - IncrementalState oldState = incrementalState; - incrementalState = - discardingEdges ? IncrementalState.CLEAR_EDGES_AND_ACTIONS : IncrementalState.NORMAL; - if (oldState != incrementalState) { - logger.info("Set incremental state to " + 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. + if (oldState != trackIncrementalState) { + logger.info("Set incremental state to " + trackIncrementalState); evaluatorNeedsReset = true; - removeActionsAfterEvaluation.set( - incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS); - } else if (incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS) { + removeActionsAfterEvaluation.set(!trackIncrementalState); + } else if (!trackIncrementalState) { evaluatorNeedsReset = true; } } @Override - public boolean hasIncrementalState() { - // TODO(bazel-team): Combine this method with clearSkyframeRelevantCaches() once legacy - // execution is removed [skyframe-execution]. - return incrementalState == IncrementalState.NORMAL; + public boolean tracksStateForIncrementality() { + return trackIncrementalState; } @Override @@ -618,7 +625,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { * phase. Instead, their analysis-time data is cleared while preserving the generating action info * needed for execution. The next build will delete the nodes (and recreate them if necessary). * - * <p>If {@link #hasIncrementalState} is false, then also delete loading-phase nodes (as + * <p>If {@link #tracksStateForIncrementality} is false, then also delete loading-phase nodes (as * determined by {@link #LOADING_TYPES}) from the graph, since there will be no future builds to * use them for. */ @@ -638,7 +645,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { } SkyKey key = keyAndEntry.getKey(); SkyFunctionName functionName = key.functionName(); - if (!hasIncrementalState() && LOADING_TYPES.contains(functionName)) { + if (!tracksStateForIncrementality() && LOADING_TYPES.contains(functionName)) { it.remove(); continue; } 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 f7926e2545..b23f49f909 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 @@ -604,7 +604,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { evaluatorDiffer(), progressReceiver, emittedEventState, - hasIncrementalState()); + tracksStateForIncrementality()); buildDriver = getBuildDriver(); } @@ -686,22 +686,20 @@ 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. } - public boolean hasIncrementalState() { + /** Whether this executor tracks state for the purpose of improving incremental performance. */ + public boolean tracksStateForIncrementality() { 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/shell/integration/discard_graph_edges_lib.sh b/src/test/shell/integration/discard_graph_edges_lib.sh index 683b96b4dd..0992d34433 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 --notrack_incremental_state" 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..0ec8559979 100755 --- a/src/test/shell/integration/discard_graph_edges_test.sh +++ b/src/test/shell/integration/discard_graph_edges_test.sh @@ -334,13 +334,13 @@ 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 \ + bazel $STARTUP_FLAGS --nobatch test $BUILD_FLAGS --track_incremental_state \ //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" \ + --track_incremental_state //testing:mytest >& "$TEST_log" \ || fail "Expected success" } @@ -348,7 +348,7 @@ 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="--notrack_incremental_state --discard_analysis_cache" test_packages_cleared STARTUP_FLAGS="$old_startup_flags" BUILD_FLAGS="$old_build_flags" @@ -356,7 +356,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 --track_incremental_state" test_packages_cleared BUILD_FLAGS="$old_build_flags" } @@ -365,21 +365,21 @@ 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="--notrack_incremental_state" 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" \ + bazel build --notrack_incremental_state //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 \ + bazel build --nobuild --notrack_incremental_state //testing:mytest \ >& "$TEST_log" || fail "Expected success" bazel query --noexperimental_ui --output=label_kind //testing:mytest \ >& "$TEST_log" || fail "Expected success" @@ -390,7 +390,7 @@ 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 \ + bazel build --nobuild --notrack_incremental_state //testing:mytest \ >& "$TEST_log" || fail "Expected success" bazel shutdown || fail "Expected success" readonly local new_server_pid="$(bazel info server_pid 2> /dev/null)" @@ -399,19 +399,19 @@ function test_shutdown_after_discard_incrementality_data() { } function test_clean_after_discard_incrementality_data() { - bazel build --nobuild --nokeep_incrementality_data //testing:mytest \ + bazel build --nobuild --notrack_incremental_state //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)" + --notrack_incremental_state server_pid 2> /dev/null)" [[ -z "$server_pid" ]] && fail "Couldn't get server pid" - bazel test --noexperimental_ui --nokeep_incrementality_data \ + bazel test --noexperimental_ui --notrack_incremental_state \ //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" - bazel test --noexperimental_ui --nokeep_incrementality_data \ + bazel test --noexperimental_ui --notrack_incremental_state \ //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" bazel test --noexperimental_ui //testing:mytest >& "$TEST_log" \ @@ -420,7 +420,7 @@ 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 \ + bazel test --noexperimental_ui --notrack_incremental_state \ //testing:mytest >& "$TEST_log" || fail "Expected success" expect_log "Loading package: testing" readonly local new_server_pid="$(bazel info server_pid 2> /dev/null)" @@ -431,10 +431,10 @@ 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 \ + expect_log "--batch and --discard_analysis_cache specified, but --notrack_incremental_state not specified" + bazel build --nobuild --discard_analysis_cache --notrack_incremental_state \ >& "$TEST_log" || fail "Expected success" - expect_log "--batch not specified with --nokeep_incrementality_data" + expect_log "--batch not specified with --notrack_incremental_state" } run_suite "test for --discard_graph_edges" |