Only detect artifact conflicts between targets in current build.

Previously, two successive builds of targets A and B would fail if the two sets had conflicting actions. Now, only fail such builds if the targets from the current build has action conflicts.

RELNOTES: None
PiperOrigin-RevId: 206974011

2 files changed, 87 insertions, 22 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 3bb767fd94..0badd50025 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -81,6 +81,8 @@ import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.WalkableGraph;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
@@ -134,6 +136,9 @@ public final class SkyframeBuildView {
    */
   private boolean skyframeAnalysisWasDiscarded;
 
+  private ImmutableSet<SkyKey> topLevelKeys = ImmutableSet.of();
+  private boolean topLevelTargetsChanged;
+
   public SkyframeBuildView(BlazeDirectories directories,
       SkyframeExecutor skyframeExecutor, ConfiguredRuleClassProvider ruleClassProvider) {
     this.factory =
@@ -267,7 +272,17 @@ public final class SkyframeBuildView {
     ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions;
     try (SilentCloseable c =
         Profiler.instance().profile("skyframeExecutor.findArtifactConflicts")) {
-      badActions = skyframeExecutor.findArtifactConflicts();
+      setTopLevelTargetKeysForArtifactConflictChecking(values, aspectKeys);
+      badActions =
+          skyframeExecutor.findArtifactConflicts(
+              () ->
+                  // If we do not track incremental state we do not have graph edges,
+                  // so we cannot traverse the graph and find only actions in the current build.
+                  // In this case we can simply return all ActionLookupValues in the graph,
+                  // since the graph's lifetime is a single build anyway.
+                  skyframeExecutor.tracksStateForIncrementality()
+                      ? getActionLookupValuesInBuild(values, aspectKeys)
+                      : getActionLookupValuesInGraph());
     }
 
     Collection<AspectValue> goodAspects = Lists.newArrayListWithCapacity(values.size());
@@ -684,28 +699,34 @@ public final class SkyframeBuildView {
     anyConfiguredTargetDeleted = false;
   }
 
-  public boolean isSomeConfiguredTargetInvalidated() {
-    return anyConfiguredTargetDeleted || !dirtiedConfiguredTargetKeys.isEmpty();
+  private void setTopLevelTargetKeysForArtifactConflictChecking(
+      Collection<ConfiguredTargetKey> ctKeys, Collection<AspectValueKey> aspectKeys) {
+    ImmutableSet<SkyKey> newKeys =
+        ImmutableSet.<SkyKey>builder().addAll(ctKeys).addAll(aspectKeys).build();
+    topLevelTargetsChanged = !topLevelKeys.equals(newKeys);
+    topLevelKeys = newKeys;
   }
 
   /**
    * Called from SkyframeExecutor to see whether the graph needs to be checked for artifact
-   * conflicts. Returns true if some configured target has been evaluated since the last time the
-   * graph was checked for artifact conflicts (with that last time marked by a call to
-   * {@link #resetEvaluatedConfiguredTargetFlag()}).
+   * conflicts.
    */
-  boolean isSomeConfiguredTargetEvaluated() {
+  boolean shouldCheckArtifactConflicts() {
     Preconditions.checkState(!enableAnalysis);
-    return someConfiguredTargetEvaluated;
+    return topLevelTargetsChanged
+        || someConfiguredTargetEvaluated
+        || anyConfiguredTargetDeleted
+        || !dirtiedConfiguredTargetKeys.isEmpty();
   }
 
   /**
-   * Called from SkyframeExecutor after the graph is checked for artifact conflicts so that
-   * the next time {@link #isSomeConfiguredTargetEvaluated} is called, it will return true only if
-   * some configured target has been evaluated since the last check for artifact conflicts.
+   * Called from SkyframeExecutor after the graph is checked for artifact conflicts so that the next
+   * time {@link #shouldCheckArtifactConflicts} is called, it will return true only if some
+   * configured target has been evaluated since the last check for artifact conflicts.
    */
-  void resetEvaluatedConfiguredTargetFlag() {
+  void resetArtifactConflictState() {
     someConfiguredTargetEvaluated = false;
+    topLevelTargetsChanged = false;
   }
 
   /**
@@ -770,4 +791,48 @@ public final class SkyframeBuildView {
       }
     }
   }
+
+  // Finds every ActionLookupValue reachable from the top-level targets of the current build
+  private Iterable<ActionLookupValue> getActionLookupValuesInBuild(
+      Iterable<ConfiguredTargetKey> ctKeys, Iterable<AspectValueKey> aspectKeys)
+      throws InterruptedException {
+    WalkableGraph walkableGraph = SkyframeExecutorWrappingWalkableGraph.of(skyframeExecutor);
+    Set<SkyKey> seen = new HashSet<>();
+    List<ActionLookupValue> result = new ArrayList<>();
+    for (ConfiguredTargetKey key : ctKeys) {
+      findActionsRecursively(walkableGraph, key, seen, result);
+    }
+    for (AspectValueKey key : aspectKeys) {
+      findActionsRecursively(walkableGraph, key, seen, result);
+    }
+    return result;
+  }
+
+  private static void findActionsRecursively(
+      WalkableGraph walkableGraph, SkyKey key, Set<SkyKey> seen, List<ActionLookupValue> result)
+      throws InterruptedException {
+    if (!seen.add(key)) {
+      return;
+    }
+    SkyValue value = walkableGraph.getValue(key);
+    if (value == null) {
+      // This means the value failed to evaluate
+      return;
+    }
+    if (value instanceof ActionLookupValue) {
+      result.add((ActionLookupValue) value);
+    }
+    for (Map.Entry<SkyKey, Iterable<SkyKey>> deps :
+        walkableGraph.getDirectDeps(ImmutableList.of(key)).entrySet()) {
+      for (SkyKey dep : deps.getValue()) {
+        findActionsRecursively(walkableGraph, dep, seen, result);
+      }
+    }
+  }
+
+  // Returns every ActionLookupValue currently contained in the whole action graph
+  private Iterable<ActionLookupValue> getActionLookupValuesInGraph() {
+    return Iterables.filter(
+        skyframeExecutor.memoizingEvaluator.getDoneValues().values(), ActionLookupValue.class);
+  }
 }
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 3e93c6c133..a95bcd02f1 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
@@ -1243,31 +1243,31 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return multiCpuOptions.build();
   }
 
-  private Iterable<ActionLookupValue> getActionLookupValues() {
-    // This filter keeps subclasses of ActionLookupValue.
-    return Iterables.filter(memoizingEvaluator.getDoneValues().values(), ActionLookupValue.class);
-  }
-
   @SuppressWarnings({"unchecked", "rawtypes"})
   Map<SkyKey, ActionLookupValue> getActionLookupValueMap() {
     return (Map) Maps.filterValues(memoizingEvaluator.getDoneValues(),
         Predicates.instanceOf(ActionLookupValue.class));
   }
 
+  /** A supplier that can throw {@link InterruptedException}. */
+  protected interface ActionLookupValueSupplier {
+    Iterable<ActionLookupValue> get() throws InterruptedException;
+  }
+
   /**
    * Checks the actions in Skyframe for conflicts between their output artifacts. Delegates to
    * {@link SkyframeActionExecutor#findAndStoreArtifactConflicts} to do the work, since any
    * conflicts found will only be reported during execution.
    */
   protected ImmutableMap<ActionAnalysisMetadata, SkyframeActionExecutor.ConflictException>
-      findArtifactConflicts() throws InterruptedException {
-    if (skyframeBuildView.isSomeConfiguredTargetEvaluated()
-        || skyframeBuildView.isSomeConfiguredTargetInvalidated()) {
+      findArtifactConflicts(ActionLookupValueSupplier actionLookupValues)
+          throws InterruptedException {
+    if (skyframeBuildView.shouldCheckArtifactConflicts()) {
       // This operation is somewhat expensive, so we only do it if the graph might have changed in
       // some way -- either we analyzed a new target or we invalidated an old one.
       try (AutoProfiler p = AutoProfiler.logged("discovering artifact conflicts", logger)) {
-        skyframeActionExecutor.findAndStoreArtifactConflicts(getActionLookupValues());
-        skyframeBuildView.resetEvaluatedConfiguredTargetFlag();
+        skyframeActionExecutor.findAndStoreArtifactConflicts(actionLookupValues.get());
+        skyframeBuildView.resetArtifactConflictState();
         // The invalidated configured targets flag will be reset later in the evaluate() call.
       }
     }