Fix regression in clean build caused by 09874b5 in critical path comp…

…uting.

Previously, we always query skyframe for missing components. In clean build, this is  a waste since there is no change pruning.

This CL fixes that by only query skyframe if there might be change pruning, indicated by the newly added event FilesModifiedEvent.

PiperOrigin-RevId: 696809273
Change-Id: I026ca3d668afc3401d203afcb0084166422ba495

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -593,6 +593,11 @@ java_library(
     ],
 )
 
+java_library(
+    name = "files_modified_event",
+    srcs = ["FilesModifiedEvent.java"],
+)
+
 java_library(
     name = "build_setting_provider",
     srcs = ["BuildSettingProvider.java"],

src/main/java/com/google/devtools/build/lib/analysis/FilesModifiedEvent.java

@@ -0,0 +1,19 @@
+// Copyright 2024 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.analysis;
+
+/**
+ * An event that is fired after Bazel detected modified source, or output, files from last build.
+ */
+public record FilesModifiedEvent(int numModifiedFiles) {}

src/main/java/com/google/devtools/build/lib/metrics/criticalpath/BUILD

@@ -17,6 +17,7 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/analysis:files_modified_event",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/concurrent",

src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputer.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.metrics.criticalpath;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Comparators;
 import com.google.common.collect.ImmutableList;
@@ -35,13 +36,15 @@
 import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
 import com.google.devtools.build.lib.actions.SpawnMetrics;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.analysis.FilesModifiedEvent;
 import com.google.devtools.build.lib.skyframe.rewinding.ActionRewoundEvent;
 import com.google.devtools.build.skyframe.WalkableGraph;
 import java.time.Duration;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BinaryOperator;
@@ -79,6 +82,8 @@ public class CriticalPathComputer {
   private final ActionKeyContext actionKeyContext;
   @Nullable private final WalkableGraph graph;
 
+  private final AtomicBoolean queryGraph = new AtomicBoolean(false);
+
   /** Maximum critical path found. */
   private final AtomicReference<CriticalPathComponent> maxCriticalPath = new AtomicReference<>();
 
@@ -134,6 +139,18 @@ public Map<Artifact, CriticalPathComponent> getCriticalPathComponentsMap() {
     return outputArtifactToComponent;
   }
 
+  @VisibleForTesting
+  void setQueryGraph(boolean queryGraph) {
+    this.queryGraph.set(queryGraph);
+  }
+
+  @Subscribe
+  public void onFilesModified(FilesModifiedEvent event) {
+    // Only allow querying the graph if we have modified files from last build: only in this case
+    // change-pruning could happen.
+    queryGraph.set(event.numModifiedFiles() > 0);
+  }
+
   /** Changes the phase of the action */
   @Subscribe
   @AllowConcurrentEvents
@@ -364,7 +381,7 @@ private void addArtifactDependency(
       throws InterruptedException {
     CriticalPathComponent depComponent = outputArtifactToComponent.get(input);
 
-    if (depComponent == null && !input.isSourceArtifact() && graph != null) {
+    if (depComponent == null && !input.isSourceArtifact() && graph != null && queryGraph.get()) {
       // The generating action of the input is missing. It happens when the action was change
       // pruned in an incremental build. Query skyframe for the Action data.
       if (Actions.getGeneratingAction(graph, input) instanceof Action action) {

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -267,6 +267,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:constraints/incompatible_target_checker",
         "//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind",
         "//src/main/java/com/google/devtools/build/lib/analysis:exec_group_collection",
+        "//src/main/java/com/google/devtools/build/lib/analysis:files_modified_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_null_config_exception",

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
+import com.google.devtools.build.lib.analysis.FilesModifiedEvent;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.actions.TemplateExpansionException;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
@@ -496,6 +497,7 @@ public void detectModifiedOutputFiles(
     Profiler.instance()
         .logSimpleTask(startTime, stopTime, ProfilerTask.INFO, "detectModifiedOutputFiles");
     long duration = stopTime - startTime;
+    getEventBus().post(new FilesModifiedEvent(modifiedFiles.get()));
     outputTreeDiffCheckingDuration = duration > 0 ? Duration.ofNanos(duration) : Duration.ZERO;
   }
 

src/test/java/com/google/devtools/build/lib/metrics/criticalpath/BUILD

@@ -39,5 +39,6 @@ java_test(
         "//third_party:junit4",
         "//third_party:mockito",
         "//third_party:truth",
+        "@maven//:com_google_testparameterinjector_test_parameter_injector",
     ],
 )

src/test/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputerTest.java

@@ -61,6 +61,8 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.WalkableGraph;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.Collections;
@@ -70,10 +72,9 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
 /** Unit tests for {@link CriticalPathComputer}. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
 public class CriticalPathComputerTest extends FoundationTestCase {
 
   private ManualClock clock;
@@ -939,7 +940,7 @@ public void toleratesCriticalPathInconsistency() throws Exception {
   }
 
   @Test
-  public void testChangePruning() throws Exception {
+  public void testChangePruning(@TestParameter boolean queryGraph) throws Exception {
     MockAction action1 =
         new MockAction(ImmutableSet.of(), ImmutableSet.of(derivedArtifact("test/action1.out")));
     MockAction action2 =
@@ -1020,6 +1021,7 @@ public Map<SkyKey, Pair<SkyValue, Iterable<SkyKey>>> getValueAndRdeps(
                 throw new UnsupportedOperationException();
               }
             });
+    computer.setQueryGraph(queryGraph);
 
     // Action 1 - 0s - 1s
     long action1Start = clock.nanoTime();
@@ -1056,24 +1058,44 @@ public Map<SkyKey, Pair<SkyValue, Iterable<SkyKey>>> getValueAndRdeps(
             new FakeActionInputFileCache(),
             mock(ActionLookupData.class)));
 
-    // The total run time should be 6s (Action 1 + Action 2 + Action 4) since Action 3 is
-    // change-pruned.
-    assertThat(computer.getMaxCriticalPath().getAggregatedElapsedTime())
-        .isEqualTo(Duration.ofSeconds(6));
-    AggregatedCriticalPath criticalPath = computer.aggregate();
-    assertThat(criticalPath.components()).hasSize(4);
-    // Action 4 has a run time of 2 seconds
-    assertThat(criticalPath.components().get(0).prettyPrintAction()).contains("action4.out");
-    assertThat(criticalPath.components().get(0).getElapsedTime()).isEqualTo(Duration.ofSeconds(3));
-    // Action 3 has a run time of 0 seconds
-    assertThat(criticalPath.components().get(1).prettyPrintAction()).contains("action3.out");
-    assertThat(criticalPath.components().get(1).getElapsedTime()).isEqualTo(Duration.ZERO);
-    // Action 2 has a run time of 2 seconds
-    assertThat(criticalPath.components().get(2).prettyPrintAction()).contains("action2.out");
-    assertThat(criticalPath.components().get(2).getElapsedTime()).isEqualTo(Duration.ofSeconds(2));
-    // Action 1 has a run time of 2 seconds
-    assertThat(criticalPath.components().get(3).prettyPrintAction()).contains("action1.out");
-    assertThat(criticalPath.components().get(3).getElapsedTime()).isEqualTo(Duration.ofSeconds(1));
+    if (queryGraph) {
+      // The total run time should be 6s (Action 1 + Action 2 + Action 4) since Action 3 is
+      // change-pruned.
+      assertThat(computer.getMaxCriticalPath().getAggregatedElapsedTime())
+          .isEqualTo(Duration.ofSeconds(6));
+      AggregatedCriticalPath criticalPath = computer.aggregate();
+      assertThat(criticalPath.components()).hasSize(4);
+      // Action 4 has a run time of 3 seconds
+      assertThat(criticalPath.components().get(0).prettyPrintAction()).contains("action4.out");
+      assertThat(criticalPath.components().get(0).getElapsedTime())
+          .isEqualTo(Duration.ofSeconds(3));
+      // Action 3 has a run time of 0 seconds
+      assertThat(criticalPath.components().get(1).prettyPrintAction()).contains("action3.out");
+      assertThat(criticalPath.components().get(1).getElapsedTime()).isEqualTo(Duration.ZERO);
+      // Action 2 has a run time of 2 seconds
+      assertThat(criticalPath.components().get(2).prettyPrintAction()).contains("action2.out");
+      assertThat(criticalPath.components().get(2).getElapsedTime())
+          .isEqualTo(Duration.ofSeconds(2));
+      // Action 1 has a run time of 1 seconds
+      assertThat(criticalPath.components().get(3).prettyPrintAction()).contains("action1.out");
+      assertThat(criticalPath.components().get(3).getElapsedTime())
+          .isEqualTo(Duration.ofSeconds(1));
+    } else {
+      // The total run time should be 3s (Action 1 + Action 4) since Action 3 is change-pruned and
+      // queryGraph is false.
+      assertThat(computer.getMaxCriticalPath().getAggregatedElapsedTime())
+          .isEqualTo(Duration.ofSeconds(4));
+      AggregatedCriticalPath criticalPath = computer.aggregate();
+      assertThat(criticalPath.components()).hasSize(2);
+      // Action 4 has a run time of 3 seconds
+      assertThat(criticalPath.components().get(0).prettyPrintAction()).contains("action4.out");
+      assertThat(criticalPath.components().get(0).getElapsedTime())
+          .isEqualTo(Duration.ofSeconds(3));
+      // Action 1 has a run time of 1 seconds
+      assertThat(criticalPath.components().get(1).prettyPrintAction()).contains("action1.out");
+      assertThat(criticalPath.components().get(1).getElapsedTime())
+          .isEqualTo(Duration.ofSeconds(1));
+    }
   }
 
   private void simulateActionExec(Action action, int totalTime) throws InterruptedException {