When no new configured targets have been analyzed, only check for artifact conflicts if the current set of configured targets is not a subset of the largest set of configured targets that have been checked for conflicts.

Also rework the flow between SkyframeBuildView and SkyframeActionExecutor to remove the SkyframeExecutor middleman.

Also reword the error message in case of an ArtifactPrefixConflictException, since a clean should no longer be necessary.

PiperOrigin-RevId: 207322139

7 files changed, 49 insertions, 94 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactPrefixConflictException.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPrefixConflictException.java
index 243c73c499..8ecd13e1a7 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactPrefixConflictException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPrefixConflictException.java
@@ -28,7 +28,7 @@ public class ArtifactPrefixConflictException extends Exception {
         String.format(
             "output path '%s' (belonging to %s) is a prefix of output path '%s' (belonging to %s). "
                 + "These actions cannot be simultaneously present; please rename one of the output "
-                + "files or, as a last resort, run 'clean' and then build just one of them",
+                + "files or build just one of them",
             firstPath, firstOwner, secondPath, secondOwner));
   }
 }
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 0badd50025..9b879db1e7 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
@@ -101,6 +101,7 @@ public final class SkyframeBuildView {
   private final ConfiguredTargetFactory factory;
   private final ArtifactFactory artifactFactory;
   private final SkyframeExecutor skyframeExecutor;
+  private final SkyframeActionExecutor skyframeActionExecutor;
   private boolean enableAnalysis = false;
 
   // This hack allows us to see when a configured target has been invalidated, and thus when the set
@@ -136,11 +137,14 @@ public final class SkyframeBuildView {
    */
   private boolean skyframeAnalysisWasDiscarded;
 
-  private ImmutableSet<SkyKey> topLevelKeys = ImmutableSet.of();
-  private boolean topLevelTargetsChanged;
+  private ImmutableSet<SkyKey> largestTopLevelKeySetCheckedForConflicts = ImmutableSet.of();
 
-  public SkyframeBuildView(BlazeDirectories directories,
-      SkyframeExecutor skyframeExecutor, ConfiguredRuleClassProvider ruleClassProvider) {
+  public SkyframeBuildView(
+      BlazeDirectories directories,
+      SkyframeExecutor skyframeExecutor,
+      ConfiguredRuleClassProvider ruleClassProvider,
+      SkyframeActionExecutor skyframeActionExecutor) {
+    this.skyframeActionExecutor = skyframeActionExecutor;
     this.factory =
         new ConfiguredTargetFactory(ruleClassProvider, skyframeExecutor.getDefaultBuildOptions());
     this.artifactFactory =
@@ -269,21 +273,34 @@ public final class SkyframeBuildView {
     } finally {
       enableAnalysis(false);
     }
-    ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions;
     try (SilentCloseable c =
         Profiler.instance().profile("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());
+      ImmutableSet<SkyKey> newKeys =
+          ImmutableSet.<SkyKey>builderWithExpectedSize(values.size() + aspectKeys.size())
+              .addAll(values)
+              .addAll(aspectKeys)
+              .build();
+      if (someConfiguredTargetEvaluated
+          || anyConfiguredTargetDeleted
+          || !dirtiedConfiguredTargetKeys.isEmpty()
+          || !largestTopLevelKeySetCheckedForConflicts.containsAll(newKeys)) {
+        largestTopLevelKeySetCheckedForConflicts = newKeys;
+        // 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 or are building
+        // targets together that haven't been built before.
+        skyframeActionExecutor.findAndStoreArtifactConflicts(
+            // 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());
+        someConfiguredTargetEvaluated = false;
+      }
     }
+    ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions =
+        skyframeActionExecutor.badActions();
 
     Collection<AspectValue> goodAspects = Lists.newArrayListWithCapacity(values.size());
     Root singleSourceRoot = skyframeExecutor.getForcedSingleSourceRootIfNoExecrootSymlinkCreation();
@@ -699,36 +716,6 @@ public final class SkyframeBuildView {
     anyConfiguredTargetDeleted = false;
   }
 
-  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.
-   */
-  boolean shouldCheckArtifactConflicts() {
-    Preconditions.checkState(!enableAnalysis);
-    return topLevelTargetsChanged
-        || someConfiguredTargetEvaluated
-        || anyConfiguredTargetDeleted
-        || !dirtiedConfiguredTargetKeys.isEmpty();
-  }
-
-  /**
-   * 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 resetArtifactConflictState() {
-    someConfiguredTargetEvaluated = false;
-    topLevelTargetsChanged = false;
-  }
-
   /**
    * {@link #createConfiguredTarget} will only create configured targets if this is set to true. It
    * should be set to true before any Skyframe update call that might call into {@link
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 36ad359ba8..ce65b6d646 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
@@ -19,7 +19,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
-import com.google.common.base.Predicates;
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Throwables;
 import com.google.common.cache.Cache;
@@ -31,7 +30,6 @@ import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Range;
 import com.google.common.eventbus.EventBus;
@@ -113,7 +111,6 @@ import com.google.devtools.build.lib.pkgcache.TargetParsingPhaseTimeEvent;
 import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
-import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction;
@@ -228,7 +225,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   private final AtomicInteger numPackagesLoaded = new AtomicInteger(0);
   @Nullable private final PackageProgressReceiver packageProgress;
 
-  protected SkyframeBuildView skyframeBuildView;
+  private final SkyframeBuildView skyframeBuildView;
   private ActionLogBufferPathGenerator actionLogBufferPathGenerator;
 
   protected BuildDriver buildDriver;
@@ -387,10 +384,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
     this.ruleClassProvider = pkgFactory.getRuleClassProvider();
     this.defaultBuildOptions = defaultBuildOptions;
-    this.skyframeBuildView = new SkyframeBuildView(
-        directories,
-        this,
-        (ConfiguredRuleClassProvider) ruleClassProvider);
+    this.skyframeBuildView =
+        new SkyframeBuildView(
+            directories,
+            this,
+            (ConfiguredRuleClassProvider) ruleClassProvider,
+            skyframeActionExecutor);
     this.artifactFactory = artifactResolverSupplier;
     this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
     this.externalFilesHelper = ExternalFilesHelper.create(
@@ -1253,37 +1252,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return new BuildConfigurationCollection(topLevelTargetConfigs, hostConfig);
   }
 
-  @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(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(actionLookupValues.get());
-        skyframeBuildView.resetArtifactConflictState();
-        // The invalidated configured targets flag will be reset later in the evaluate() call.
-      }
-    }
-    return skyframeActionExecutor.badActions();
-  }
-
   /**
    * Asks the Skyframe evaluator to build the given artifacts and targets, and to test the
    * given test targets.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorWrappingWalkableGraph.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorWrappingWalkableGraph.java
index 75a8a03d8a..e99efe1245 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorWrappingWalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorWrappingWalkableGraph.java
@@ -33,7 +33,7 @@ public class SkyframeExecutorWrappingWalkableGraph extends DelegatingWalkableGra
           @Override
           public NodeEntry get(@Nullable SkyKey requestor, Reason reason, SkyKey key)
               throws InterruptedException {
-            return evaluator.getGraphMap().get(key);
+            return evaluator.getExistingEntryAtLatestVersion(key);
           }
 
           @Override
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index b5c06b7bfb..e8f9a91648 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -299,7 +299,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   @Override
   @Nullable
   public SkyValue getExistingValue(SkyKey key) {
-    NodeEntry entry = getExistingEntryForTesting(key);
+    NodeEntry entry = getExistingEntryAtLatestVersion(key);
     try {
       return isDone(entry) ? entry.getValue() : null;
     } catch (InterruptedException e) {
@@ -309,7 +309,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
 
   @Override
   @Nullable public ErrorInfo getExistingErrorForTesting(SkyKey key) {
-    NodeEntry entry = getExistingEntryForTesting(key);
+    NodeEntry entry = getExistingEntryAtLatestVersion(key);
     try {
       return isDone(entry) ? entry.getErrorInfo() : null;
     } catch (InterruptedException e) {
@@ -319,7 +319,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
 
   @Nullable
   @Override
-  public NodeEntry getExistingEntryForTesting(SkyKey key) {
+  public NodeEntry getExistingEntryAtLatestVersion(SkyKey key) {
     return graph.get(null, Reason.OTHER, key);
   }
 
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index adff0d0589..cc0332af6f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -123,6 +123,9 @@ public interface MemoizingEvaluator {
   @Nullable
   SkyValue getExistingValue(SkyKey key) throws InterruptedException;
 
+  @Nullable
+  NodeEntry getExistingEntryAtLatestVersion(SkyKey key) throws InterruptedException;
+
   /**
    * Returns an error if and only if an earlier call to {@link #evaluate} created it; null
    * otherwise.
@@ -134,9 +137,6 @@ public interface MemoizingEvaluator {
   @Nullable
   ErrorInfo getExistingErrorForTesting(SkyKey key) throws InterruptedException;
 
-  @Nullable
-  NodeEntry getExistingEntryForTesting(SkyKey key) throws InterruptedException;
-
   /**
    * Tests that want finer control over the graph being used may provide a {@code transformer} here.
    * This {@code transformer} will be applied to the graph for each invalidation/evaluation.
diff --git a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
index 51724cdc05..9a4ad9e168 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
@@ -67,6 +67,6 @@ public class SequentialBuildDriver implements BuildDriver {
   @Nullable
   @Override
   public NodeEntry getEntryForTesting(SkyKey key) throws InterruptedException {
-    return memoizingEvaluator.getExistingEntryForTesting(key);
+    return memoizingEvaluator.getExistingEntryAtLatestVersion(key);
   }
 }