Rename --keep_incrementality_data to --track_incremental_state.

New name clears the namespace a 2nd flag that will wipe the build graph after the build. The old name would be confusing as it could easily apply to that, and so needs to be more specifically just about tracking state in the first place. The new flag can be clearly separate and about keeping state after the build.

Partial roll forward of https://github.com/bazelbuild/bazel/commit/9321316b34767b06c3071b2cf2a4de189874fcce, with fixes to documentation that are still relevant.

RELNOTES: Rename --keep_incrementality_data to --track_incremental_state
PiperOrigin-RevId: 179078292

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"