Automated rollback of commit 03f6af6.

*** Reason for rollback ***

Potentially causes a performance regression: []

*** Original change description ***

Define a natural ordering for `ActionInput`.

`Artifact` defines natural ordering by comparing exec paths, which are defined at the level of `ActionInput`. Move that to `ActionInput` interface. Move `Artifact.EXEC_PATH_COMPARATOR` to `ActionInput` to allow convenient comparisons with null.

PiperOrigin-RevId: 380019680

src/main/java/com/google/devtools/build/lib/actions/ActionInput.java

@@ -15,47 +15,30 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Comparator;
 
 /**
  * Represents an input file to a build action, with an appropriate relative path.
  *
- * <p>Artifact is the only notable implementer of the interface, but the interface remains because
- * 1) some Google specific rules ship files that could be Artifacts to remote execution by
- * instantiating ad-hoc derived classes of ActionInput. 2) historically, Google C++ rules allow
- * underspecified C++ builds. For that case, we have extra logic to guess the undeclared header
- * inclusions (eg. computed inclusions). The extra logic lives in a file that is not needed for
- * remote execution, but is a dependency, and it is inserted as a non-Artifact ActionInput.
+ * <p>Artifact is the only notable implementer of the interface, but the interface remains
+ * because 1) some Google specific rules ship files that could be Artifacts to remote execution
+ * by instantiating ad-hoc derived classes of ActionInput.  2) historically, Google C++ rules
+ * allow underspecified C++ builds. For that case, we have extra logic to guess the undeclared
+ * header inclusions (eg. computed inclusions). The extra logic lives in a file that is not
+ * needed for remote execution, but is a dependency, and it is inserted as a non-Artifact
+ * ActionInput.
  *
- * <p>ActionInput is used as a cache "key" for ActionInputFileCache: for Artifacts, the digest/size
- * is already stored in Artifact, but for non-artifacts, we use getExecPathString to find this data
- * in a filesystem related cache.
- *
- * <p>Note: this interface defines a natural ordering but no equals, therefore there is no guarantee
- * about consistency between the two.
+ * <p>ActionInput is used as a cache "key" for ActionInputFileCache: for Artifacts, the
+ * digest/size is already stored in Artifact, but for non-artifacts, we use getExecPathString
+ * to find this data in a filesystem related cache.
  */
-public interface ActionInput extends Comparable<ActionInput> {
-
-  /** Compares action input according to their exec paths. Sorts null values first. */
-  @SuppressWarnings("ReferenceEquality") // "this == other" is an optimization
-  Comparator<ActionInput> EXEC_PATH_COMPARATOR =
-      (a, b) -> {
-        if (a == b) {
-          return 0;
-        }
-        if (a == null) {
-          return -1;
-        }
-        if (b == null) {
-          return 1;
-        }
-        return a.getExecPath().compareTo(b.getExecPath());
-      };
+public interface ActionInput {
 
-  /** Returns the relative path to the input file. */
+  /** @return the relative path to the input file. */
   String getExecPathString();
 
-  /** Returns the relative path to the input file. */
+  /**
+   * @return the relative path to the input file.
+   */
   PathFragment getExecPath();
 
   /** The input is a symlink that is supposed to stay un-dereferenced. */
@@ -68,10 +51,4 @@ public interface ActionInput extends Comparable<ActionInput> {
   default boolean contentBasedPath() {
     return false;
   }
-
-  /** Orders action inputs by {@linkplain #getExecPath() exec paths}, nulls first. */
-  @Override
-  default int compareTo(ActionInput other) {
-    return EXEC_PATH_COMPARATOR.compare(this, other);
-  }
 }

src/main/java/com/google/devtools/build/lib/actions/Artifact.java

@@ -123,11 +123,27 @@ public abstract class Artifact
     implements FileType.HasFileType,
         ActionInput,
         FileApi,
+        Comparable<Artifact>,
         CommandLineItem,
         SkyKey {
 
   public static final Depset.ElementType TYPE = Depset.ElementType.of(Artifact.class);
 
+  /** Compares artifact according to their exec paths. Sorts null values first. */
+  @SuppressWarnings("ReferenceEquality") // "a == b" is an optimization
+  public static final Comparator<Artifact> EXEC_PATH_COMPARATOR =
+      (a, b) -> {
+        if (a == b) {
+          return 0;
+        } else if (a == null) {
+          return -1;
+        } else if (b == null) {
+          return 1;
+        } else {
+          return a.execPath.compareTo(b.execPath);
+        }
+      };
+
   /** Compares artifact according to their root relative paths. Sorts null values first. */
   @SuppressWarnings("ReferenceEquality") // "a == b" is an optimization
   public static final Comparator<Artifact> ROOT_RELATIVE_PATH_COMPARATOR =
@@ -196,10 +212,9 @@ public static List<SkyKey> keys(List<Artifact> artifacts) {
     return Lists.transform(artifacts, Artifact::key);
   }
 
-  /** Pass-through override to make the method final. */
   @Override
-  public final int compareTo(ActionInput o) {
-    return ActionInput.super.compareTo(o);
+  public int compareTo(Artifact o) {
+    return EXEC_PATH_COMPARATOR.compare(this, o);
   }
 
   /** An object that can expand middleman and tree artifacts. */

src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java

@@ -201,7 +201,7 @@ private Map<Artifact, String> getOrphanArtifactMap() {
     }
     // The order of the artifacts.entrySet iteration is unspecified - we use a TreeMap here to
     // guarantee that the return value of this method is deterministic.
-    Map<Artifact, String> orphanArtifacts = new TreeMap<>();
+    Map<Artifact, String> orphanArtifacts = new TreeMap<>(Artifact.EXEC_PATH_COMPARATOR);
     for (Map.Entry<Artifact, Object> entry : artifacts.entrySet()) {
       Artifact a = entry.getKey();
       if (!a.isSourceArtifact() && !artifactsWithActions.contains(a)) {

src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java

@@ -298,7 +298,7 @@ ImmutableSet<Artifact> getPathLevelHintedInclusions(
           continue;
         }
         if (hints == null) {
-          hints = ImmutableSortedSet.naturalOrder();
+          hints = ImmutableSortedSet.orderedBy(Artifact.EXEC_PATH_COMPARATOR);
         }
         PathFragment relativePath = PathFragment.create(m.replaceFirst(rule.findRoot));
         logger.atFine().log(

src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java

@@ -290,7 +290,7 @@ private static Map<Artifact, Label> getMergeeManifests(
     }
     switch (manifestMergerOrder) {
       case ALPHABETICAL:
-        return ImmutableSortedMap.copyOf(builder.build());
+        return ImmutableSortedMap.copyOf(builder.build(), Artifact.EXEC_PATH_COMPARATOR);
       case ALPHABETICAL_BY_CONFIGURATION:
         return ImmutableSortedMap.copyOf(builder.build(), Artifact.ROOT_RELATIVE_PATH_COMPARATOR);
       case DEPENDENCY:

src/main/java/com/google/devtools/build/lib/rules/android/ProguardHelper.java

@@ -214,7 +214,7 @@ public static ImmutableList<Artifact> collectTransitiveProguardSpecs(
     }
 
     ImmutableSortedSet.Builder<Artifact> builder =
-        ImmutableSortedSet.<Artifact>naturalOrder()
+        ImmutableSortedSet.orderedBy(Artifact.EXEC_PATH_COMPARATOR)
             .addAll(localProguardSpecs)
             .addAll(specsToInclude);
     for (ProguardSpecProvider dep : proguardDeps) {

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
-import static java.util.Comparator.naturalOrder;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Ascii;
@@ -103,6 +102,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
@@ -416,7 +416,7 @@ private NestedSet<Artifact> findUsedHeaders(
           return null;
         }
 
-        includes.sort(naturalOrder());
+        Collections.sort(includes, Artifact.EXEC_PATH_COMPARATOR);
         return NestedSetBuilder.wrap(Order.STABLE_ORDER, includes);
       } catch (IORuntimeException e) {
         throw new EnvironmentalExecException(

src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java

@@ -61,7 +61,7 @@ public CreateIncSymlinkAction(
         owner,
         NestedSetBuilder.wrap(Order.STABLE_ORDER, symlinks.values()),
         ImmutableSet.copyOf(symlinks.keySet()));
-    this.symlinks = ImmutableSortedMap.copyOf(symlinks);
+    this.symlinks = ImmutableSortedMap.copyOf(symlinks, Artifact.EXEC_PATH_COMPARATOR);
     this.includePath = includePath;
   }
 

src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java

@@ -118,17 +118,16 @@ public void testEquivalenceRelation() throws Exception {
         .isFalse();
   }
 
-  @SuppressWarnings("SelfComparison")
   @Test
   public void testComparison() {
     PathFragment aPath = PathFragment.create("src/a");
     PathFragment bPath = PathFragment.create("src/b");
     Artifact aArtifact = ActionsTestUtil.createArtifactWithRootRelativePath(rootDir, aPath);
     Artifact bArtifact = ActionsTestUtil.createArtifactWithRootRelativePath(rootDir, bPath);
-    assertThat(aArtifact.compareTo(bArtifact)).isEqualTo(-1);
-    assertThat(aArtifact.compareTo(aArtifact)).isEqualTo(0);
-    assertThat(bArtifact.compareTo(bArtifact)).isEqualTo(0);
-    assertThat(bArtifact.compareTo(aArtifact)).isEqualTo(1);
+    assertThat(Artifact.EXEC_PATH_COMPARATOR.compare(aArtifact, bArtifact)).isEqualTo(-1);
+    assertThat(Artifact.EXEC_PATH_COMPARATOR.compare(aArtifact, aArtifact)).isEqualTo(0);
+    assertThat(Artifact.EXEC_PATH_COMPARATOR.compare(bArtifact, bArtifact)).isEqualTo(0);
+    assertThat(Artifact.EXEC_PATH_COMPARATOR.compare(bArtifact, aArtifact)).isEqualTo(1);
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/actions/BUILD

@@ -77,7 +77,6 @@ java_library(
         "//third_party:mockito",
         "//third_party:truth",
         "//third_party/protobuf:protobuf_java",
-        "@com_google_testparameterinjector//:testparameterinjector",
     ],
 )