commit 2fdac7e4b0d40254474489142fd6b045f9985487
author jhorvitz <jhorvitz@google.com> Tue Apr 05 08:13:26 2022 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Apr 05 08:14:53 2022 -0700
tree eda636db804b563a5f011cabe0507f9e52398310
parent 9c98120f33579b72561e02826d9fccf222eccb3c

Remove the critical path consistency check.

The invariant that an action producing an input is not running when an action consuming that input completes may be violated by action rewinding. Instead of conditionally disabling the check when rewinding is enabled, just remove it.

PiperOrigin-RevId: 439584625

site/en/BUILD

diff --git a/site/en/BUILD b/site/en/BUILD
index d24c281..47cf127 100644
--- a/site/en/BUILD
+++ b/site/en/BUILD
@@ -1,5 +1,3 @@
-package(default_applicable_licenses = ["//third_party/devsite/bazel:license"])
-
 licenses(["notice"])
 
 exports_files(

src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java

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 875e088..a322f12 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
@@ -79,21 +79,13 @@
   private final ActionKeyContext actionKeyContext;
 
   /** Maximum critical path found. */
-  private final AtomicReference<CriticalPathComponent> maxCriticalPath;
+  private final AtomicReference<CriticalPathComponent> maxCriticalPath = new AtomicReference<>();
 
   private final Clock clock;
-  private final boolean checkCriticalPathInconsistencies;
-
-  public CriticalPathComputer(
-      ActionKeyContext actionKeyContext, Clock clock, boolean checkCriticalPathInconsistencies) {
-    this.actionKeyContext = actionKeyContext;
-    this.clock = clock;
-    maxCriticalPath = new AtomicReference<>();
-    this.checkCriticalPathInconsistencies = checkCriticalPathInconsistencies;
-  }
 
   public CriticalPathComputer(ActionKeyContext actionKeyContext, Clock clock) {
-    this(actionKeyContext, clock, /*checkCriticalPathInconsistencies=*/ true);
+    this.actionKeyContext = actionKeyContext;
+    this.clock = clock;
   }
 
   /**
@@ -217,8 +209,8 @@
 
   private Stream<CriticalPathComponent> uniqueActions() {
     return outputArtifactToComponent.entrySet().stream()
-        .filter((e) -> e.getValue().isPrimaryOutput(e.getKey()))
-        .map((e) -> e.getValue());
+        .filter(e -> e.getValue().isPrimaryOutput(e.getKey()))
+        .map(Map.Entry::getValue);
   }
 
   /** Creates a CriticalPathComponent and adds the duration of input discovery and changes phase. */
@@ -349,7 +341,7 @@
   }
 
   /** Maximum critical path component found during the build. */
-  protected CriticalPathComponent getMaxCriticalPath() {
+  CriticalPathComponent getMaxCriticalPath() {
     return maxCriticalPath.get();
   }
 
@@ -373,36 +365,12 @@
   private void addArtifactDependency(
       CriticalPathComponent actionStats, Artifact input, long componentFinishNanos) {
     CriticalPathComponent depComponent = outputArtifactToComponent.get(input);
-    if (depComponent != null) {
-      if (depComponent.isRunning()) {
-        checkCriticalPathInconsistency(
-            (Artifact.DerivedArtifact) input, depComponent.getAction(), actionStats);
-        return;
-      }
+    // Typically, the dep component should already be finished since its output was used as an input
+    // for a just-completed action. However, we tolerate it still running for (a) action rewinding
+    // and (b) the 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.
+    if (depComponent != null && !depComponent.isRunning()) {
       actionStats.addDepInfo(depComponent, componentFinishNanos);
     }
   }
-
-  private void checkCriticalPathInconsistency(
-      Artifact.DerivedArtifact input, Action dependencyAction, CriticalPathComponent actionStats) {
-    if (!checkCriticalPathInconsistencies) {
-      return;
-    }
-    // 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.
-    for (Artifact actionOutput : dependencyAction.getOutputs()) {
-      if (input.equals(actionOutput)
-          && input
-              .getGeneratingActionKey()
-              .equals(((Artifact.DerivedArtifact) actionOutput).getGeneratingActionKey())) {
-        // This (currently running) dependency action is the same action that produced the input for
-        // the finished actionStats CriticalPathComponent. This should be impossible.
-        throw new IllegalStateException(
-            String.format(
-                "Cannot add critical path stats when the dependency action is not finished. "
-                    + "%s. %s. %s.",
-                input, actionStats.prettyPrintAction(), dependencyAction));
-      }
-    }
-  }
 }