Automated rollback of commit f309ad3be36363070e87eef0ee04b12f4956d601.

*** Reason for rollback ***

Fixed duplicate derived inputs bug. Test is in diffbase.

RELNOTES[INC]: If the same artifact is generated by two distinct but identical actions, and a downstream action has both those actions' outputs in its inputs, the artifact will now appear twice in the downstream action's inputs. If this causes problems in Skylark actions, you can use the uniquify=True argument in Args.add_args.

PiperOrigin-RevId: 205863806

1 files changed, 22 insertions, 105 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
index 277b26933b..701aacaec4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
@@ -13,99 +13,52 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.Collections2;
 import com.google.common.collect.Interner;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
-import java.util.Collection;
 
 /**
- * A {@link SkyKey} coming from an {@link Artifact}. Source artifacts are checked for existence,
- * while output artifacts imply creation of the output file.
+ * A {@link SkyKey} coming from an {@link Artifact} that is not mandatory: absence of the Artifact
+ * does not imply any error.
  *
- * <p>There are effectively three kinds of output artifact values corresponding to these keys. The
- * first corresponds to an ordinary artifact {@link FileArtifactValue}. It stores the relevant data
- * for the artifact -- digest/mtime and size. The second corresponds to either an "aggregating"
- * artifact -- the output of an aggregating middleman action -- or a TreeArtifact. It stores the
- * relevant data of all its inputs, as well as a combined digest for itself.
- *
- * <p>Artifacts are compared using just their paths, but in Skyframe, the configured target that
- * owns an artifact must also be part of the comparison. For example, suppose we build //foo:foo in
- * configurationA, yielding artifact foo.out. If we change the configuration to configurationB in
- * such a way that the path to the artifact does not change, requesting foo.out from the graph will
- * result in the value entry for foo.out under configurationA being returned. This would prevent
- * caching the graph in different configurations, and also causes big problems with change pruning,
- * which assumes the invariant that a value's first dependency will always be the same. In this
- * case, the value entry's old dependency on //foo:foo in configurationA would cause it to request
- * (//foo:foo, configurationA) from the graph, causing an undesired re-analysis of (//foo:foo,
- * configurationA).
- *
- * <p>In order to prevent that, instead of just comparing Artifacts, we compare for equality using
- * both the Artifact, and the owner. The effect is functionally that of making Artifact.equals()
- * check the owner, but only within these keys, since outside of Skyframe it is quite crucial that
- * Artifacts with different owners be able to compare equal.
+ * <p>Since {@link Artifact} is already a {@link SkyKey}, this wrapper is only needed when the
+ * {@link Artifact} is not mandatory (discovered during include scanning).
  */
+// TODO(janakr): pull mandatory/non-mandatory handling up to consumers and get rid of this wrapper.
 @AutoCodec
 public class ArtifactSkyKey implements SkyKey {
   private static final Interner<ArtifactSkyKey> INTERNER = BlazeInterners.newWeakInterner();
-  private static final Function<Artifact, ArtifactSkyKey> TO_MANDATORY_KEY =
-      artifact -> key(artifact, true);
-  private static final Function<ArtifactSkyKey, Artifact> TO_ARTIFACT = ArtifactSkyKey::getArtifact;
 
-  private final Artifact artifact;
-  // Always true for derived artifacts.
-  private final boolean isMandatory;
-  // TODO(janakr): we may want to remove this field in the future. The expensive hash computation
-  // is already cached one level down (in the Artifact), so the CPU overhead here may not be
-  // worth the memory. However, when running with +CompressedOops, this field is free, so we leave
-  // it. When running with -CompressedOops, we might be able to save memory by using polymorphism
-  // for isMandatory and dropping this field.
-  private int hashCode = 0;
+  private final Artifact.SourceArtifact artifact;
 
-  private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) {
-    Preconditions.checkArgument(sourceArtifact.isSourceArtifact());
+  private ArtifactSkyKey(Artifact.SourceArtifact sourceArtifact) {
     this.artifact = Preconditions.checkNotNull(sourceArtifact);
-    this.isMandatory = mandatory;
-  }
-
-  /**
-   * Constructs an ArtifactSkyKey for a derived artifact. The mandatory attribute is not needed
-   * because a derived artifact must be a mandatory input for some action in order to ensure that it
-   * is built in the first place. If it fails to build, then that fact is cached in the node, so any
-   * action that has it as a non-mandatory input can retrieve that information from the node.
-   */
-  private ArtifactSkyKey(Artifact derivedArtifact) {
-    this.artifact = Preconditions.checkNotNull(derivedArtifact);
-    Preconditions.checkArgument(!derivedArtifact.isSourceArtifact(), derivedArtifact);
-    this.isMandatory = true; // Unused.
   }
 
   @ThreadSafety.ThreadSafe
-  @AutoCodec.Instantiator
-  public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) {
-    return INTERNER.intern(
-        artifact.isSourceArtifact()
-            ? new ArtifactSkyKey(artifact, isMandatory)
-            : new ArtifactSkyKey(artifact));
+  public static SkyKey key(Artifact artifact, boolean isMandatory) {
+    if (isMandatory || !artifact.isSourceArtifact()) {
+      return artifact;
+    }
+    return create(((Artifact.SourceArtifact) artifact));
   }
 
-  @ThreadSafety.ThreadSafe
-  public static Iterable<ArtifactSkyKey> mandatoryKeys(Iterable<Artifact> artifacts) {
-    return Iterables.transform(artifacts, TO_MANDATORY_KEY);
+  @AutoCodec.VisibleForSerialization
+  @AutoCodec.Instantiator
+  static ArtifactSkyKey create(Artifact.SourceArtifact artifact) {
+    return INTERNER.intern(new ArtifactSkyKey(artifact));
   }
 
-  public static Collection<Artifact> artifacts(Collection<? extends ArtifactSkyKey> keys) {
-    return Collections2.transform(keys, TO_ARTIFACT);
+  public static Artifact artifact(SkyKey key) {
+    return key instanceof Artifact ? (Artifact) key : ((ArtifactSkyKey) key).artifact;
   }
 
-  public static Artifact artifact(SkyKey key) {
-    return TO_ARTIFACT.apply((ArtifactSkyKey) key.argument());
+  public static boolean isMandatory(SkyKey key) {
+    return key instanceof Artifact;
   }
 
   @Override
@@ -115,32 +68,7 @@ public class ArtifactSkyKey implements SkyKey {
 
   @Override
   public int hashCode() {
-    // We use the hash code caching strategy employed by java.lang.String. There are three subtle
-    // things going on here:
-    //
-    // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
-    // Yes, this means that if the hash code is really 0 then we will "recompute" it each time.
-    // But this isn't a problem in practice since a hash code of 0 should be rare.
-    //
-    // (2) Since we have no synchronization, multiple threads can race here thinking there are the
-    // first one to compute and cache the hash code.
-    //
-    // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
-    // thread may not be visible by another. Note that we probably don't need to worry about
-    // multiple inefficient reads of 'hashCode' on the same thread since it's non-volatile.
-    //
-    // All three of these issues are benign from a correctness perspective; in the end we have no
-    // overhead from synchronization, at the cost of potentially computing the hash code more than
-    // once.
-    if (hashCode == 0) {
-      hashCode = computeHashCode();
-    }
-    return hashCode;
-  }
-
-  private int computeHashCode() {
-    int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
-    return isMandatory ? initialHash : 47 * initialHash + 1;
+    return artifact.hashCode();
   }
 
   @Override
@@ -152,24 +80,13 @@ public class ArtifactSkyKey implements SkyKey {
       return false;
     }
     ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that);
-    return Artifact.equalWithOwner(artifact, thatArtifactSkyKey.artifact)
-        && isMandatory == thatArtifactSkyKey.isMandatory;
+    return artifact.equals(thatArtifactSkyKey.artifact);
   }
 
   public Artifact getArtifact() {
     return artifact;
   }
 
-  /**
-   * Returns whether the artifact is a mandatory input of its requesting action. May only be called
-   * for source artifacts, since a derived artifact must be a mandatory input of some action in
-   * order to have been built in the first place.
-   */
-  public boolean isMandatory() {
-    Preconditions.checkState(artifact.isSourceArtifact(), artifact);
-    return isMandatory;
-  }
-
   @Override
   public String toString() {
     return artifact.prettyPrint() + " " + artifact.getArtifactOwner();