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

5 files changed, 55 insertions, 107 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 d769c68ece..d193bb3064 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.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;
@@ -405,51 +404,18 @@ 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 = "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 2e5f599692..9f20f76622 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
-          || !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 @@ 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 2c11eac9e0..54552a6c5f 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 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 e6c4137327..f3e4bf4da0 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.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;
@@ -108,14 +107,17 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
 
   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 @@ 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);
-    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(
+          incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS);
+    } else if (incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS) {
       evaluatorNeedsReset = true;
-      removeActionsAfterEvaluation.set(discardingEdges);
     }
   }
 
   @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 c91a693029..da4d4efef1 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   /**
-   * 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 @@ 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,