When tracking the critical path, if not keeping incremental state, don't keep references to actions indefinitely. Instead, once an action is finished executing, keep just some metadata about it. This allows actions to be unconditionally dropped when running with --batch, --discard_analysis_cache, and --keep_going, even if profiling is enabled.

The additional fields here add between 8 and 12 bytes per component, and we have one component per action. This additional penalty is only incurred when we are already saving memory, so I think it's ok. The full penalty will be realized only towards the end of the build, when every action has started executing at least once. Users can still specify --noexperimental_enable_critical_path_profiling if they want to squeeze even more memory out.

PiperOrigin-RevId: 152328870

8 files changed, 157 insertions, 43 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AbstractCriticalPathComponent.java b/src/main/java/com/google/devtools/build/lib/runtime/AbstractCriticalPathComponent.java
index 35c642455e..8c383b2292 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/AbstractCriticalPathComponent.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/AbstractCriticalPathComponent.java
@@ -14,6 +14,8 @@
 package com.google.devtools.build.lib.runtime;
 
 import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.actions.ActionOwner;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.util.Clock;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -29,13 +31,13 @@ public class AbstractCriticalPathComponent<C extends AbstractCriticalPathCompone
   // These two fields are values of BlazeClock.nanoTime() at the relevant points in time.
   private long startNanos;
   private long finishNanos = 0;
-  protected volatile boolean isRunning = true;
+  volatile boolean isRunning = true;
 
   /** We keep here the critical path time for the most expensive child. */
   private long childAggregatedElapsedTime = 0;
 
-  /** The action for which we are storing the stat. */
-  private final Action action;
+  /** May be nulled out after finished running to allow the action to be GC'ed. */
+  @Nullable protected Action action;
 
   /**
    * Child with the maximum critical path.
@@ -68,11 +70,35 @@ public class AbstractCriticalPathComponent<C extends AbstractCriticalPathCompone
     return false;
   }
 
-  /** The action for which we are storing the stat. */
-  public Action getAction() {
+  /**
+   * The action for which we are storing the stat. May be null if the action has finished running.
+   */
+  @Nullable
+  public final Action maybeGetAction() {
     return action;
   }
 
+  public String prettyPrintAction() {
+    return getActionNotNull().prettyPrint();
+  }
+
+  @Nullable
+  public Label getOwner() {
+    ActionOwner owner = getActionNotNull().getOwner();
+    if (owner != null && owner.getLabel() != null) {
+      return owner.getLabel();
+    }
+    return null;
+  }
+
+  public String getMnemonic() {
+    return getActionNotNull().getMnemonic();
+  }
+
+  private Action getActionNotNull() {
+    return Preconditions.checkNotNull(action, this);
+  }
+
   /**
    * Add statistics for one dependency of this action. Caller should ensure {@code dep} not
    * running.
@@ -95,7 +121,7 @@ public class AbstractCriticalPathComponent<C extends AbstractCriticalPathCompone
   }
 
   long getElapsedTimeNanos() {
-    Preconditions.checkState(!isRunning, "Still running %s", action);
+    Preconditions.checkState(!isRunning, "Still running %s", this);
     return finishNanos - startNanos;
   }
 
@@ -109,7 +135,7 @@ public class AbstractCriticalPathComponent<C extends AbstractCriticalPathCompone
   }
 
   long getAggregatedElapsedTimeNanos() {
-    Preconditions.checkState(!isRunning, "Still running %s", action);
+    Preconditions.checkState(!isRunning, "Still running %s", this);
     return getElapsedTimeNanos() + childAggregatedElapsedTime;
   }
 
@@ -132,7 +158,7 @@ public class AbstractCriticalPathComponent<C extends AbstractCriticalPathCompone
     if (!isRunning) {
       currentTime = String.format("%.2f", getElapsedTimeMillis() / 1000.0) + "s ";
     }
-    return currentTime + action.describe();
+    return currentTime + prettyPrintAction();
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ActionDiscardingCriticalPathComponent.java b/src/main/java/com/google/devtools/build/lib/runtime/ActionDiscardingCriticalPathComponent.java
new file mode 100644
index 0000000000..200846fa68
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ActionDiscardingCriticalPathComponent.java
@@ -0,0 +1,64 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.runtime;
+
+import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.cmdline.Label;
+import javax.annotation.Nullable;
+
+/**
+ * {@link SimpleCriticalPathComponent} that clears its {@link Action} reference after the component
+ * is finished running. This allows the action to be GC'ed, if other Bazel components are configured
+ * to also release their references to the action.
+ *
+ * <p>This class is separate from {@link SimpleCriticalPathComponent} for memory reasons: the
+ * additional references here add between 8 and 12 bytes per component instance with compressed OOPS
+ * and 24 bytes without compressed OOPS, which is a price we'd rather not pay unless the user
+ * explicitly enables this clearing of {@link Action} objects.
+ */
+class ActionDiscardingCriticalPathComponent extends SimpleCriticalPathComponent {
+  @Nullable private final Label owner;
+  private final String prettyPrint;
+  private final String mnemonic;
+
+  ActionDiscardingCriticalPathComponent(Action action, long relativeStartNanos) {
+    super(action, relativeStartNanos);
+    this.prettyPrint = super.prettyPrintAction();
+    this.owner = super.getOwner();
+    this.mnemonic = super.getMnemonic();
+  }
+
+  @Override
+  public String getMnemonic() {
+    return mnemonic;
+  }
+
+  @Override
+  public String prettyPrintAction() {
+    return prettyPrint;
+  }
+
+  @Nullable
+  @Override
+  public Label getOwner() {
+    return owner;
+  }
+
+  @Override
+  public synchronized boolean finishActionExecution(long startNanos, long finishNanos) {
+    boolean result = super.finishActionExecution(startNanos, finishNanos);
+    this.action = null;
+    return result;
+  }
+}
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 705a00ab15..0061e3f937 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
@@ -41,6 +41,7 @@ public class BuildSummaryStatsModule extends BlazeModule {
   private EventBus eventBus;
   private Reporter reporter;
   private boolean enabled;
+  private boolean discardActions;
 
   @Override
   public void beforeCommand(Command command, CommandEnvironment env) {
@@ -59,12 +60,13 @@ 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();
   }
 
   @Subscribe
   public void executionPhaseStarting(ExecutionStartingEvent event) {
     if (enabled) {
-      criticalPathComputer = new SimpleCriticalPathComputer(BlazeClock.instance());
+      criticalPathComputer = new SimpleCriticalPathComputer(BlazeClock.instance(), discardActions);
       eventBus.register(criticalPathComputer);
     }
   }
@@ -88,10 +90,12 @@ public class BuildSummaryStatsModule extends BlazeModule {
         // when the actions were executed while critical path computation is stored in the reverse
         // way.
         for (SimpleCriticalPathComponent stat : criticalPath.components().reverse()) {
-          Profiler.instance().logSimpleTaskDuration(
-              stat.getStartNanos(),
-              stat.getElapsedTimeNanos(),
-              ProfilerTask.CRITICAL_PATH_COMPONENT, stat.getAction());
+          Profiler.instance()
+              .logSimpleTaskDuration(
+                  stat.getStartNanos(),
+                  stat.getElapsedTimeNanos(),
+                  ProfilerTask.CRITICAL_PATH_COMPONENT,
+                  stat.prettyPrintAction());
         }
         Profiler.instance().completeTask(ProfilerTask.CRITICAL_PATH);
       }
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 46c133fcc8..cb2c80416b 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
@@ -32,7 +32,6 @@ import com.google.devtools.build.lib.analysis.config.DefaultsPackage;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.OutputService;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
@@ -572,8 +571,7 @@ public final class CommandEnvironment {
     // Let skyframe figure out if it needs to store graph edges for this build.
     skyframeExecutor.decideKeepIncrementalState(
         runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).batch,
-        optionsParser.getOptions(BuildView.Options.class),
-        optionsParser.getOptions(ExecutionOptions.class));
+        optionsParser.getOptions(BuildView.Options.class));
 
     // Start the performance and memory profilers.
     runtime.beforeCommand(this, options, execStartTimeNanos);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
index ad6d27ed83..0702fd198f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
@@ -51,6 +51,7 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
   /** Maximum critical path found. */
   private C maxCriticalPath;
   private final Clock clock;
+  protected final boolean discardActions;
 
   /**
    * The list of slowest individual components, ignoring the time to build dependencies.
@@ -69,8 +70,9 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
 
   private final Object lock = new Object();
 
-  protected CriticalPathComputer(Clock clock) {
+  protected CriticalPathComputer(Clock clock, boolean discardActions) {
     this.clock = clock;
+    this.discardActions = discardActions;
     maxCriticalPath = null;
   }
 
@@ -122,17 +124,43 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
    * @return The component to be used for updating the time stats.
    */
   private C tryAddComponent(C newComponent) {
-    Action newAction = newComponent.getAction();
+    Action newAction = Preconditions.checkNotNull(newComponent.maybeGetAction(), newComponent);
     Artifact primaryOutput = newAction.getPrimaryOutput();
     C storedComponent = outputArtifactToComponent.putIfAbsent(primaryOutput, newComponent);
 
     if (storedComponent != null) {
-      if (!Actions.canBeShared(newAction, storedComponent.getAction())) {
-        throw new IllegalStateException("Duplicate output artifact found for unsharable actions."
-            + "This could happen  if a previous event registered the action.\n"
-            + "Old action: " + storedComponent.getAction() + "\n\n"
-            + "New action: " + newAction + "\n\n"
-            + "Artifact: " + primaryOutput + "\n");
+      Action oldAction = storedComponent.maybeGetAction();
+      if (oldAction != null) {
+        if (!Actions.canBeShared(newAction, oldAction)) {
+          throw new IllegalStateException(
+              "Duplicate output artifact found for unsharable actions."
+                  + "This can happen if a previous event registered the action.\n"
+                  + "Old action: "
+                  + oldAction
+                  + "\n\nNew action: "
+                  + newAction
+                  + "\n\nArtifact: "
+                  + primaryOutput
+                  + "\n");
+        }
+      } else {
+        String mnemonic = storedComponent.getMnemonic();
+        String prettyPrint = storedComponent.prettyPrintAction();
+        if (!newAction.getMnemonic().equals(mnemonic)
+            || !newAction.prettyPrint().equals(prettyPrint)) {
+          throw new IllegalStateException(
+              "Duplicate output artifact found for unsharable actions."
+                  + "This can happen if a previous event registered the action.\n"
+                  + "Old action mnemonic and prettyPrint: "
+                  + mnemonic
+                  + ", "
+                  + prettyPrint
+                  + "\n\nNew action: "
+                  + newAction
+                  + "\n\nArtifact: "
+                  + primaryOutput
+                  + "\n");
+        }
       }
     } else {
       storedComponent = newComponent;
@@ -197,12 +225,12 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
   }
 
   private void finalizeActionStat(long startTimeNanos, Action action, C component) {
-    boolean updated = component.finishActionExecution(startTimeNanos, clock.nanoTime());
 
     for (Artifact input : action.getInputs()) {
       addArtifactDependency(component, input);
     }
 
+    boolean updated = component.finishActionExecution(startTimeNanos, clock.nanoTime());
     synchronized (lock) {
       if (isBiggestCriticalPath(component)) {
         maxCriticalPath = component;
@@ -243,10 +271,10 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
   private void addArtifactDependency(C actionStats, Artifact input) {
     C depComponent = outputArtifactToComponent.get(input);
     if (depComponent != null) {
-      if (depComponent.isRunning) {
+      Action action = depComponent.maybeGetAction();
+      if (depComponent.isRunning && action != null) {
         // Rare case that an action depending on a previously-cached shared action sees a different
         // shared action that is in the midst of being an action cache hit.
-        Action action = depComponent.getAction();
         for (Artifact actionOutput : action.getOutputs()) {
           if (input.equals(actionOutput)
               && Objects.equals(input.getArtifactOwner(), actionOutput.getArtifactOwner())) {
@@ -255,7 +283,7 @@ public abstract class CriticalPathComputer<C extends AbstractCriticalPathCompone
             throw new IllegalStateException(
                 String.format(
                     "Cannot add critical path stats when the action is not finished. %s. %s. %s",
-                    input, actionStats.getAction(), depComponent.getAction()));
+                    input, actionStats.prettyPrintAction(), action));
           }
         }
         return;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/SimpleCriticalPathComputer.java b/src/main/java/com/google/devtools/build/lib/runtime/SimpleCriticalPathComputer.java
index 9ceefa979b..df80bc143d 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/SimpleCriticalPathComputer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/SimpleCriticalPathComputer.java
@@ -24,13 +24,15 @@ public class SimpleCriticalPathComputer
     extends CriticalPathComputer<SimpleCriticalPathComponent,
         AggregatedCriticalPath<SimpleCriticalPathComponent>> {
 
-  public SimpleCriticalPathComputer(Clock clock) {
-    super(clock);
+  SimpleCriticalPathComputer(Clock clock, boolean discardActions) {
+    super(clock, discardActions);
   }
 
   @Override
   public SimpleCriticalPathComponent createComponent(Action action, long relativeStartNanos) {
-    return new SimpleCriticalPathComponent(action, relativeStartNanos);
+    return discardActions
+        ? new ActionDiscardingCriticalPathComponent(action, relativeStartNanos)
+        : new SimpleCriticalPathComponent(action, relativeStartNanos);
   }
 
   /**
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 aed5573cdf..e6b18384e7 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
@@ -26,7 +26,7 @@ import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
-import com.google.devtools.build.lib.analysis.BuildView;
+import com.google.devtools.build.lib.analysis.BuildView.Options;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
@@ -35,7 +35,6 @@ import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
@@ -97,7 +96,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
 
   private enum IncrementalState {
     NORMAL,
-    CLEAR_EDGES,
     CLEAR_EDGES_AND_ACTIONS
   }
 
@@ -550,8 +548,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
   }
 
   @Override
-  public void decideKeepIncrementalState(
-      boolean batch, BuildView.Options viewOptions, ExecutionOptions executionOptions) {
+  public void decideKeepIncrementalState(boolean batch, Options viewOptions) {
     Preconditions.checkState(!active);
     if (viewOptions == null) {
       // Some blaze commands don't include the view options. Don't bother with them.
@@ -562,10 +559,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
           incrementalState == IncrementalState.NORMAL,
           "May only be called once if successful: %s",
           incrementalState);
-      incrementalState =
-          executionOptions.enableCriticalPathProfiling
-              ? IncrementalState.CLEAR_EDGES
-              : IncrementalState.CLEAR_EDGES_AND_ACTIONS;
+      incrementalState = IncrementalState.CLEAR_EDGES_AND_ACTIONS;
       // Graph will be recreated on next sync.
       LOG.info("Set incremental state to " + incrementalState);
     }
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 fa7fccc86c..b5bda26ca8 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
@@ -80,7 +80,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.OutputService;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -659,8 +658,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    *       way).
    * </ol>
    */
-  public void decideKeepIncrementalState(
-      boolean batch, Options viewOptions, ExecutionOptions executionOptions) {
+  public void decideKeepIncrementalState(boolean batch, Options viewOptions) {
     // Assume incrementality.
   }