Remove support for --discard_actions_after_execution.

The memory savings from this flag are not worth the complexity, and it interferes with action restarting.

RELNOTES: Remove support for --discard_actions_after_execution.
PiperOrigin-RevId: 201077905

6 files changed, 19 insertions, 71 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 034f6c2670..d5dbb8b686 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -446,11 +446,6 @@ public abstract class AbstractAction implements Action, ActionApi {
     return MiddlemanType.NORMAL;
   }
 
-  @Override
-  public boolean canRemoveAfterExecution() {
-    return true;
-  }
-
   /** If the action might create directories as outputs this method must be called. */
   protected void checkOutputsForDirectories(ActionExecutionContext actionExecutionContext) {
     for (Artifact output : getOutputs()) {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index 72c881c4ef..4c27425f91 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -182,8 +182,6 @@ public interface Action extends ActionExecutionMetadata {
    */
   boolean showsOutputUnconditionally();
 
-  boolean canRemoveAfterExecution();
-
   /**
    * Called by {@link com.google.devtools.build.lib.analysis.extra.ExtraAction} at execution time to
    * extract information from this action into a protocol buffer to be used by extra_action rules.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java
index 08b0c6d55d..0c81217d4a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java
@@ -14,14 +14,12 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -31,16 +29,8 @@ import javax.annotation.Nullable;
  */
 public abstract class ActionLookupValue implements SkyValue {
 
-  @AutoCodec.VisibleForSerialization protected final boolean removeActionsAfterEvaluation;
-
-  public ActionLookupValue(boolean removeActionsAfterEvaluation) {
-    this.removeActionsAfterEvaluation = removeActionsAfterEvaluation;
-  }
-
-  /**
-   * Returns a list of actions registered by this {@link SkyValue}.
-   */
-  protected abstract ArrayList<ActionAnalysisMetadata> getActions();
+  /** Returns a list of actions registered by this {@link SkyValue}. */
+  protected abstract ImmutableList<ActionAnalysisMetadata> getActions();
 
   /**
    * Returns a map where keys are artifacts generated by this {@link SkyValue}, and values are
@@ -63,8 +53,7 @@ public abstract class ActionLookupValue implements SkyValue {
   /**
    * Returns the action that generates {@code artifact}, if known to this value, or null. This
    * method should be avoided. Call it only when the action is really needed, and it is known to be
-   * present, either because the execution phase has not started, or because {@link
-   * Action#canRemoveAfterExecution} is known to be false for the action being requested.
+   * present.
    */
   @Nullable
   public ActionAnalysisMetadata getGeneratingActionDangerousReadJavadoc(Artifact artifact) {
@@ -120,25 +109,6 @@ public abstract class ActionLookupValue implements SkyValue {
     return getActions().size();
   }
 
-  /**
-   * If this object was initialized with {@code removeActionsAfterEvaluation} and {@link
-   * Action#canRemoveAfterExecution()} is true for {@code action}, then remove this action from this
-   * object's index as a memory-saving measure. The {@code artifact -> index} mapping remains
-   * intact, so this action's execution value can still be addressed by its inputs.
-   */
-  @ThreadSafe
-  public void actionEvaluated(int actionIndex, Action action) {
-    if (removeActionsAfterEvaluation && action.canRemoveAfterExecution()) {
-      // This method may concurrently mutate an ArrayList, which is unsafe on its face. However,
-      // ArrayList mutation on different indices that does not affect the size of the ArrayList is
-      // safe, and that is what does this code does.
-      ArrayList<ActionAnalysisMetadata> actionArrayList = getActions();
-      ActionAnalysisMetadata oldAction = actionArrayList.set(actionIndex, null);
-      Preconditions.checkState(
-          action.equals(oldAction), "Not same: %s %s %s %s", action, oldAction, this, actionIndex);
-    }
-  }
-
 
   /**
    * All subclasses of ActionLookupValue "own" artifacts with {@link ArtifactOwner}s that are
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
index 3311ca34ba..a3a6ba6b58 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
@@ -29,7 +29,6 @@ import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
@@ -118,7 +117,7 @@ public final class Actions {
    * @throws ActionConflictException iff there are two actions generate the same output
    */
   public static GeneratingActions findAndThrowActionConflict(
-      ActionKeyContext actionKeyContext, List<ActionAnalysisMetadata> actions)
+      ActionKeyContext actionKeyContext, ImmutableList<ActionAnalysisMetadata> actions)
       throws ActionConflictException {
     return Actions.maybeFilterSharedActionsAndThrowIfConflict(
         actionKeyContext, actions, /*allowSharedAction=*/ false);
@@ -135,7 +134,7 @@ public final class Actions {
    *     output
    */
   public static GeneratingActions filterSharedActionsAndThrowActionConflict(
-      ActionKeyContext actionKeyContext, List<? extends ActionAnalysisMetadata> actions)
+      ActionKeyContext actionKeyContext, ImmutableList<ActionAnalysisMetadata> actions)
       throws ActionConflictException {
     return Actions.maybeFilterSharedActionsAndThrowIfConflict(
         actionKeyContext, actions, /*allowSharedAction=*/ true);
@@ -143,7 +142,7 @@ public final class Actions {
 
   private static GeneratingActions maybeFilterSharedActionsAndThrowIfConflict(
       ActionKeyContext actionKeyContext,
-      List<? extends ActionAnalysisMetadata> actions,
+      ImmutableList<ActionAnalysisMetadata> actions,
       boolean allowSharedAction)
       throws ActionConflictException {
     Map<Artifact, Integer> generatingActions = new HashMap<>();
@@ -338,11 +337,11 @@ public final class Actions {
     public static final GeneratingActions EMPTY =
         new GeneratingActions(ImmutableList.of(), ImmutableMap.of());
 
-    private final List<? extends ActionAnalysisMetadata> actions;
+    private final ImmutableList<ActionAnalysisMetadata> actions;
     private final ImmutableMap<Artifact, Integer> generatingActionIndex;
 
     private GeneratingActions(
-        List<? extends ActionAnalysisMetadata> actions,
+        ImmutableList<ActionAnalysisMetadata> actions,
         ImmutableMap<Artifact, Integer> generatingActionIndex) {
       this.actions = actions;
       this.generatingActionIndex = generatingActionIndex;
@@ -362,7 +361,7 @@ public final class Actions {
       return generatingActionIndex;
     }
 
-    public List<? extends ActionAnalysisMetadata> getActions() {
+    public ImmutableList<ActionAnalysisMetadata> getActions() {
       return actions;
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java
index 7ac35c75b3..d75143b567 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java
@@ -16,43 +16,36 @@ package com.google.devtools.build.lib.actions;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.MoreObjects.ToStringHelper;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import java.util.ArrayList;
 
 /**
  * Basic implementation of {@link ActionLookupValue} where the value itself owns and maintains
  * the list of generating actions.
  */
 public class BasicActionLookupValue extends ActionLookupValue {
-  protected final ArrayList<ActionAnalysisMetadata> actions;
+  protected final ImmutableList<ActionAnalysisMetadata> actions;
   @VisibleForSerialization protected final ImmutableMap<Artifact, Integer> generatingActionIndex;
 
-  protected BasicActionLookupValue(
-      ActionAnalysisMetadata action,
-      boolean removeActionAfterEvaluation) {
-    this(Actions.GeneratingActions.fromSingleAction(action), removeActionAfterEvaluation);
+  protected BasicActionLookupValue(ActionAnalysisMetadata action) {
+    this(Actions.GeneratingActions.fromSingleAction(action));
   }
 
   protected BasicActionLookupValue(
-      ArrayList<ActionAnalysisMetadata> actions,
-      ImmutableMap<Artifact, Integer> generatingActionIndex,
-      boolean removeActionsAfterEvaluation) {
-    super(removeActionsAfterEvaluation);
+      ImmutableList<ActionAnalysisMetadata> actions,
+      ImmutableMap<Artifact, Integer> generatingActionIndex) {
     this.actions = actions;
     this.generatingActionIndex = generatingActionIndex;
   }
 
   @VisibleForTesting
-  public BasicActionLookupValue(
-      Actions.GeneratingActions generatingActions, boolean removeActionsAfterEvaluation) {
-    this(new ArrayList<>(generatingActions.getActions()),
-        generatingActions.getGeneratingActionIndex(),
-        removeActionsAfterEvaluation);
+  public BasicActionLookupValue(Actions.GeneratingActions generatingActions) {
+    this(generatingActions.getActions(), generatingActions.getGeneratingActionIndex());
   }
 
   @Override
-  protected ArrayList<ActionAnalysisMetadata> getActions() {
+  protected ImmutableList<ActionAnalysisMetadata> getActions() {
     return actions;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
index 15c500766c..d95bbfba66 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
@@ -86,13 +86,6 @@ public final class MiddlemanAction extends AbstractAction {
   }
 
   @Override
-  public boolean canRemoveAfterExecution() {
-    // Aggregating middleman actions' inputs are needed by their output artifacts, and the type of
-    // the middleman isn't known without having the action, so we just make sure it stays.
-    return false;
-  }
-
-  @Override
   protected String getRawProgressMessage() {
     return null; // users don't really want to know about Middlemen.
   }