[Skymeld] Restructure the initialization and clearing of the various …

…conflict checking states.

Previously, these get unconditionally created (skymeld/noskymeld) but only used
and reset in Skymeld mode.

This CL would make it so that these states are:
- Only created in Skymeld mode, once per invocation
- Cleared at the end of analysis (to save memory) and unconditionally at the end of the build (to cover the case there analysis never finished)

PiperOrigin-RevId: 542568005
Change-Id: Ia75a3a3cf25592c02a5f9f1107c98cb8bb73e14c

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

@@ -611,12 +611,16 @@ private static void declareDependenciesAndCheckValues(
   ImmutableMap<ActionAnalysisMetadata, ConflictException> checkActionConflicts(
       ActionLookupKeyOrProxy actionLookupKey, boolean strictConflictCheck)
       throws InterruptedException {
+    IncrementalArtifactConflictFinder localRef = incrementalArtifactConflictFinder.get();
+    // a null value means that the conflict checker is shut down.
+    if (localRef == null) {
+      return ImmutableMap.of();
+    }
     ActionLookupValuesCollectionResult transitiveValueCollectionResult =
         transitiveActionLookupValuesHelper.collect(actionLookupKey);
 
     ImmutableMap<ActionAnalysisMetadata, ConflictException> conflicts =
-        incrementalArtifactConflictFinder
-            .get()
+        localRef
             .findArtifactConflicts(
                 transitiveValueCollectionResult.collectedValues(), strictConflictCheck)
             .getConflicts();
@@ -695,5 +699,10 @@ static ActionLookupValuesCollectionResult create(
       return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult(
           collectedValues, visitedKeys);
     }
+
+    static ActionLookupValuesCollectionResult empty() {
+      return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult(
+          ImmutableSet.of(), ImmutableSet.of());
+    }
   }
 }

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

@@ -660,7 +660,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           // Required for incremental correctness.
           // We unconditionally reset the states here instead of in #analysisFinishedCallback since
           // in case of --nokeep_going & analysis error, the analysis phase is never finished.
-          skyframeExecutor.resetIncrementalArtifactConflictFindingStates();
+          skyframeExecutor.clearIncrementalArtifactConflictFindingStates();
           skyframeExecutor.resetBuildDriverFunction();
           skyframeExecutor.setTestTypeResolver(null);
 
@@ -825,14 +825,18 @@ private void analysisFinishedCallback(
       throws InterruptedException {
     if (shouldPublishBuildGraphMetrics.get()) {
       // Now that we have the full picture, it's time to collect the metrics of the whole graph.
-      BuildGraphMetrics buildGraphMetrics =
+      BuildGraphMetrics.Builder buildGraphMetricsBuilder =
           skyframeExecutor
               .collectActionLookupValuesInBuild(
                   configuredTargetKeys, buildResultListener.getAnalyzedAspects().keySet())
-              .getMetrics()
-              .setOutputArtifactCount(skyframeExecutor.getOutputArtifactCount())
-              .build();
-      eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetrics));
+              .getMetrics();
+      IncrementalArtifactConflictFinder incrementalArtifactConflictFinder =
+          skyframeExecutor.getIncrementalArtifactConflictFinder();
+      if (incrementalArtifactConflictFinder != null) {
+        buildGraphMetricsBuilder.setOutputArtifactCount(
+            incrementalArtifactConflictFinder.getOutputArtifactCount());
+      }
+      eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetricsBuilder.build()));
     }
 
     if (shouldDiscardAnalysisCache) {
@@ -844,7 +848,7 @@ private void analysisFinishedCallback(
     // At this point, it's safe to clear objects related to action conflict checking.
     // Clearing the states here is a performance optimization (reduce peak heap size) and isn't
     // required for correctness.
-    skyframeExecutor.resetIncrementalArtifactConflictFindingStates();
+    skyframeExecutor.clearIncrementalArtifactConflictFindingStates();
 
     // Clearing the syscall cache here to free up some heap space.
     // TODO(b/273225564) Would this incur more CPU cost for the execution phase cache misses?

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

@@ -418,17 +418,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur
   private RemoteOutputsMode lastRemoteOutputsMode;
   private Boolean lastRemoteCacheEnabled;
 
+  // start: Skymeld-only
   // This is set once every build and set to null at the end of each.
   @Nullable private Supplier<Boolean> mergedSkyframeAnalysisExecutionSupplier;
 
   // Reset after each build.
-  private IncrementalArtifactConflictFinder incrementalArtifactConflictFinder;
+  @Nullable private IncrementalArtifactConflictFinder incrementalArtifactConflictFinder;
   // Reset after each build.
+  @Nullable
   private TransitiveActionLookupKeysCollector incrementalTransitiveActionLookupKeysCollector;
   // A set of ActionLookupKeys which have been confirmed to be conflict-free. This is used for
   // pruning while going through the ActionLookupKeys in the build for conflict checking.
   // Reset after each build.
   private Set<ActionLookupKey> conflictFreeActionLookupKeysGlobalSet;
+  // end: Skymeld-only
+
   private RuleContextConstraintSemantics ruleContextConstraintSemantics;
   private RegexFilter extraActionFilter;
   @Nullable private ActionExecutionInactivityWatchdog watchdog;
@@ -881,15 +885,6 @@ protected final void init() {
     memoizingEvaluator = createEvaluator(skyFunctions(), progressReceiver, emittedEventState);
     skyframeExecutorConsumerOnInit.accept(this);
 
-    // Initialize the various conflict-finding states. This is unconditionally created but only used
-    // with Skymeld i.e. --experimental_merged_skyframe_analysis_execution.
-    incrementalArtifactConflictFinder =
-        IncrementalArtifactConflictFinder.createWithActionGraph(
-            new MapBasedActionGraph(actionKeyContext));
-    conflictFreeActionLookupKeysGlobalSet = Sets.newConcurrentHashSet();
-    incrementalTransitiveActionLookupKeysCollector =
-        new TransitiveActionLookupKeysCollector(
-            SkyframeExecutorWrappingWalkableGraph.of(this), conflictFreeActionLookupKeysGlobalSet);
   }
 
   @ForOverride
@@ -2153,6 +2148,9 @@ EvaluationResult<SkyValue> evaluateBuildDriverKeys(
     checkActive();
     try {
       buildDriverFunction.setShouldCheckForConflict(shouldCheckForConflicts);
+      if (shouldCheckForConflicts.get()) {
+        initializeSkymeldConflictFindingStates();
+      }
       eventHandler.post(new ConfigurationPhaseStartedEvent(configuredTargetProgress));
       // For the workspace status actions.
       eventHandler.post(SomeExecutionStartedEvent.create());
@@ -2278,9 +2276,8 @@ public void resetBuildDriverFunction() {
     buildDriverFunction.resetStates();
   }
 
-  /** Resets the incremental artifact conflict finder to ensure incremental correctness. */
-  public void resetIncrementalArtifactConflictFindingStates() {
-    incrementalArtifactConflictFinder.shutdown();
+  // Initialize the various conflict-finding states. These are good for 1 invocation.
+  private void initializeSkymeldConflictFindingStates() {
     incrementalArtifactConflictFinder =
         IncrementalArtifactConflictFinder.createWithActionGraph(
             new MapBasedActionGraph(actionKeyContext));
@@ -2290,12 +2287,20 @@ public void resetIncrementalArtifactConflictFindingStates() {
             SkyframeExecutorWrappingWalkableGraph.of(this), conflictFreeActionLookupKeysGlobalSet);
   }
 
-  private IncrementalArtifactConflictFinder getIncrementalArtifactConflictFinder() {
-    return incrementalArtifactConflictFinder;
+  /** Clear the incremental conflict finding states to save memory. */
+  public void clearIncrementalArtifactConflictFindingStates() {
+    // Create a local ref for shutting down, in case there's a race.
+    IncrementalArtifactConflictFinder localRef = incrementalArtifactConflictFinder;
+    if (localRef != null) {
+      localRef.shutdown();
+    }
+    incrementalArtifactConflictFinder = null;
+    conflictFreeActionLookupKeysGlobalSet = Sets.newConcurrentHashSet();
+    incrementalTransitiveActionLookupKeysCollector = null;
   }
 
-  public int getOutputArtifactCount() {
-    return incrementalArtifactConflictFinder.getOutputArtifactCount();
+  public IncrementalArtifactConflictFinder getIncrementalArtifactConflictFinder() {
+    return incrementalArtifactConflictFinder;
   }
 
   private Set<ActionLookupKey> getConflictFreeActionLookupKeysGlobalSet() {
@@ -3183,8 +3188,13 @@ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOf
     try (SilentCloseable c =
         Profiler.instance().profile("SkyframeExecutor.collectTransitiveActionLookupValuesOfKey")) {
       if (tracksStateForIncrementality()) {
+        var localRef = incrementalTransitiveActionLookupKeysCollector;
+        // a null value means that the conflict checker is shut down.
+        if (localRef == null) {
+          return ActionLookupValuesCollectionResult.empty();
+        }
         ImmutableMap<ActionLookupKey, SkyValue> foundTransitiveActionLookupEntities =
-            incrementalTransitiveActionLookupKeysCollector.collect(key);
+            localRef.collect(key);
         return ActionLookupValuesCollectionResult.create(
             foundTransitiveActionLookupEntities.values(),
             ImmutableSet.copyOf(foundTransitiveActionLookupEntities.keySet()));