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, 56 insertions, 60 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index f21d579aed..49dad3ff38 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -22,14 +22,14 @@ import com.google.common.base.Functions;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
@@ -47,6 +47,7 @@ import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
 import java.io.IOException;
@@ -55,7 +56,6 @@ import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Objects;
-import java.util.concurrent.ExecutionException;
 import javax.annotation.Nullable;
 
 /**
@@ -110,7 +110,8 @@ public class Artifact
         ActionInput,
         FileApi,
         Comparable<Object>,
-        CommandLineItem {
+        CommandLineItem,
+        SkyKey {
 
   /** Compares artifact according to their exec paths. Sorts null values first. */
   @SuppressWarnings("ReferenceEquality")  // "a == b" is an optimization
@@ -167,8 +168,7 @@ public class Artifact
   /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */
   public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact();
 
-  private static final Cache<InternedArtifact, Artifact> ARTIFACT_INTERNER =
-      CacheBuilder.newBuilder().weakValues().build();
+  private static final Interner<Artifact> ARTIFACT_INTERNER = BlazeInterners.newWeakInterner();
 
   private final int hashCode;
   private final ArtifactRoot root;
@@ -203,15 +203,7 @@ public class Artifact
     if (artifact.isSourceArtifact()) {
       return artifact;
     } else {
-      return intern(artifact);
-    }
-  }
-
-  private static Artifact intern(Artifact artifact) {
-    try {
-      return ARTIFACT_INTERNER.get(new InternedArtifact(artifact), () -> artifact);
-    } catch (ExecutionException e) {
-      throw new IllegalStateException(e);
+      return ARTIFACT_INTERNER.intern(artifact);
     }
   }
 
@@ -257,6 +249,9 @@ public class Artifact
       throw new IllegalArgumentException(
           "it is illegal to create an artifact with an empty execPath");
     }
+    // The ArtifactOwner is not part of this computation because it is very rare that two Artifacts
+    // have the same execPath and different owners, so a collision is fine there. If this is
+    // changed, OwnerlessArtifactWrapper must also be changed.
     this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
     this.root = root;
     this.execPath = execPath;
@@ -484,6 +479,15 @@ public class Artifact
     public SourceArtifact asSourceArtifact() {
       return this;
     }
+
+    /**
+     * SourceArtifacts are compared without their owners, since owners do not affect behavior,
+     * unlike with derived artifacts, whose owners determine their generating actions.
+     */
+    @Override
+    public boolean equals(Object other) {
+      return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other);
+    }
   }
 
   /**
@@ -614,34 +618,6 @@ public class Artifact
   }
 
   /**
-   * Wraps an artifact for interning because we need to check the artifact owner when doing equals.
-   */
-  private static final class InternedArtifact {
-    private final Artifact wrappedArtifact;
-
-    InternedArtifact(Artifact artifact) {
-      this.wrappedArtifact = artifact;
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      if (!(other instanceof InternedArtifact)) {
-        return false;
-      }
-      if (!getClass().equals(other.getClass())) {
-        return false;
-      }
-      InternedArtifact that = (InternedArtifact) other;
-      return Artifact.equalWithOwner(wrappedArtifact, that.wrappedArtifact);
-    }
-
-    @Override
-    public final int hashCode() {
-      return wrappedArtifact.hashCode();
-    }
-  }
-
-  /**
    * Returns the relative path to this artifact relative to its root.  (Useful
    * when deriving output filenames from input files, etc.)
    */
@@ -693,37 +669,28 @@ public class Artifact
     return rootRelativePath.toString();
   }
 
+  @SuppressWarnings("EqualsGetClass") // Distinct classes of Artifact are never equal.
   @Override
-  public final boolean equals(Object other) {
+  public boolean equals(Object other) {
     if (!(other instanceof Artifact)) {
       return false;
     }
     if (!getClass().equals(other.getClass())) {
       return false;
     }
-    // We don't bother to check root in the equivalence relation, because we
-    // assume that no root is an ancestor of another one.
     Artifact that = (Artifact) other;
-    return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root);
+    return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner());
   }
 
-  /**
-   * Compare equality including Artifact owner equality, a notable difference compared to the
-   * {@link #equals(Object)} method of {@link Artifact}.
-   */
-  public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) {
-    if (first != null) {
-      return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
-    } else {
-      return second == null;
-    }
+  public boolean equalsWithoutOwner(Artifact other) {
+    return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root);
   }
 
   @Override
   public final int hashCode() {
-    // This is just path.hashCode().  We cache a copy in the Artifact object to reduce LLC misses
-    // during operations which build a HashSet out of many Artifacts.  This is a slight loss for
-    // memory but saves ~1% overall CPU in some real builds.
+    // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact
+    // object to reduce LLC misses during operations which build a HashSet out of many Artifacts.
+    // This is a slight loss for memory but saves ~1% overall CPU in some real builds.
     return hashCode;
   }
 
@@ -1031,4 +998,33 @@ public class Artifact
       printer.append("<generated file " + rootRelativePath + ">");
     }
   }
+
+  /**
+   * A utility class that compares {@link Artifact}s without taking their owners into account.
+   * Should only be used for detecting action conflicts and merging shared action data.
+   */
+  public static class OwnerlessArtifactWrapper {
+    private final Artifact artifact;
+
+    public OwnerlessArtifactWrapper(Artifact artifact) {
+      this.artifact = artifact;
+    }
+
+    @Override
+    public int hashCode() {
+      // Depends on the fact that Artifact#hashCode does not use ArtifactOwner.
+      return artifact.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      return obj instanceof OwnerlessArtifactWrapper
+          && this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact);
+    }
+  }
+
+  @Override
+  public SkyFunctionName functionName() {
+    return ARTIFACT;
+  }
 }