diff options
18 files changed, 296 insertions, 240 deletions
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 b71321cf7f..b68665bfd0 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,7 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.pkgcache.PackageProvider; -import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact; +import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact; import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey; import com.google.devtools.build.skyframe.CycleInfo; import com.google.devtools.build.skyframe.SkyFunctionName; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index fdbd5be422..8ec6f0ee02 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -411,8 +411,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver inputArtifactData.putAll(state.inputArtifactData); for (Map.Entry<SkyKey, SkyValue> entry : metadataFoundDuringActionExecution.entrySet()) { inputArtifactData.put( - ArtifactValue.artifact(entry.getKey()), - (FileArtifactValue) entry.getValue()); + ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue()); } state.inputArtifactData = inputArtifactData; metadataHandler = @@ -461,7 +460,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver for (Artifact artifact : discoveredInputs) { if (!inputData.containsKey(artifact)) { // Note that if the artifact is derived, the mandatory flag is ignored. - keys.add(ArtifactValue.key(artifact, /*mandatory=*/false)); + keys.add(ArtifactSkyKey.key(artifact, /*mandatory=*/ false)); } } // We do not do a getValuesOrThrow() call for the following reasons: @@ -474,7 +473,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Map<SkyKey, SkyValue> data = env.getValues(keys); if (!env.valuesMissing()) { for (Entry<SkyKey, SkyValue> entry : data.entrySet()) { - inputData.put(ArtifactValue.artifact(entry.getKey()), (FileArtifactValue) entry.getValue()); + inputData.put( + ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue()); } } } @@ -508,17 +508,19 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver if (mandatoryInputs == null) { // This is a non inputs-discovering action, so no need to distinguish mandatory from regular // inputs. - return Iterables.transform(inputs, new Function<Artifact, SkyKey>() { - @Override - public SkyKey apply(Artifact artifact) { - return ArtifactValue.key(artifact, true); - } - }); + return Iterables.transform( + inputs, + new Function<Artifact, SkyKey>() { + @Override + public SkyKey apply(Artifact artifact) { + return ArtifactSkyKey.key(artifact, true); + } + }); } else { Collection<SkyKey> discoveredArtifacts = new HashSet<>(); Set<Artifact> mandatory = Sets.newHashSet(mandatoryInputs); for (Artifact artifact : inputs) { - discoveredArtifacts.add(ArtifactValue.key(artifact, mandatory.contains(artifact))); + discoveredArtifacts.add(ArtifactSkyKey.key(artifact, mandatory.contains(artifact))); } return discoveredArtifacts; } @@ -549,9 +551,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver ActionExecutionException firstActionExecutionException = null; for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> depsEntry : inputDeps.entrySet()) { - Artifact input = ArtifactValue.artifact(depsEntry.getKey()); + Artifact input = ArtifactSkyKey.artifact(depsEntry.getKey()); try { - ArtifactValue value = (ArtifactValue) depsEntry.getValue().get(); + SkyValue value = depsEntry.getValue().get(); if (populateInputData) { if (value instanceof AggregatingArtifactValue) { AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; @@ -574,8 +576,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver expandedArtifacts.put(input, expandedTreeArtifacts); // Again, we cache the "digest" of the value for cache checking. inputArtifactData.put(input, setValue.getSelfData()); - } else if (value instanceof FileArtifactValue) { - // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed. + } else { + Preconditions.checkState(value instanceof FileArtifactValue, depsEntry); inputArtifactData.put(input, (FileArtifactValue) value); } } @@ -760,7 +762,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver new Function<SkyKey, Artifact>() { @Override public Artifact apply(SkyKey key) { - return ArtifactValue.artifact(key); + return ArtifactSkyKey.artifact(key); } }); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 681ed81a18..dd93e04784 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -57,14 +57,13 @@ import javax.annotation.Nullable; * <p>As well, this cache collects data about the action's output files, which is used in three * ways. First, it is served as requested during action execution, primarily by the {@code * ActionCacheChecker} when determining if the action must be rerun, and then after the action is - * run, to gather information about the outputs. Second, it is accessed by {@link - * ArtifactFunction}s in order to construct {@link ArtifactValue}, and by this class itself to - * generate {@link TreeArtifactValue}s. Third, the {@link - * FilesystemValueChecker} uses it to determine the set of output files to check for inter-build - * modifications. Because all these use cases are slightly different, we must occasionally store two - * versions of the data for a value. See {@link #getAdditionalOutputData} for elaboration on - * the difference between these cases, and see the javadoc for the various internal maps to see - * what is stored where. + * run, to gather information about the outputs. Second, it is accessed by {@link ArtifactFunction}s + * in order to construct {@link FileArtifactValue}s, and by this class itself to generate {@link + * TreeArtifactValue}s. Third, the {@link FilesystemValueChecker} uses it to determine the set of + * output files to check for inter-build modifications. Because all these use cases are slightly + * different, we must occasionally store two versions of the data for a value. See {@link + * #getAdditionalOutputData} for elaboration on the difference between these cases, and see the + * javadoc for the various internal maps to see what is stored where. */ @VisibleForTesting public class ActionMetadataHandler implements MetadataHandler { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java index 26d254b9eb..59572f1b9a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java @@ -23,10 +23,8 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; -import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - import javax.annotation.Nullable; /** @@ -45,7 +43,7 @@ public class ActionTemplateExpansionFunction implements SkyFunction { SpawnActionTemplate actionTemplate = key.getActionTemplate(); // Requests the TreeArtifactValue object for the input TreeArtifact. - SkyKey artifactValueKey = ArtifactValue.key(actionTemplate.getInputTreeArtifact(), true); + SkyKey artifactValueKey = ArtifactSkyKey.key(actionTemplate.getInputTreeArtifact(), true); TreeArtifactValue treeArtifactValue = (TreeArtifactValue) env.getValue(artifactValueKey); // Input TreeArtifact is not ready yet. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java index 030bf13746..2fb2199e7c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java @@ -16,11 +16,11 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.util.Pair; - +import com.google.devtools.build.skyframe.SkyValue; import java.util.Collection; /** Value for aggregating artifacts, which must be expanded to a set of other artifacts. */ -class AggregatingArtifactValue extends ArtifactValue { +class AggregatingArtifactValue implements SkyValue { private final FileArtifactValue selfData; private final ImmutableList<Pair<Artifact, FileArtifactValue>> inputs; 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 5393d198ab..f7007b7d59 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 @@ -30,7 +30,7 @@ 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.ActionLookupValue.ActionLookupKey; -import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact; +import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -41,13 +41,10 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - import java.io.IOException; import java.util.Map; -/** - * A builder for {@link ArtifactValue}s. - */ +/** A builder of values for {@link ArtifactSkyKey} keys. */ class ArtifactFunction implements SkyFunction { private final Predicate<PathFragment> allowedMissingInputs; @@ -169,7 +166,7 @@ class ArtifactFunction implements SkyFunction { return TreeArtifactValue.create(map.build()); } - private ArtifactValue createSourceValue(Artifact artifact, boolean mandatory, Environment env) + private FileArtifactValue createSourceValue(Artifact artifact, boolean mandatory, Environment env) throws MissingInputFileException { SkyKey fileSkyKey = FileValue.key(RootedPath.toRootedPath(artifact.getRoot().getPath(), artifact.getPath())); @@ -204,8 +201,9 @@ class ArtifactFunction implements SkyFunction { return allowedMissingInputs.apply(((RootedPath) fileSkyKey.argument()).getRelativePath()); } - private static ArtifactValue missingInputFile(Artifact artifact, boolean mandatory, - Exception failure, EventHandler reporter) throws MissingInputFileException { + private static FileArtifactValue missingInputFile( + Artifact artifact, boolean mandatory, Exception failure, EventHandler reporter) + throws MissingInputFileException { if (!mandatory) { return FileArtifactValue.MISSING_FILE_MARKER; } @@ -251,14 +249,17 @@ class ArtifactFunction implements SkyFunction { } } - private AggregatingArtifactValue createAggregatingValue(Artifact artifact, - ActionAnalysisMetadata action, FileArtifactValue value, SkyFunction.Environment env) { + private static AggregatingArtifactValue createAggregatingValue( + Artifact artifact, + ActionAnalysisMetadata action, + FileArtifactValue value, + SkyFunction.Environment env) { // 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(ArtifactValue.mandatoryKeys(action.getInputs())).entrySet()) { - Artifact input = ArtifactValue.artifact(entry.getKey()); - ArtifactValue inputValue = (ArtifactValue) entry.getValue(); + 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); if (!(inputValue instanceof FileArtifactValue)) { // We do not recurse in aggregating middleman artifacts. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java index d9a51504cb..4fa5d9537d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java @@ -21,23 +21,22 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; - import java.util.Collection; /** - * A value representing an artifact. Source artifacts are checked for existence, while output - * artifacts imply creation of the output file. + * 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. * - * <p>There are effectively three kinds of output artifact values. 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>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. */ @Immutable @ThreadSafe -public abstract class ArtifactValue implements SkyValue { +public final class ArtifactSkyKey { + private ArtifactSkyKey() {} @ThreadSafe public static SkyKey key(Artifact artifact, boolean isMandatory) { @@ -63,11 +62,11 @@ public abstract class ArtifactValue implements SkyValue { private static final Function<OwnedArtifact, Artifact> TO_ARTIFACT = new Function<OwnedArtifact, Artifact>() { - @Override - public Artifact apply(OwnedArtifact key) { - return key.getArtifact(); - } - }; + @Override + public Artifact apply(OwnedArtifact key) { + return key.getArtifact(); + } + }; public static Collection<Artifact> artifacts(Collection<? extends OwnedArtifact> keys) { return Collections2.transform(keys, TO_ARTIFACT); @@ -78,8 +77,7 @@ public abstract class ArtifactValue implements SkyValue { } public static boolean equalWithOwner(Artifact first, Artifact second) { - return first.equals(second) - && first.getArtifactOwner().equals(second.getArtifactOwner()); + return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner()); } /** @@ -113,11 +111,11 @@ public abstract class ArtifactValue implements SkyValue { } /** - * 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. + * 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); @@ -127,7 +125,7 @@ public abstract class ArtifactValue implements SkyValue { @Override public int hashCode() { - int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode(); + int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode(); return isMandatory ? initialHash : 47 * initialHash + 1; } 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 1f48c84f3b..db9ab97534 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 @@ -36,10 +36,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException2; - import java.util.Map; import java.util.concurrent.atomic.AtomicReference; - import javax.annotation.Nullable; /** @@ -248,7 +246,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps = env.getValuesOrThrow( - ArtifactValue.mandatoryKeys( + ArtifactSkyKey.mandatoryKeys( completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts()), MissingInputFileException.class, ActionExecutionException.class); @@ -259,7 +257,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S NestedSetBuilder<Label> rootCausesBuilder = NestedSetBuilder.stableOrder(); for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> depsEntry : inputDeps.entrySet()) { - Artifact input = ArtifactValue.artifact(depsEntry.getKey()); + Artifact input = ArtifactSkyKey.artifact(depsEntry.getKey()); try { depsEntry.getValue().get(); } catch (MissingInputFileException e) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java index 1e7a4c7ca2..dcbe1bc236 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java @@ -20,68 +20,194 @@ import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; import java.util.Arrays; import javax.annotation.Nullable; /** * Stores the actual metadata data of a file. We have the following cases: - * <ul><li> - * an ordinary file, in which case we would expect to see a digest and size; - * </li><li> - * a directory, in which case we would expect to see an mtime; - * </li><li> - * an intentionally omitted file which the build system is aware of but doesn't actually exist, - * where all access methods are unsupported; - * </li><li> - * The "self data" of a middleman artifact or TreeArtifact, where we would expect to see a digest - * representing the artifact's contents, and a size of 1. - * </li></ul> + * + * <ul> + * <li> an ordinary file, in which case we would expect to see a digest and size; + * <li> a directory, in which case we would expect to see an mtime; + * <li> an intentionally omitted file which the build system is aware of but doesn't actually exist, + * where all access methods are unsupported; + * <li> a "middleman marker" object, which has a null digest, 0 size, and mtime of 0. + * <li> The "self data" of a TreeArtifact, where we would expect to see a digest representing the + * artifact's contents, and a size of 0. + * </ul> */ -public class FileArtifactValue extends ArtifactValue { - /** Data for Middleman artifacts that did not have data specified. */ - static final FileArtifactValue DEFAULT_MIDDLEMAN = new FileArtifactValue(null, 0, 0); - /** Data that marks that a file is not present on the filesystem. */ - @VisibleForTesting - public static final FileArtifactValue MISSING_FILE_MARKER = new FileArtifactValue(null, 1, 0) { +// TODO(janakr): make this an interface once JDK8 allows us to have static methods on interfaces. +public abstract class FileArtifactValue implements SkyValue { + private static final class SingletonMarkerValue extends FileArtifactValue { + @Nullable + @Override + public byte[] getDigest() { + return null; + } + @Override - public boolean exists() { + boolean isFile() { return false; } - }; + + @Override + public long getSize() { + return 0; + } + + @Override + public long getModifiedTime() { + return 0; + } + + @Override + public String toString() { + return "singleton marker artifact value (" + hashCode() + ")"; + } + } + + static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue(); + /** Data that marks that a file is not present on the filesystem. */ + @VisibleForTesting + public static final FileArtifactValue MISSING_FILE_MARKER = new SingletonMarkerValue(); /** - * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods - * are unsupported. + * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods are + * unsupported. */ - static final FileArtifactValue OMITTED_FILE_MARKER = new FileArtifactValue(null, 2, 0) { - @Override public byte[] getDigest() { throw new UnsupportedOperationException(); } - @Override public boolean isFile() { throw new UnsupportedOperationException(); } - @Override public long getSize() { throw new UnsupportedOperationException(); } - @Override public long getModifiedTime() { throw new UnsupportedOperationException(); } - @Override public boolean equals(Object o) { return this == o; } - @Override public int hashCode() { return System.identityHashCode(this); } - @Override public String toString() { return "OMITTED_FILE_MARKER"; } - }; - - @Nullable private final byte[] digest; - private final long mtime; - private final long size; - - private FileArtifactValue(byte[] digest, long size) { - this.digest = Preconditions.checkNotNull(digest, size); - this.size = size; - this.mtime = -1; + static final FileArtifactValue OMITTED_FILE_MARKER = + new FileArtifactValue() { + @Override + public byte[] getDigest() { + throw new UnsupportedOperationException(); + } + + @Override + boolean isFile() { + throw new UnsupportedOperationException(); + } + + @Override + public long getSize() { + throw new UnsupportedOperationException(); + } + + @Override + public long getModifiedTime() { + throw new UnsupportedOperationException(); + } + + @Override + public String toString() { + return "OMITTED_FILE_MARKER"; + } + }; + + private static final class DirectoryArtifactValue extends FileArtifactValue { + private final long mtime; + + private DirectoryArtifactValue(long mtime) { + this.mtime = mtime; + } + + @Nullable + @Override + public byte[] getDigest() { + return null; + } + + @Override + public long getModifiedTime() { + return mtime; + } + + @Override + public long getSize() { + return 0; + } + + @Override + public boolean isFile() { + return false; + } + + @Override + public int hashCode() { + return (int) mtime; + } + + @Override + public boolean equals(Object other) { + return (this == other) + || ((other instanceof DirectoryArtifactValue) + && this.mtime == ((DirectoryArtifactValue) other).mtime); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("mtime", mtime).toString(); + } } - // Only used by directories (null digest). - private FileArtifactValue(byte[] digest, long mtime, long size) { - Preconditions.checkState(mtime >= 0, "mtime must be non-negative: %s %s", mtime, size); - Preconditions.checkState(size == 0, "size must be zero: %s %s", mtime, size); - Preconditions.checkState(digest == null, "digest must be null:"); - this.digest = digest; - this.size = size; - this.mtime = mtime; + private static final class RegularFileArtifactValue extends FileArtifactValue { + private final byte[] digest; + private final long size; + + private RegularFileArtifactValue(byte[] digest, long size) { + this.digest = Preconditions.checkNotNull(digest); + this.size = size; + } + + @Override + public byte[] getDigest() { + return digest; + } + + @Override + boolean isFile() { + return true; + } + + @Override + public long getSize() { + return size; + } + + @Override + public long getModifiedTime() { + throw new UnsupportedOperationException( + "regular file's mtime should never be called. (" + this + ")"); + } + + @Override + public int hashCode() { + // Hash digest by content, not reference. + return 37 * (int) size + Arrays.hashCode(digest); + } + + /** + * Two RegularFileArtifactValues will only compare equal if they have the same content. This + * differs from the {@code Metadata#equivalence} method, which allows for comparison using mtime + * if one object does not have a digest available. + */ + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (!(other instanceof RegularFileArtifactValue)) { + return false; + } + RegularFileArtifactValue that = (RegularFileArtifactValue) other; + return this.size == that.size && Arrays.equals(this.digest, that.digest); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("digest", digest).add("size", size).toString(); + } } @VisibleForTesting @@ -107,10 +233,10 @@ public class FileArtifactValue extends ArtifactValue { // In this case, we need to store the mtime because the action cache uses mtime for // directories to determine if this artifact has changed. We want this code path to go away // somehow (maybe by implementing FileSet in Skyframe). - return new FileArtifactValue(digest, artifact.getPath().getLastModifiedTime(), size); + return new DirectoryArtifactValue(artifact.getPath().getLastModifiedTime()); } Preconditions.checkState(digest != null, artifact); - return new FileArtifactValue(digest, size); + return new RegularFileArtifactValue(digest, size); } /** @@ -119,65 +245,19 @@ public class FileArtifactValue extends ArtifactValue { */ static FileArtifactValue createProxy(byte[] digest) { Preconditions.checkNotNull(digest); - // The Middleman artifact values have size 1 because we want their digests to be used. This hack - // can be removed once empty files are digested. - return new FileArtifactValue(digest, /*size=*/1); + return new RegularFileArtifactValue(digest, /*size=*/ 0); } + /** Returns the digest of this value. Null for non-files, non-null for files. */ @Nullable - public byte[] getDigest() { - return digest; - } + public abstract byte[] getDigest(); /** @return true if this is a file or a symlink to an existing file */ - boolean isFile() { - return digest != null; - } - - /** Gets the size of the file. Directories have size 0. */ - public long getSize() { - return size; - } - - /** Gets last modified time of file. Should only be called if this is a directory. */ - long getModifiedTime() { - Preconditions.checkState(size == 0 && digest == null, "%s %s %s", digest, mtime, size); - return mtime; - } - - public boolean exists() { - return true; - } + abstract boolean isFile(); - @Override - public int hashCode() { - // Hash digest by content, not reference. Note that digest is the only array in this array. - return Arrays.deepHashCode(new Object[] {size, mtime, digest}); - } + /** Gets the size of the file. Non-files (including directories) have size 0. */ + public abstract long getSize(); - /** - * Two FileArtifactValues will only compare equal if they have the same content. This differs - * from the {@code Metadata#equivalence} method, which allows for comparison using mtime if - * one object does not have a digest available. - */ - @Override - public boolean equals(Object other) { - if (this == other) { - return true; - } - if (!(other instanceof FileArtifactValue)) { - return false; - } - FileArtifactValue that = (FileArtifactValue) other; - return this.mtime == that.mtime && this.size == that.size - && Arrays.equals(this.digest, that.digest); - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(FileArtifactValue.class) - .add("digest", digest) - .add("mtime", mtime) - .add("size", size).toString(); - } + /** Gets last modified time of file. Should only be called if this is not a file. */ + abstract long getModifiedTime(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java index d4ce6caa77..6b2bfa7216 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java @@ -21,30 +21,26 @@ import com.google.devtools.build.lib.vfs.PathFragment; 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.Objects; - import javax.annotation.Nullable; /** * A value that corresponds to a file (or directory or symlink or non-existent file), fully * accounting for symlinks (e.g. proper dependencies on ancestor symlinks so as to be incrementally * correct). Anything in Skyframe that cares about the fully resolved path of a file (e.g. anything - * that cares about the contents of a file) should have a dependency on the corresponding - * {@link FileValue}. + * that cares about the contents of a file) should have a dependency on the corresponding {@link + * FileValue}. * - * <p> - * Note that the existence of a file value does not imply that the file exists on the filesystem. + * <p>Note that the existence of a file value does not imply that the file exists on the filesystem. * File values for missing files will be created on purpose in order to facilitate incremental * builds in the case those files have reappeared. * - * <p> - * This class contains the relevant metadata for a file, although not the contents. Note that + * <p>This class contains the relevant metadata for a file, although not the contents. Note that * since a FileValue doesn't store its corresponding SkyKey, it's possible for the FileValues for * two different paths to be the same. * - * <p> - * This should not be used for build outputs; use {@link ArtifactValue} for those. + * <p>This should not be used for build outputs; use {@link ArtifactSkyKey} to create keys for + * those. */ @Immutable @ThreadSafe 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 f9426d1b70..a735261a9e 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 @@ -137,7 +137,6 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.WalkableGraph.WalkableGraphFactory; import com.google.devtools.common.options.OptionsClassProvider; - import java.io.IOException; import java.io.PrintStream; import java.util.ArrayList; @@ -155,7 +154,6 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; - import javax.annotation.Nullable; /** @@ -1089,7 +1087,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { resourceManager.resetResourceUsage(); try { progressReceiver.executionProgressReceiver = executionProgressReceiver; - Iterable<SkyKey> artifactKeys = ArtifactValue.mandatoryKeys(artifactsToBuild); + Iterable<SkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild); Iterable<SkyKey> targetKeys = TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext); Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext); @@ -1743,7 +1741,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { + "Please remove '" + directories.getInstallBase() + "' and try again."; throw new AbruptExitException(message, ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e); } - values.put(ArtifactValue.key(artifact, /*isMandatory=*/true), fileArtifactValue); + values.put(ArtifactSkyKey.key(artifact, /*isMandatory=*/ true), fileArtifactValue); } injectable().inject(values); needToInjectEmbeddedArtifacts = false; 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 cdb22dd3e9..d277b1955a 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 @@ -53,12 +53,12 @@ public final class TestCompletionFunction implements SkyFunction { if (key.exclusiveTesting()) { // Request test artifacts iteratively if testing exclusively. for (Artifact testArtifact : TestProvider.getTestStatusArtifacts(ct)) { - if (env.getValue(ArtifactValue.key(testArtifact, /*isMandatory=*/true)) == null) { + if (env.getValue(ArtifactSkyKey.key(testArtifact, /*isMandatory=*/ true)) == null) { return null; } } } else { - env.getValues(ArtifactValue.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct))); + env.getValues(ArtifactSkyKey.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct))); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index a0f2241531..bddd5925af 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -26,20 +26,18 @@ import com.google.devtools.build.lib.actions.cache.Digest; import com.google.devtools.build.lib.actions.cache.Metadata; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; - +import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; - import java.util.Arrays; import java.util.Map; import java.util.Set; - import javax.annotation.Nullable; /** - * Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s - * of its child {@link TreeFileArtifact}s. + * Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s of its child + * {@link TreeFileArtifact}s. */ -public class TreeArtifactValue extends ArtifactValue { +public class TreeArtifactValue implements SkyValue { private static final Function<Artifact, PathFragment> PARENT_RELATIVE_PATHS = new Function<Artifact, PathFragment>() { @Override 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 790429a735..1b725b367a 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 @@ -35,7 +35,7 @@ import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate; 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.ArtifactValue.OwnedArtifact; +import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact; import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.Path; @@ -49,22 +49,20 @@ import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; /** Tests for {@link ActionTemplateExpansionFunction}. */ @RunWith(JUnit4.class) public final class ActionTemplateExpansionFunctionTest extends FoundationTestCase { - private Map<Artifact, ArtifactValue> artifactValueMap; + private Map<Artifact, TreeArtifactValue> artifactValueMap; private SequentialBuildDriver driver; @Before @@ -225,9 +223,9 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas /** Dummy ArtifactFunction that just returns injected values */ private static class DummyArtifactFunction implements SkyFunction { - private final Map<Artifact, ArtifactValue> artifactValueMap; + private final Map<Artifact, TreeArtifactValue> artifactValueMap; - DummyArtifactFunction(Map<Artifact, ArtifactValue> artifactValueMap) { + DummyArtifactFunction(Map<Artifact, TreeArtifactValue> artifactValueMap) { this.artifactValueMap = artifactValueMap; } @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 1ba51c8bde..dcd7a3359b 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 @@ -49,12 +49,6 @@ import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - import java.io.IOException; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; @@ -62,6 +56,10 @@ import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; import java.util.Map; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; /** * Tests for {@link ArtifactFunction}. @@ -185,8 +183,8 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase { file(input1.getPath(), "source contents"); evaluate( Iterables.toArray( - ArtifactValue.mandatoryKeys(ImmutableSet.of(input2, input1, input2)), SkyKey.class)); - ArtifactValue value = evaluateArtifactValue(output); + ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2)), SkyKey.class)); + SkyValue value = evaluateArtifactValue(output); assertThat(((AggregatingArtifactValue) value).getInputs()) .containsExactly(Pair.of(input1, create(input1)), Pair.of(input2, create(input2))); } @@ -247,7 +245,7 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase { try { value.getModifiedTime(); fail("mtime for non-empty file should not be stored."); - } catch (IllegalStateException e) { + } catch (UnsupportedOperationException e) { // Expected. } } @@ -451,14 +449,13 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase { return ((FileArtifactValue) evaluateArtifactValue(artifact)); } - private ArtifactValue evaluateArtifactValue(Artifact artifact) throws Throwable { + private SkyValue evaluateArtifactValue(Artifact artifact) throws Throwable { return evaluateArtifactValue(artifact, /*isMandatory=*/ true); } - private ArtifactValue evaluateArtifactValue(Artifact artifact, boolean mandatory) - throws Throwable { - SkyKey key = ArtifactValue.key(artifact, mandatory); - EvaluationResult<ArtifactValue> result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0])); + private SkyValue evaluateArtifactValue(Artifact artifact, boolean mandatory) throws Throwable { + SkyKey key = ArtifactSkyKey.key(artifact, mandatory); + EvaluationResult<SkyValue> result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0])); if (result.hasError()) { throw result.getError().getException(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java index 17a273ebe5..de989417da 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java @@ -40,12 +40,6 @@ import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationS import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -53,8 +47,11 @@ import java.nio.charset.StandardCharsets; import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicInteger; - import javax.annotation.Nullable; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; /** Tests for {@link SkyframeAwareAction}. */ @RunWith(JUnit4.class) @@ -740,7 +737,7 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase { // ArtifactFunction(genfiles/gen0) | return FileValue(genfiles/foo:non-existent) // CONFIGURED_TARGET://foo:gen0 | // ACTION_EXECUTION:gen0_action | MockFunction() - // return ArtifactValue(genfiles/gen0) | FILE:genfiles/foo + // return ArtifactSkyKey(genfiles/gen0) | FILE:genfiles/foo // | FILE:genfiles/foo/bar/gen1 // ActionExecutionFunction(gen1_action) | env.valuesMissing():yes ==> return // ARTIFACT:genfiles/gen0 | diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index de3984d617..037a9e8620 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -73,9 +73,6 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - -import org.junit.Before; - import java.io.IOException; import java.io.PrintStream; import java.util.ArrayList; @@ -88,8 +85,8 @@ import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; - import javax.annotation.Nullable; +import org.junit.Before; /** * The common code that's shared between various builder tests. @@ -203,9 +200,8 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { return new Builder() { private void setGeneratingActions() { if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) { - differencer.inject(ImmutableMap.of( - OWNER_KEY, - new ActionLookupValue(ImmutableList.copyOf(actions)))); + differencer.inject( + ImmutableMap.of(OWNER_KEY, new ActionLookupValue(ImmutableList.copyOf(actions)))); } } @@ -229,11 +225,12 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { executor, keepGoing, /*explain=*/ false, - new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, false), null); + new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, false), + null); List<SkyKey> keys = new ArrayList<>(); for (Artifact artifact : artifacts) { - keys.add(ArtifactValue.key(artifact, true)); + keys.add(ArtifactSkyKey.key(artifact, true)); } setGeneratingActions(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index 3fe2c2b412..8d8b5115e7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -188,10 +188,9 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase { return output; } - private ArtifactValue evaluateArtifactValue(Artifact artifact, boolean mandatory) - throws Exception { - SkyKey key = ArtifactValue.key(artifact, mandatory); - EvaluationResult<ArtifactValue> result = evaluate(key); + private SkyValue evaluateArtifactValue(Artifact artifact, boolean mandatory) throws Exception { + SkyKey key = ArtifactSkyKey.key(artifact, mandatory); + EvaluationResult<SkyValue> result = evaluate(key); if (result.hasError()) { throw result.getError().getException(); } |