diff options
16 files changed, 169 insertions, 249 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index a3a6ba6b58..5364316f0c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.actions; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.escape.Escaper; import com.google.common.escape.Escapers; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; @@ -24,7 +25,6 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.Collection; import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; @@ -74,39 +74,15 @@ public final class Actions { } // Don't bother to check input and output counts first; the expected result for these tests is // to always be true (i.e., that this method returns true). - if (!artifactsEqualWithoutOwner(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) { + if (!Iterables.elementsEqual(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) { return false; } - if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) { + if (!Iterables.elementsEqual(actionA.getOutputs(), actionB.getOutputs())) { return false; } return true; } - private static boolean artifactsEqualWithoutOwner( - Iterable<Artifact> iterable1, Iterable<Artifact> iterable2) { - if (iterable1 instanceof Collection && iterable2 instanceof Collection) { - Collection<?> collection1 = (Collection<?>) iterable1; - Collection<?> collection2 = (Collection<?>) iterable2; - if (collection1.size() != collection2.size()) { - return false; - } - } - Iterator<Artifact> iterator1 = iterable1.iterator(); - Iterator<Artifact> iterator2 = iterable2.iterator(); - while (iterator1.hasNext()) { - if (!iterator2.hasNext()) { - return false; - } - Artifact artifact1 = iterator1.next(); - Artifact artifact2 = iterator2.next(); - if (!artifact1.equalsWithoutOwner(artifact2)) { - return false; - } - } - return !iterator2.hasNext(); - } - /** * Finds action conflicts. An action conflict happens if two actions generate the same output * artifact. Shared actions are tolerated. See {@link #canBeShared} for details. 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 f91c5cde5a..a974646d55 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 @@ -45,7 +45,6 @@ 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; @@ -108,8 +107,7 @@ public class Artifact ActionInput, FileApi, Comparable<Object>, - CommandLineItem, - SkyKey { + CommandLineItem { /** Compares artifact according to their exec paths. Sorts null values first. */ @SuppressWarnings("ReferenceEquality") // "a == b" is an optimization @@ -228,9 +226,6 @@ 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; @@ -458,15 +453,6 @@ 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); - } } /** @@ -649,26 +635,36 @@ public class Artifact } @Override - public boolean equals(Object other) { + public final 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 equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner()); + return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root); } - public boolean equalsWithoutOwner(Artifact other) { - return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root); + /** + * 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; + } } @Override public final int hashCode() { - // 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. + // 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. return hashCode; } @@ -976,33 +972,4 @@ 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; - } } 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 701aacaec4..277b26933b 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,52 +13,99 @@ // 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} that is not mandatory: absence of the Artifact - * does not imply any error. + * A {@link SkyKey} coming from an {@link Artifact}. Source artifacts are checked for existence, + * while output artifacts imply creation of the output file. * - * <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). + * <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. */ -// 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.SourceArtifact artifact; + 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 sourceArtifact) { + private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) { + Preconditions.checkArgument(sourceArtifact.isSourceArtifact()); this.artifact = Preconditions.checkNotNull(sourceArtifact); + this.isMandatory = mandatory; } - @ThreadSafety.ThreadSafe - public static SkyKey key(Artifact artifact, boolean isMandatory) { - if (isMandatory || !artifact.isSourceArtifact()) { - return artifact; - } - return create(((Artifact.SourceArtifact) artifact)); + /** + * 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. } - @AutoCodec.VisibleForSerialization + @ThreadSafety.ThreadSafe @AutoCodec.Instantiator - static ArtifactSkyKey create(Artifact.SourceArtifact artifact) { - return INTERNER.intern(new ArtifactSkyKey(artifact)); + public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) { + return INTERNER.intern( + artifact.isSourceArtifact() + ? new ArtifactSkyKey(artifact, isMandatory) + : new ArtifactSkyKey(artifact)); } - public static Artifact artifact(SkyKey key) { - return key instanceof Artifact ? (Artifact) key : ((ArtifactSkyKey) key).artifact; + @ThreadSafety.ThreadSafe + public static Iterable<ArtifactSkyKey> mandatoryKeys(Iterable<Artifact> artifacts) { + return Iterables.transform(artifacts, TO_MANDATORY_KEY); } - public static boolean isMandatory(SkyKey key) { - return key instanceof Artifact; + 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((ArtifactSkyKey) key.argument()); } @Override @@ -68,7 +115,32 @@ public class ArtifactSkyKey implements SkyKey { @Override public int hashCode() { - return artifact.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 @@ -80,13 +152,24 @@ public class ArtifactSkyKey implements SkyKey { return false; } ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that); - return artifact.equals(thatArtifactSkyKey.artifact); + return Artifact.equalWithOwner(artifact, thatArtifactSkyKey.artifact) + && isMandatory == thatArtifactSkyKey.isMandatory; } 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(); diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java index fca7d2218c..703611fd27 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; -import java.util.Objects; import javax.annotation.Nullable; /** @@ -112,7 +111,9 @@ public interface FilesetTraversalParams { if (o instanceof FilesetTraversalParams.DirectTraversalRoot) { FilesetTraversalParams.DirectTraversalRoot that = (FilesetTraversalParams.DirectTraversalRoot) o; - return Objects.equals(this.getOutputArtifact(), that.getOutputArtifact()) + // Careful! We must compare the artifact owners, which the default {@link Artifact#equals()} + // method does not do. See the comments on {@link ArtifactSkyKey} and http://b/73738481. + return Artifact.equalWithOwner(this.getOutputArtifact(), that.getOutputArtifact()) && (this.getRootPart().equals(that.getRootPart())) && (this.getRelativePart().equals(that.getRelativePart())); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java index dadad155e6..f35f963bf7 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java +++ b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.actions; import com.google.common.base.Preconditions; -import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; @@ -25,7 +24,7 @@ import javax.annotation.concurrent.ThreadSafe; @ThreadSafe public final class MapBasedActionGraph implements MutableActionGraph { private final ActionKeyContext actionKeyContext; - private final ConcurrentMultimapWithHeadElement<OwnerlessArtifactWrapper, ActionAnalysisMetadata> + private final ConcurrentMultimapWithHeadElement<Artifact, ActionAnalysisMetadata> generatingActionMap = new ConcurrentMultimapWithHeadElement<>(); public MapBasedActionGraph(ActionKeyContext actionKeyContext) { @@ -35,18 +34,17 @@ public final class MapBasedActionGraph implements MutableActionGraph { @Override @Nullable public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { - return generatingActionMap.get(new OwnerlessArtifactWrapper(artifact)); + return generatingActionMap.get(artifact); } @Override public void registerAction(ActionAnalysisMetadata action) throws ActionConflictException { for (Artifact artifact : action.getOutputs()) { - OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact); - ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(wrapper, action); + ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(artifact, action); if (previousAction != null && previousAction != action && !Actions.canBeShared(actionKeyContext, action, previousAction)) { - generatingActionMap.remove(wrapper, action); + generatingActionMap.remove(artifact, action); throw new ActionConflictException(actionKeyContext, artifact, previousAction, action); } } @@ -55,9 +53,8 @@ public final class MapBasedActionGraph implements MutableActionGraph { @Override public void unregisterAction(ActionAnalysisMetadata action) { for (Artifact artifact : action.getOutputs()) { - OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact); - generatingActionMap.remove(wrapper, action); - ActionAnalysisMetadata otherAction = generatingActionMap.get(wrapper); + generatingActionMap.remove(artifact, action); + ActionAnalysisMetadata otherAction = generatingActionMap.get(artifact); Preconditions.checkState( otherAction == null || (otherAction != action diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java index 1496bc288e..fa835618ec 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java @@ -126,7 +126,8 @@ public class NestedSetCodecWithStore implements ObjectCodec<NestedSet<?>> { * NestedSet#getChildren()}. When that field is an Object[], this is just identity hash code and * reference equality, but when it is something else (like an Artifact), we will do an actual * equality comparison. This may make some singleton NestedSets reference-equal where they were - * not before. This should be ok as long as the contained object properly implements equality. + * not before. This should be ok, but isn't because Artifact does not properly implement equality + * (it ignores the ArtifactOwner). */ @SuppressWarnings("unchecked") private NestedSet<?> intern(Order order, Object contents) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index b571d5e1a3..37f8b2031f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java @@ -19,14 +19,10 @@ import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; -import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; -import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; @@ -38,8 +34,6 @@ import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; -import java.util.function.Function; -import java.util.stream.Collectors; import javax.annotation.Nullable; /** @@ -234,73 +228,4 @@ public class ActionExecutionValue implements SkyValue { super(artifactData, treeArtifactData, additionalOutputData, outputSymlinks); } } - - private static <V> ImmutableMap<Artifact, V> transformKeys( - ImmutableMap<Artifact, V> data, Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap) { - if (data.isEmpty()) { - return data; - } - ImmutableMap.Builder<Artifact, V> result = ImmutableMap.builderWithExpectedSize(data.size()); - for (Map.Entry<Artifact, V> entry : data.entrySet()) { - Artifact artifact = entry.getKey(); - Artifact transformedArtifact = - newArtifactMap.get(new OwnerlessArtifactWrapper(entry.getKey())); - if (transformedArtifact == null) { - // If this action generated a tree artifact, then the declared outputs of the action will - // not include the contents of the directory corresponding to that artifact, but the - // contents are present in this ActionExecutionValue as TreeFileArtifacts. We must create - // corresponding artifacts in the shared action's ActionExecutionValue. We can do that since - // a TreeFileArtifact is uniquely described by its parent, its owner, and its parent- - // relative path. Since the child was not a declared output, the child and parent must be - // generated by the same action, hence they have the same owner, and the parent was a - // declared output, so it is present in the shared action. Then we can create the new - // TreeFileArtifact to have the shared action's version of the parent artifact (instead of - // the original parent artifact); the same parent-relative path; and the new parent's - // ArtifactOwner. - Preconditions.checkState( - artifact.hasParent(), - "Output artifact %s from one shared action not present in another's outputs (%s)", - artifact, - newArtifactMap); - ArtifactOwner childOwner = artifact.getArtifactOwner(); - Artifact parent = Preconditions.checkNotNull(artifact.getParent(), artifact); - ArtifactOwner parentOwner = parent.getArtifactOwner(); - Preconditions.checkState( - parentOwner.equals(childOwner), - "A parent tree artifact %s has a different ArtifactOwner (%s) than its child %s (owned " - + "by %s), but both artifacts were generated by the same action", - parent, - parentOwner, - artifact, - childOwner); - Artifact newParent = - Preconditions.checkNotNull( - newArtifactMap.get(new OwnerlessArtifactWrapper(parent)), - "parent %s of %s was not present in shared action's data (%s)", - parent, - artifact, - newArtifactMap); - transformedArtifact = - ActionInputHelper.treeFileArtifact( - (Artifact.SpecialArtifact) newParent, artifact.getParentRelativePath()); - } - result.put(transformedArtifact, entry.getValue()); - } - return result.build(); - } - - ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) { - Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap = - outputs - .stream() - .collect(Collectors.toMap(OwnerlessArtifactWrapper::new, Function.identity())); - // This is only called for shared actions, so we'll almost certainly have to transform all keys - // in all sets. - return create( - transformKeys(artifactData, newArtifactMap), - transformKeys(treeArtifactData, newArtifactMap), - transformKeys(additionalOutputData, newArtifactMap), - outputSymlinks, - this instanceof CrossServerUnshareableActionExecutionValue); - } } 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 ac2f4cdab4..c153135c09 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 @@ -53,11 +53,11 @@ class ArtifactFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws ArtifactFunctionException, InterruptedException { - Artifact artifact = ArtifactSkyKey.artifact(skyKey); - boolean isMandatory = ArtifactSkyKey.isMandatory(skyKey); + ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument(); + Artifact artifact = artifactSkyKey.getArtifact(); if (artifact.isSourceArtifact()) { try { - return createSourceValue(artifact, 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 @@ -279,7 +279,8 @@ class ArtifactFunction implements SkyFunction { throws InterruptedException { // This artifact aggregates other artifacts. Keep track of them so callers can find them. ImmutableList.Builder<Pair<Artifact, FileArtifactValue>> inputs = ImmutableList.builder(); - for (Map.Entry<SkyKey, SkyValue> entry : env.getValues(action.getInputs()).entrySet()) { + for (Map.Entry<SkyKey, SkyValue> entry : + env.getValues(ArtifactSkyKey.mandatoryKeys(action.getInputs())).entrySet()) { Artifact input = ArtifactSkyKey.artifact(entry.getKey()); SkyValue inputValue = entry.getValue(); Preconditions.checkNotNull(inputValue, "%s has null dep %s", artifact, input); @@ -315,7 +316,7 @@ class ArtifactFunction implements SkyFunction { @Override public String extractTag(SkyKey skyKey) { - return Label.print(ArtifactSkyKey.artifact(skyKey).getOwner()); + return Label.print(((ArtifactSkyKey) skyKey.argument()).getArtifact().getOwner()); } static ActionLookupKey getActionLookupKey(Artifact artifact) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 0f07d7ee79..76179b3f14 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -303,7 +303,8 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps = env.getValuesOrThrow( - completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts(), + ArtifactSkyKey.mandatoryKeys( + completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts()), MissingInputFileException.class, ActionExecutionException.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 0e009cd78b..a415e4859f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -51,7 +51,6 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; -import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.CachedActionEvent; @@ -146,8 +145,7 @@ public final class SkyframeActionExecutor { // We don't want to execute the action again on the second entry to the SkyFunction. // In both cases, we store the already-computed ActionExecutionValue to avoid having to compute it // again. - private ConcurrentMap< - OwnerlessArtifactWrapper, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>> + private ConcurrentMap<Artifact, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>> buildActionMap; // Errors found when examining all actions in the graph are stored here, so that they can be @@ -404,16 +402,13 @@ public final class SkyframeActionExecutor { } boolean probeActionExecution(Action action) { - return buildActionMap.containsKey(new OwnerlessArtifactWrapper(action.getPrimaryOutput())); + return buildActionMap.containsKey(action.getPrimaryOutput()); } private boolean actionReallyExecuted(Action action, ActionLookupData actionLookupData) { Pair<ActionLookupData, ?> cachedRun = Preconditions.checkNotNull( - buildActionMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())), - "%s %s", - action, - actionLookupData); + buildActionMap.get(action.getPrimaryOutput()), "%s %s", action, actionLookupData); return actionLookupData.equals(cachedRun.getFirst()); } @@ -453,8 +448,7 @@ public final class SkyframeActionExecutor { actionLookupData)); // Check to see if another action is already executing/has executed this value. Pair<ActionLookupData, FutureTask<ActionExecutionValue>> oldAction = - buildActionMap.putIfAbsent( - new OwnerlessArtifactWrapper(primaryOutput), Pair.of(actionLookupData, actionTask)); + buildActionMap.putIfAbsent(primaryOutput, Pair.of(actionLookupData, actionTask)); // true if this is a non-shared action or it's shared and to be executed. boolean isPrimaryActionForTheValue = oldAction == null; @@ -465,10 +459,7 @@ public final class SkyframeActionExecutor { actionTask = oldAction.second; } try { - ActionExecutionValue value = actionTask.get(); - return isPrimaryActionForTheValue - ? value - : value.transformForSharedAction(action.getOutputs()); + return actionTask.get(); } catch (ExecutionException e) { Throwables.propagateIfPossible(e.getCause(), ActionExecutionException.class, InterruptedException.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index b0aee1bb43..fc95c3b39d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -48,6 +48,7 @@ import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier; import com.google.devtools.build.lib.actions.ArtifactRoot; +import com.google.devtools.build.lib.actions.ArtifactSkyKey; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.Executor; @@ -1303,13 +1304,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { resourceManager.resetResourceUsage(); try { progressReceiver.executionProgressReceiver = executionProgressReceiver; + Iterable<ArtifactSkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild); Iterable<TargetCompletionValue.TargetCompletionKey> targetKeys = TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext, targetsToTest); Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext); Iterable<SkyKey> testKeys = TestCompletionValue.keys(targetsToTest, topLevelArtifactContext, exclusiveTesting); return buildDriver.evaluate( - Iterables.concat(artifactsToBuild, targetKeys, aspectKeys, testKeys), + Iterables.concat(artifactKeys, targetKeys, aspectKeys, testKeys), keepGoing, numJobs, reporter); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java index 9a7d7a6aac..a1358c53fe 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java @@ -19,6 +19,7 @@ import com.google.common.collect.Multimaps; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactSkyKey; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.lib.analysis.test.TestProvider; @@ -66,9 +67,10 @@ public final class TestCompletionFunction implements SkyFunction { } } } else { - Multimap<ActionLookupValue.ActionLookupKey, Artifact> keyToArtifactMap = + Multimap<ActionLookupValue.ActionLookupKey, ArtifactSkyKey> keyToArtifactMap = Multimaps.index( - TestProvider.getTestStatusArtifacts(ct), ArtifactFunction::getActionLookupKey); + ArtifactSkyKey.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct)), + (val) -> ArtifactFunction.getActionLookupKey(val.getArtifact())); Map<SkyKey, SkyValue> actionLookupValues = env.getValues(keyToArtifactMap.keySet()); if (env.valuesMissing()) { return null; @@ -80,7 +82,7 @@ public final class TestCompletionFunction implements SkyFunction { .map( entry -> getActionLookupData( - entry.getValue(), + entry.getValue().getArtifact(), entry.getKey(), (ActionLookupValue) actionLookupValues.get(entry.getKey()))) .distinct() diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java index e4bd5b9a46..abbd3472c8 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; -import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier; @@ -412,40 +411,6 @@ public class ArtifactTest { assertThat(new Artifact(scratch.file("/newRoot/foo"), root).getRoot()).isEqualTo(root); } - @Test - public void hashCodeAndEquals() throws IOException { - Path execRoot = scratch.getFileSystem().getPath("/"); - ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, scratch.dir("/newRoot")); - ArtifactOwner firstOwner = () -> Label.parseAbsoluteUnchecked("//bar:bar"); - ArtifactOwner secondOwner = () -> Label.parseAbsoluteUnchecked("//foo:foo"); - Artifact derived1 = new Artifact(root, PathFragment.create("newRoot/shared"), firstOwner); - Artifact derived2 = new Artifact(root, PathFragment.create("newRoot/shared"), secondOwner); - ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(root.getRoot().asPath())); - Artifact source1 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), firstOwner); - Artifact source2 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), secondOwner); - new EqualsTester() - .addEqualityGroup(derived1) - .addEqualityGroup(derived2) - .addEqualityGroup(source1, source2) - .testEquals(); - assertThat(derived1.hashCode()).isEqualTo(derived2.hashCode()); - assertThat(derived1.hashCode()).isNotEqualTo(source1.hashCode()); - assertThat(source1.hashCode()).isEqualTo(source2.hashCode()); - Artifact.OwnerlessArtifactWrapper wrapper1 = new Artifact.OwnerlessArtifactWrapper(derived1); - Artifact.OwnerlessArtifactWrapper wrapper2 = new Artifact.OwnerlessArtifactWrapper(derived2); - Artifact.OwnerlessArtifactWrapper wrapper3 = new Artifact.OwnerlessArtifactWrapper(source1); - Artifact.OwnerlessArtifactWrapper wrapper4 = new Artifact.OwnerlessArtifactWrapper(source2); - new EqualsTester() - .addEqualityGroup(wrapper1, wrapper2) - .addEqualityGroup(wrapper3, wrapper4) - .testEquals(); - Path path1 = derived1.getPath(); - Path path2 = derived2.getPath(); - Path path3 = source1.getPath(); - Path path4 = source2.getPath(); - new EqualsTester().addEqualityGroup(path1, path2, path3, path4).testEquals(); - } - private Artifact createDirNameArtifact() throws Exception { return new Artifact( scratch.file("/aaa/bbb/ccc/ddd"), 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 8b32d520f9..c8f9da1a7d 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 @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.ArtifactRoot; +import com.google.devtools.build.lib.actions.ArtifactSkyKey; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -261,7 +262,9 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas } @Override public SkyValue compute(SkyKey skyKey, Environment env) { - return Preconditions.checkNotNull(artifactValueMap.get(skyKey)); + ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument(); + Artifact artifact = artifactSkyKey.getArtifact(); + return Preconditions.checkNotNull(artifactValueMap.get(artifact)); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index b6e5a369b8..8dc33e22a7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -152,7 +152,10 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase { actions.add(action); file(input2.getPath(), "contents"); file(input1.getPath(), "source contents"); - evaluate(Iterables.toArray(ImmutableSet.of(input2, input1, input2, tree), SkyKey.class)); + evaluate( + Iterables.toArray( + ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2, tree)), + SkyKey.class)); SkyValue value = evaluateArtifactValue(output); assertThat(((AggregatingArtifactValue) value).getInputs()) .containsExactly( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index 488c9378d0..b75009d447 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -896,8 +896,10 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { + ArtifactSkyKey artifactKey = (ArtifactSkyKey) skyKey.argument(); + Artifact artifact = artifactKey.getArtifact(); try { - return FileArtifactValue.create(((Artifact) skyKey).getPath()); + return FileArtifactValue.create(artifact.getPath()); } catch (IOException e) { throw new SkyFunctionException(e, Transience.PERSISTENT){}; } |