diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib/actions')
5 files changed, 116 insertions, 177 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 5364316f0c..a3a6ba6b58 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,7 +17,6 @@ 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; @@ -25,6 +24,7 @@ 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,15 +74,39 @@ 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 (!Iterables.elementsEqual(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) { + if (!artifactsEqualWithoutOwner(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) { return false; } - if (!Iterables.elementsEqual(actionA.getOutputs(), actionB.getOutputs())) { + if (!artifactsEqualWithoutOwner(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 f21d579aed..49dad3ff38 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -22,14 +22,14 @@ import com.google.common.base.Functions; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Interner; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType; import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.shell.ShellUtils; import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; @@ -55,7 +56,6 @@ import java.util.Collection; import java.util.Comparator; import java.util.List; import java.util.Objects; -import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; /** @@ -110,7 +110,8 @@ public class Artifact ActionInput, FileApi, Comparable<Object>, - CommandLineItem { + CommandLineItem, + SkyKey { /** Compares artifact according to their exec paths. Sorts null values first. */ @SuppressWarnings("ReferenceEquality") // "a == b" is an optimization @@ -167,8 +168,7 @@ public class Artifact /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */ public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact(); - private static final Cache<InternedArtifact, Artifact> ARTIFACT_INTERNER = - CacheBuilder.newBuilder().weakValues().build(); + private static final Interner<Artifact> ARTIFACT_INTERNER = BlazeInterners.newWeakInterner(); private final int hashCode; private final ArtifactRoot root; @@ -203,15 +203,7 @@ public class Artifact if (artifact.isSourceArtifact()) { return artifact; } else { - return intern(artifact); - } - } - - private static Artifact intern(Artifact artifact) { - try { - return ARTIFACT_INTERNER.get(new InternedArtifact(artifact), () -> artifact); - } catch (ExecutionException e) { - throw new IllegalStateException(e); + return ARTIFACT_INTERNER.intern(artifact); } } @@ -257,6 +249,9 @@ public class Artifact throw new IllegalArgumentException( "it is illegal to create an artifact with an empty execPath"); } + // The ArtifactOwner is not part of this computation because it is very rare that two Artifacts + // have the same execPath and different owners, so a collision is fine there. If this is + // changed, OwnerlessArtifactWrapper must also be changed. this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13; this.root = root; this.execPath = execPath; @@ -484,6 +479,15 @@ public class Artifact public SourceArtifact asSourceArtifact() { return this; } + + /** + * SourceArtifacts are compared without their owners, since owners do not affect behavior, + * unlike with derived artifacts, whose owners determine their generating actions. + */ + @Override + public boolean equals(Object other) { + return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other); + } } /** @@ -614,34 +618,6 @@ public class Artifact } /** - * Wraps an artifact for interning because we need to check the artifact owner when doing equals. - */ - private static final class InternedArtifact { - private final Artifact wrappedArtifact; - - InternedArtifact(Artifact artifact) { - this.wrappedArtifact = artifact; - } - - @Override - public boolean equals(Object other) { - if (!(other instanceof InternedArtifact)) { - return false; - } - if (!getClass().equals(other.getClass())) { - return false; - } - InternedArtifact that = (InternedArtifact) other; - return Artifact.equalWithOwner(wrappedArtifact, that.wrappedArtifact); - } - - @Override - public final int hashCode() { - return wrappedArtifact.hashCode(); - } - } - - /** * Returns the relative path to this artifact relative to its root. (Useful * when deriving output filenames from input files, etc.) */ @@ -693,37 +669,28 @@ public class Artifact return rootRelativePath.toString(); } + @SuppressWarnings("EqualsGetClass") // Distinct classes of Artifact are never equal. @Override - public final boolean equals(Object other) { + public boolean equals(Object other) { if (!(other instanceof Artifact)) { return false; } if (!getClass().equals(other.getClass())) { return false; } - // We don't bother to check root in the equivalence relation, because we - // assume that no root is an ancestor of another one. Artifact that = (Artifact) other; - return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root); + return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner()); } - /** - * Compare equality including Artifact owner equality, a notable difference compared to the - * {@link #equals(Object)} method of {@link Artifact}. - */ - public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) { - if (first != null) { - return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner()); - } else { - return second == null; - } + public boolean equalsWithoutOwner(Artifact other) { + return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root); } @Override public final int hashCode() { - // This is just path.hashCode(). We cache a copy in the Artifact object to reduce LLC misses - // during operations which build a HashSet out of many Artifacts. This is a slight loss for - // memory but saves ~1% overall CPU in some real builds. + // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact + // object to reduce LLC misses during operations which build a HashSet out of many Artifacts. + // This is a slight loss for memory but saves ~1% overall CPU in some real builds. return hashCode; } @@ -1031,4 +998,33 @@ public class Artifact printer.append("<generated file " + rootRelativePath + ">"); } } + + /** + * A utility class that compares {@link Artifact}s without taking their owners into account. + * Should only be used for detecting action conflicts and merging shared action data. + */ + public static class OwnerlessArtifactWrapper { + private final Artifact artifact; + + public OwnerlessArtifactWrapper(Artifact artifact) { + this.artifact = artifact; + } + + @Override + public int hashCode() { + // Depends on the fact that Artifact#hashCode does not use ArtifactOwner. + return artifact.hashCode(); + } + + @Override + public boolean equals(Object obj) { + return obj instanceof OwnerlessArtifactWrapper + && this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact); + } + } + + @Override + public SkyFunctionName functionName() { + return ARTIFACT; + } } 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 277b26933b..701aacaec4 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,99 +13,52 @@ // 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}. Source artifacts are checked for existence, - * while output artifacts imply creation of the output file. + * A {@link SkyKey} coming from an {@link Artifact} that is not mandatory: absence of the Artifact + * does not imply any error. * - * <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. + * <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). */ +// 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 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 final Artifact.SourceArtifact artifact; - private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) { - Preconditions.checkArgument(sourceArtifact.isSourceArtifact()); + private ArtifactSkyKey(Artifact.SourceArtifact sourceArtifact) { this.artifact = Preconditions.checkNotNull(sourceArtifact); - this.isMandatory = mandatory; - } - - /** - * 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. } @ThreadSafety.ThreadSafe - @AutoCodec.Instantiator - public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) { - return INTERNER.intern( - artifact.isSourceArtifact() - ? new ArtifactSkyKey(artifact, isMandatory) - : new ArtifactSkyKey(artifact)); + public static SkyKey key(Artifact artifact, boolean isMandatory) { + if (isMandatory || !artifact.isSourceArtifact()) { + return artifact; + } + return create(((Artifact.SourceArtifact) artifact)); } - @ThreadSafety.ThreadSafe - public static Iterable<ArtifactSkyKey> mandatoryKeys(Iterable<Artifact> artifacts) { - return Iterables.transform(artifacts, TO_MANDATORY_KEY); + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static ArtifactSkyKey create(Artifact.SourceArtifact artifact) { + return INTERNER.intern(new ArtifactSkyKey(artifact)); } - public static Collection<Artifact> artifacts(Collection<? extends ArtifactSkyKey> keys) { - return Collections2.transform(keys, TO_ARTIFACT); + public static Artifact artifact(SkyKey key) { + return key instanceof Artifact ? (Artifact) key : ((ArtifactSkyKey) key).artifact; } - public static Artifact artifact(SkyKey key) { - return TO_ARTIFACT.apply((ArtifactSkyKey) key.argument()); + public static boolean isMandatory(SkyKey key) { + return key instanceof Artifact; } @Override @@ -115,32 +68,7 @@ public class ArtifactSkyKey implements SkyKey { @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; + return artifact.hashCode(); } @Override @@ -152,24 +80,13 @@ public class ArtifactSkyKey implements SkyKey { return false; } ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that); - return Artifact.equalWithOwner(artifact, thatArtifactSkyKey.artifact) - && isMandatory == thatArtifactSkyKey.isMandatory; + return artifact.equals(thatArtifactSkyKey.artifact); } 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 7740a05b51..75907d4f24 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,6 +26,7 @@ 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; /** @@ -111,9 +112,7 @@ public interface FilesetTraversalParams { if (o instanceof FilesetTraversalParams.DirectTraversalRoot) { FilesetTraversalParams.DirectTraversalRoot that = (FilesetTraversalParams.DirectTraversalRoot) o; - // 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()) + return Objects.equals(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 f35f963bf7..dadad155e6 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,6 +15,7 @@ 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; @@ -24,7 +25,7 @@ import javax.annotation.concurrent.ThreadSafe; @ThreadSafe public final class MapBasedActionGraph implements MutableActionGraph { private final ActionKeyContext actionKeyContext; - private final ConcurrentMultimapWithHeadElement<Artifact, ActionAnalysisMetadata> + private final ConcurrentMultimapWithHeadElement<OwnerlessArtifactWrapper, ActionAnalysisMetadata> generatingActionMap = new ConcurrentMultimapWithHeadElement<>(); public MapBasedActionGraph(ActionKeyContext actionKeyContext) { @@ -34,17 +35,18 @@ public final class MapBasedActionGraph implements MutableActionGraph { @Override @Nullable public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { - return generatingActionMap.get(artifact); + return generatingActionMap.get(new OwnerlessArtifactWrapper(artifact)); } @Override public void registerAction(ActionAnalysisMetadata action) throws ActionConflictException { for (Artifact artifact : action.getOutputs()) { - ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(artifact, action); + OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact); + ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(wrapper, action); if (previousAction != null && previousAction != action && !Actions.canBeShared(actionKeyContext, action, previousAction)) { - generatingActionMap.remove(artifact, action); + generatingActionMap.remove(wrapper, action); throw new ActionConflictException(actionKeyContext, artifact, previousAction, action); } } @@ -53,8 +55,9 @@ public final class MapBasedActionGraph implements MutableActionGraph { @Override public void unregisterAction(ActionAnalysisMetadata action) { for (Artifact artifact : action.getOutputs()) { - generatingActionMap.remove(artifact, action); - ActionAnalysisMetadata otherAction = generatingActionMap.get(artifact); + OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact); + generatingActionMap.remove(wrapper, action); + ActionAnalysisMetadata otherAction = generatingActionMap.get(wrapper); Preconditions.checkState( otherAction == null || (otherAction != action |