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
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 299f9f8..b67636c 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 @@ 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 8c49441f..ae4ced6 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.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 @@ } 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 @@ 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 @@ 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 2fe2b69..5727adf 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.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 @@ @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 @@ @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 df7314b..4370089 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.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 ArtifactSkyKey() {} + 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; - @ThreadSafe - public static SkyKey key(Artifact artifact, boolean isMandatory) { - return INTERNER.intern( - artifact.isSourceArtifact() - ? new OwnedArtifact(artifact, isMandatory) - : new OwnedArtifact(artifact)); + private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) { + Preconditions.checkArgument(sourceArtifact.isSourceArtifact()); + this.artifact = Preconditions.checkNotNull(sourceArtifact); + this.isMandatory = mandatory; } - private static final Function<Artifact, SkyKey> TO_MANDATORY_KEY = - new Function<Artifact, SkyKey>() { - @Override - public SkyKey apply(Artifact artifact) { - return key(artifact, true); - } - }; + /** + * 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 Iterable<SkyKey> mandatoryKeys(Iterable<Artifact> artifacts) { + @ThreadSafety.ThreadSafe + @AutoCodec.Instantiator + public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) { + return INTERNER.intern( + artifact.isSourceArtifact() + ? new ArtifactSkyKey(artifact, isMandatory) + : new ArtifactSkyKey(artifact)); + } + + @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()); } + @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; + } + + 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; + } + if (!(that instanceof ArtifactSkyKey)) { + return false; + } + ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that); + return equalWithOwner(artifact, thatArtifactSkyKey.artifact) + && isMandatory == thatArtifactSkyKey.isMandatory; + } + + Artifact getArtifact() { + return artifact; + } + /** - * 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. + * 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. */ - 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; + public boolean isMandatory() { + Preconditions.checkState(artifact.isSourceArtifact(), artifact); + return isMandatory; + } - /** 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 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; - } - - @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; - } - - 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(); - } + @Override + public String toString() { + return artifact.prettyPrint() + " " + artifact.getArtifactOwner(); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index 147b8b2..7c86478 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate.OutputPathMapper; import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; -import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact; import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -243,8 +242,8 @@ } @Override public SkyValue compute(SkyKey skyKey, Environment env) { - OwnedArtifact ownedArtifact = (OwnedArtifact) skyKey.argument(); - Artifact artifact = ownedArtifact.getArtifact(); + ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument(); + Artifact artifact = artifactSkyKey.getArtifact(); return Preconditions.checkNotNull(artifactValueMap.get(artifact)); }