Refactor ArtifactSkyKey to get rid of an unnecessary wrapper class: actually essentially promote OwnedArtifact to ArtifactSkyKey and rename it to ArtifactSkyKey. The king is dead...

Also add some other execution-phase codecs.

PiperOrigin-RevId: 184552706

4 files changed, 138 insertions, 155 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
index 299f9f8c21..b67636c084 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
@@ -16,15 +16,17 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.Objects;
 import java.util.Set;
 
-/**
- * Contains options which control the set of artifacts to build for top-level targets.
- */
+/** Contains options which control the set of artifacts to build for top-level targets. */
 @Immutable
+@AutoCodec
 public final class TopLevelArtifactContext {
+  public static final ObjectCodec<TopLevelArtifactContext> CODEC =
+      new TopLevelArtifactContext_AutoCodec();
 
   private final boolean runTestsExclusively;
   private final ImmutableSortedSet<String> outputGroups;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
index 8c49441f25..ae4ced6fb3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
@@ -20,7 +20,6 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
-import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
 import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey;
 import com.google.devtools.build.lib.skyframe.TestCompletionValue.TestCompletionKey;
 import com.google.devtools.build.skyframe.CycleInfo;
@@ -48,8 +47,8 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
   }
 
   private String prettyPrint(SkyFunctionName skyFunctionName, Object arg) {
-    if (arg instanceof OwnedArtifact) {
-      return "file: " + ((OwnedArtifact) arg).getArtifact().getRootRelativePathString();
+    if (arg instanceof ArtifactSkyKey) {
+      return "file: " + ((ArtifactSkyKey) arg).getArtifact().getRootRelativePathString();
     } else if (arg instanceof ActionLookupData) {
       return "action from: " + arg;
     } else if (arg instanceof TargetCompletionKey
@@ -60,14 +59,14 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
       return "test target: " + ((TestCompletionKey) arg).configuredTargetKey().getLabel();
     }
     throw new IllegalStateException(
-        "Argument is not Action, TargetCompletion, TestCompletion or OwnedArtifact: " + arg);
+        "Argument is not Action, TargetCompletion, TestCompletion or ArtifactSkyKey: " + arg);
   }
 
   @Override
   protected Label getLabel(SkyKey key) {
     Object arg = key.argument();
-    if (arg instanceof OwnedArtifact) {
-      return ((OwnedArtifact) arg).getArtifact().getOwner();
+    if (arg instanceof ArtifactSkyKey) {
+      return ((ArtifactSkyKey) arg).getArtifact().getOwner();
     } else if (arg instanceof ActionLookupData) {
       return ((ActionLookupData) arg).getLabelForErrors();
     } else if (arg instanceof TargetCompletionKey
@@ -78,7 +77,7 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
       return ((TestCompletionKey) arg).configuredTargetKey().getLabel();
     }
     throw new IllegalStateException(
-        "Argument is not Action, TargetCompletion, TestCompletion or OwnedArtifact: " + arg);
+        "Argument is not Action, TargetCompletion, TestCompletion or ArtifactSkyKey: " + arg);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 2fe2b69ef2..5727adf638 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.analysis.actions.ActionTemplate;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -54,11 +53,11 @@ class ArtifactFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env)
       throws ArtifactFunctionException, InterruptedException {
-    OwnedArtifact ownedArtifact = (OwnedArtifact) skyKey.argument();
-    Artifact artifact = ownedArtifact.getArtifact();
+    ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
+    Artifact artifact = artifactSkyKey.getArtifact();
     if (artifact.isSourceArtifact()) {
       try {
-        return createSourceValue(artifact, ownedArtifact.isMandatory(), env);
+        return createSourceValue(artifact, artifactSkyKey.isMandatory(), env);
       } catch (MissingInputFileException e) {
         // The error is not necessarily truly transient, but we mark it as such because we have
         // the above side effect of posting an event to the EventBus. Importantly, that event
@@ -306,7 +305,7 @@ class ArtifactFunction implements SkyFunction {
 
   @Override
   public String extractTag(SkyKey skyKey) {
-    return Label.print(((OwnedArtifact) skyKey.argument()).getArtifact().getOwner());
+    return Label.print(((ArtifactSkyKey) skyKey.argument()).getArtifact().getOwner());
   }
 
   @VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
index df7314bf34..4370089799 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
@@ -20,184 +20,167 @@ import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.concurrent.ThreadSafety;
+import com.google.devtools.build.lib.skyframe.serialization.InjectingObjectCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.vfs.FileSystemProvider;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import java.util.Collection;
 
 /**
- * A utility class for {@link SkyKey}s coming from {@link Artifact}s. Source artifacts are checked
- * for existence, while output artifacts imply creation of the output file.
+ * A {@link SkyKey} coming from an {@link Artifact}. Source artifacts are checked for existence,
+ * while output artifacts imply creation of the output file.
  *
  * <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.
  */
-@Immutable
-@ThreadSafe
-public final class ArtifactSkyKey {
-  private static final Interner<OwnedArtifact> INTERNER = BlazeInterners.newWeakInterner();
+@AutoCodec(dependency = FileSystemProvider.class)
+public class ArtifactSkyKey implements SkyKey {
+  private static final Interner<ArtifactSkyKey> INTERNER = BlazeInterners.newWeakInterner();
+  public static final InjectingObjectCodec<ArtifactSkyKey, FileSystemProvider> CODEC =
+      new ArtifactSkyKey_AutoCodec();
+  private static final Function<Artifact, SkyKey> 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 ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) {
+    Preconditions.checkArgument(sourceArtifact.isSourceArtifact());
+    this.artifact = Preconditions.checkNotNull(sourceArtifact);
+    this.isMandatory = mandatory;
+  }
 
-  private ArtifactSkyKey() {}
+  /**
+   * 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.
+  }
 
-  @ThreadSafe
-  public static SkyKey key(Artifact artifact, boolean isMandatory) {
+  @ThreadSafety.ThreadSafe
+  @AutoCodec.Instantiator
+  public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) {
     return INTERNER.intern(
         artifact.isSourceArtifact()
-            ? new OwnedArtifact(artifact, isMandatory)
-            : new OwnedArtifact(artifact));
+            ? new ArtifactSkyKey(artifact, isMandatory)
+            : new ArtifactSkyKey(artifact));
   }
 
-  private static final Function<Artifact, SkyKey> TO_MANDATORY_KEY =
-      new Function<Artifact, SkyKey>() {
-        @Override
-        public SkyKey apply(Artifact artifact) {
-          return key(artifact, true);
-        }
-      };
-
-  @ThreadSafe
-  public static Iterable<SkyKey> mandatoryKeys(Iterable<Artifact> artifacts) {
+  @ThreadSafety.ThreadSafe
+  static Iterable<SkyKey> mandatoryKeys(Iterable<Artifact> artifacts) {
     return Iterables.transform(artifacts, TO_MANDATORY_KEY);
   }
 
-  private static final Function<OwnedArtifact, Artifact> TO_ARTIFACT =
-      new Function<OwnedArtifact, Artifact>() {
-        @Override
-        public Artifact apply(OwnedArtifact key) {
-          return key.getArtifact();
-        }
-      };
-
-  public static Collection<Artifact> artifacts(Collection<? extends OwnedArtifact> keys) {
+  public static Collection<Artifact> artifacts(Collection<? extends ArtifactSkyKey> keys) {
     return Collections2.transform(keys, TO_ARTIFACT);
   }
 
   public static Artifact artifact(SkyKey key) {
-    return TO_ARTIFACT.apply((OwnedArtifact) key.argument());
+    return TO_ARTIFACT.apply((ArtifactSkyKey) key.argument());
   }
 
   public static boolean equalWithOwner(Artifact first, Artifact second) {
     return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
   }
 
-  /**
-   * 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 using Artifacts as keys in the graph, we use
-   * OwnedArtifacts, which 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 Skyframe,
-   * since outside of Skyframe it is quite crucial that Artifacts with different owners be able to
-   * compare equal.
-   */
-  static class OwnedArtifact implements SkyKey {
-    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;
-
-    /** Constructs an OwnedArtifact wrapper for a source artifact. */
-    private OwnedArtifact(Artifact sourceArtifact, boolean mandatory) {
-      Preconditions.checkArgument(sourceArtifact.isSourceArtifact());
-      this.artifact = Preconditions.checkNotNull(sourceArtifact);
-      this.isMandatory = mandatory;
-    }
-
-    /**
-     * Constructs an OwnedArtifact wrapper 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 OwnedArtifact(Artifact derivedArtifact) {
-      this.artifact = Preconditions.checkNotNull(derivedArtifact);
-      Preconditions.checkArgument(!derivedArtifact.isSourceArtifact(), derivedArtifact);
-      this.isMandatory = true; // Unused.
-    }
+  @Override
+  public SkyFunctionName functionName() {
+    return SkyFunctions.ARTIFACT;
+  }
 
-    @Override
-    public SkyFunctionName functionName() {
-      return SkyFunctions.ARTIFACT;
+  @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;
+  }
 
-    @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;
+  }
 
-    private int computeHashCode() {
-      int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
-      return isMandatory ? initialHash : 47 * initialHash + 1;
+  @Override
+  public boolean equals(Object that) {
+    if (this == that) {
+      return true;
     }
-
-    @Override
-    public boolean equals(Object that) {
-      if (this == that) {
-        return true;
-      }
-      if (!(that instanceof OwnedArtifact)) {
-        return false;
-      }
-      OwnedArtifact thatOwnedArtifact = ((OwnedArtifact) that);
-      Artifact thatArtifact = thatOwnedArtifact.artifact;
-      return equalWithOwner(artifact, thatArtifact) && isMandatory == thatOwnedArtifact.isMandatory;
+    if (!(that instanceof ArtifactSkyKey)) {
+      return false;
     }
+    ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that);
+    return equalWithOwner(artifact, thatArtifactSkyKey.artifact)
+        && isMandatory == thatArtifactSkyKey.isMandatory;
+  }
 
-    Artifact getArtifact() {
-      return artifact;
-    }
+  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;
-    }
+  /**
+   * 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();
-    }
+  @Override
+  public String toString() {
+    return artifact.prettyPrint() + " " + artifact.getArtifactOwner();
   }
 }