[Skymeld] Avoid running conflict checking when unnecessary.

This CL provides a way for BuildDriverFunction#compute to look at the state of
the build and decide whether a conflict check is necessary. This decision is
made using the very same SkyframeBuildView#shouldCheckForConflicts method.

PiperOrigin-RevId: 523685723
Change-Id: I042d3201889dc33aded34414b9eb9918e37bf0c1

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

@@ -21,7 +21,6 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -77,6 +76,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /**
@@ -88,6 +88,8 @@ public class BuildDriverFunction implements SkyFunction {
   private final Supplier<RuleContextConstraintSemantics> ruleContextConstraintSemantics;
   private final Supplier<RegexFilter> extraActionFilterSupplier;
 
+  @Nullable private Supplier<Boolean> shouldCheckForConflict;
+
   // A set of BuildDriverKeys that have been checked for conflicts.
   // This gets cleared after each build.
   // We can't use SkyKeyComputeState here since it doesn't guarantee that the same state for
@@ -127,6 +129,11 @@ private static class State implements SkyKeyComputeState {
     private boolean checkedForCompatibility = false;
     private boolean checkedForPlatformCompatibility = false;
   }
+
+  public void setShouldCheckForConflict(Supplier<Boolean> shouldCheckForConflict) {
+    this.shouldCheckForConflict = shouldCheckForConflict;
+  }
+
   /**
    * From the ConfiguredTarget/Aspect keys, get the top-level artifacts. Then evaluate them together
    * with the appropriate CompletionFunctions. This is the bridge between the conceptual analysis &
@@ -156,12 +163,11 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       return null;
     }
 
-    // Unconditionally check for action conflicts.
-    // TODO(b/214371092): Only check when necessary.
-    try (SilentCloseable c =
-        Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) {
-      // We only check for action conflict once per BuildDriverKey.
-      if (checkedForConflicts.add(buildDriverKey)) {
+    // We only check for action conflict once per BuildDriverKey.
+    if (Preconditions.checkNotNull(shouldCheckForConflict).get()
+        && checkedForConflicts.add(buildDriverKey)) {
+      try (SilentCloseable c =
+          Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) {
         ImmutableMap<ActionAnalysisMetadata, ConflictException> actionConflicts =
             checkActionConflicts(actionLookupKey, buildDriverKey.strictActionConflictCheck());
         if (!actionConflicts.isEmpty()) {
@@ -171,7 +177,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                       + actionLookupKey.getLabel(),
                   actionConflicts));
         }
-
       }
     }
 

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

@@ -556,10 +556,6 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
             .addAll(ctKeys)
             .addAll(topLevelAspectsKeys)
             .build();
-    boolean checkingForConflict = shouldCheckForConflicts(checkForActionConflicts, newKeys);
-    if (checkingForConflict) {
-      largestTopLevelKeySetCheckedForConflicts = newKeys;
-    }
 
     ImmutableList<Artifact> workspaceStatusArtifacts =
         skyframeExecutor.getWorkspaceStatusArtifacts(eventHandler);
@@ -613,7 +609,9 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
                     skyframeExecutor,
                     ctKeys,
                     shouldDiscardAnalysisCache,
-                    /* measuredAnalysisTime= */ analysisWorkTimer.stop().elapsed().toMillis()),
+                    /* measuredAnalysisTime= */ analysisWorkTimer.stop().elapsed().toMillis(),
+                    /* shouldPublishBuildGraphMetrics= */ () ->
+                        shouldCheckForConflicts(checkForActionConflicts, newKeys)),
             /* executionGoAheadCallback= */ executor::launchQueuedUpExecutionPhaseTasks)) {
 
       try {
@@ -630,7 +628,8 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
                   buildDriverAspectKeys,
                   keepGoing,
                   executors.executionParallelism(),
-                  executor);
+                  executor,
+                  () -> shouldCheckForConflicts(checkForActionConflicts, newKeys));
         } finally {
           // Required for incremental correctness.
           // We unconditionally reset the states here instead of in #analysisFinishedCallback since
@@ -668,6 +667,11 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
                         /* includeExecutionPhase= */ true)
                     .executionDetailedExitCode());
           }
+          // These attributes affect whether conflict checking will be done during the next build.
+          if (shouldCheckForConflicts(checkForActionConflicts, newKeys)) {
+            largestTopLevelKeySetCheckedForConflicts = newKeys;
+          }
+          someActionLookupValueEvaluated = false;
         }
 
         // The exclusive tests whose analysis succeeded i.e. those that can be run.
@@ -782,17 +786,21 @@ private void analysisFinishedCallback(
       SkyframeExecutor skyframeExecutor,
       List<ConfiguredTargetKey> configuredTargetKeys,
       boolean shouldDiscardAnalysisCache,
-      long measuredAnalysisTime)
+      long measuredAnalysisTime,
+      Supplier<Boolean> shouldPublishBuildGraphMetrics)
       throws InterruptedException {
-    // Now that we have the full picture, it's time to collect the metrics of the whole graph.
-    BuildGraphMetrics buildGraphMetrics =
-        skyframeExecutor
-            .collectActionLookupValuesInBuild(
-                configuredTargetKeys, buildResultListener.getAnalyzedAspects().keySet())
-            .getMetrics()
-            .setOutputArtifactCount(skyframeExecutor.getOutputArtifactCount())
-            .build();
-    eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetrics));
+
+    if (shouldPublishBuildGraphMetrics.get()) {
+      // Now that we have the full picture, it's time to collect the metrics of the whole graph.
+      BuildGraphMetrics buildGraphMetrics =
+          skyframeExecutor
+              .collectActionLookupValuesInBuild(
+                  configuredTargetKeys, buildResultListener.getAnalyzedAspects().keySet())
+              .getMetrics()
+              .setOutputArtifactCount(skyframeExecutor.getOutputArtifactCount())
+              .build();
+      eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetrics));
+    }
 
     if (shouldDiscardAnalysisCache) {
       clearAnalysisCache(

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

@@ -2344,10 +2344,12 @@ EvaluationResult<BuildDriverValue> evaluateBuildDriverKeys(
       Set<BuildDriverKey> buildDriverAspectKeys,
       boolean keepGoing,
       int executionParallelism,
-      QuiescingExecutor executor)
+      QuiescingExecutor executor,
+      Supplier<Boolean> shouldCheckForConflicts)
       throws InterruptedException {
     checkActive();
     try {
+      buildDriverFunction.setShouldCheckForConflict(shouldCheckForConflicts);
       eventHandler.post(new ConfigurationPhaseStartedEvent(configuredTargetProgress));
       EvaluationContext evaluationContext =
           newEvaluationContextBuilder()

src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java

@@ -17,6 +17,7 @@
 import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.eventbus.EventBus;
 import com.google.common.eventbus.Subscribe;
 import com.google.devtools.build.lib.actions.BuildFailedException;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
@@ -48,6 +49,7 @@ public class MetricsCollectorTest extends BuildIntegrationTestCase {
   static class BuildMetricsEventListener extends BlazeModule {
 
     private BuildMetricsEvent event;
+    private EventBus eventBus;
 
     @Override
     public void beforeCommand(CommandEnvironment env) {
@@ -601,7 +603,7 @@ public void testActionData() throws Exception {
   }
 
   @Test
-  public void skymeldNullIncrementalBuild_buildGraphMetricsCollected() throws Exception {
+  public void skymeldNullIncrementalBuild_buildGraphMetricsNotCollected() throws Exception {
     write(
         "foo/BUILD",
         "genrule(",
@@ -635,11 +637,17 @@ public void skymeldNullIncrementalBuild_buildGraphMetricsCollected() throws Exce
     // Null build.
     buildTarget("//foo:foo", "//foo:bar");
 
-    // For Skymeld, we expect BuildGraphMetrics to include all the entities that were touched in the
-    // build. This is not true for regular blaze: the metric is only collected if there's conflict
-    // checking.
+    BuildGraphMetrics expectedNullBuild =
+        BuildGraphMetrics.newBuilder()
+            .setActionLookupValueCount(0)
+            .setActionLookupValueCountNotIncludingAspects(0)
+            .setActionCount(0)
+            .setActionCountNotIncludingAspects(0)
+            .setInputFileConfiguredTargetCount(0)
+            .setOutputArtifactCount(0)
+            .build();
     assertThat(buildMetricsEventListener.event.getBuildMetrics().getBuildGraphMetrics())
         .comparingExpectedFieldsOnly()
-        .isEqualTo(expected);
+        .isEqualTo(expectedNullBuild);
   }
 }