diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build')
5 files changed, 136 insertions, 170 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 57abd35703..1a6c4c8862 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -50,6 +50,28 @@ import javax.annotation.Nullable; * otherwise lightweight, and should be constructed anew and discarded for each build request. */ public class ActionCacheChecker { + private static final Metadata CONSTANT_METADATA = new Metadata() { + @Override + public boolean isFile() { + return false; + } + + @Override + public byte[] getDigest() { + throw new UnsupportedOperationException(); + } + + @Override + public long getSize() { + return 0; + } + + @Override + public long getModifiedTime() { + return -1; + } + }; + private final ActionCache actionCache; private final Predicate<? super Action> executionFilter; private final ArtifactResolver artifactResolver; @@ -294,7 +316,7 @@ public class ActionCacheChecker { private static Metadata getMetadataOrConstant(MetadataHandler metadataHandler, Artifact artifact) throws IOException { if (artifact.isConstantMetadata()) { - return Metadata.CONSTANT_METADATA; + return CONSTANT_METADATA; } else { return metadataHandler.getMetadata(artifact); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/Metadata.java b/src/main/java/com/google/devtools/build/lib/actions/cache/Metadata.java index 0f5a037ac9..642bd29593 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/Metadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/Metadata.java @@ -14,104 +14,43 @@ package com.google.devtools.build.lib.actions.cache; -import com.google.common.io.BaseEncoding; -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 java.util.Arrays; -import java.util.Date; - /** - * A class to represent file metadata. - * ActionCacheChecker may assume that, for a given file, equal - * metadata at different moments implies equal file-contents, - * where metadata equality is computed using Metadata.equals(). - * <p> - * NB! Several other parts of Blaze are relying on the fact that metadata - * uses mtime and not ctime. If metadata is ever changed - * to use ctime, all uses of Metadata must be carefully examined. + * An interface to represent the state of a file (or directory). This is used to determine whether a + * file has changed or not. + * + * <p>NB! Several other parts of Blaze are relying on the fact that metadata uses mtime and not + * ctime. If metadata is ever changed to use ctime, all uses of Metadata must be carefully examined. */ -@Immutable @ThreadSafe -public final class Metadata { - private final long mtime; - private final byte[] digest; - - // Convenience object for use with volatile files that we do not want checked - // (e.g. the build-changelist.txt) - public static final Metadata CONSTANT_METADATA = new Metadata(-1); - +public interface Metadata { /** - * Construct an instance for a directory with the specified mtime. The {@link #isFile} method - * returns true if and only if a digest is set. + * Marker interface for singleton implementations of the Metadata interface. This is only needed + * for a correct implementation of {@code equals}. */ - public Metadata(long mtime) { - this.mtime = mtime; - this.digest = null; + public interface Singleton { } /** - * Construct an instance for a file with the specified digest. The {@link #isFile} method returns - * true if and only if a digest is set. + * Whether the underlying file system object is a file or a symlink to a file, rather than a + * directory. All files are guaranteed to have a digest, and {@link #getDigest} must only be + * called on files. */ - public Metadata(byte[] digest) { - this.mtime = 0L; - this.digest = Preconditions.checkNotNull(digest); - } - - public boolean isFile() { - return digest != null; - } + boolean isFile(); /** - * Returns the digest for the underlying file system object. + * Returns the file's digest; must only be called on objects for which {@link #isFile} returns + * true. * * <p>The return value is owned by the cache and must not be modified. */ - public byte[] getDigest() { - Preconditions.checkState(digest != null); - return digest; - } + byte[] getDigest(); - public long getModifiedTime() { - return mtime; - } + /** Returns the file's size, or 0 if the underlying file system object is not a file. */ + // TODO(ulfjack): Throw an exception if it's not a file. + long getSize(); - @Override - public int hashCode() { - int hash = 0; - if (digest != null) { - // We are already dealing with the digest so we can just use portion of it - // as a hash code. - hash += digest[0] + (digest[1] << 8) + (digest[2] << 16) + (digest[3] << 24); - } else { - // Inlined hashCode for Long, so we don't - // have to construct an Object, just to compute - // a 32-bit hash out of a 64 bit value. - hash = (int) (mtime ^ (mtime >>> 32)); - } - return hash; - } - - @Override - public boolean equals(Object that) { - if (this == that) { - return true; - } - if (!(that instanceof Metadata)) { - return false; - } - // Do a strict comparison - both digest and mtime should match - return Arrays.equals(this.digest, ((Metadata) that).digest) - && this.mtime == ((Metadata) that).mtime; - } - - @Override - public String toString() { - if (digest != null) { - return "MD5 " + BaseEncoding.base16().lowerCase().encode(digest); - } else if (mtime > 0) { - return "timestamp " + new Date(mtime); - } - return "no metadata"; - } + /** + * Returns the last modified time; must only be called on objects for which {@link #isFile} + * returns false. + */ + long getModifiedTime(); } 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 47cd18f278..3027a3841b 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 @@ -153,10 +153,7 @@ public class ActionMetadataHandler implements MetadataHandler { || value == FileArtifactValue.OMITTED_FILE_MARKER) { throw new FileNotFoundException(); } - // If the file is a directory, we need to return the mtime because the action cache uses mtime - // to determine if this artifact has changed. We want this code path to go away somehow - // for directories (maybe by implementing FileSet in Skyframe). - return value.isFile() ? new Metadata(value.getDigest()) : new Metadata(value.getModifiedTime()); + return value; } @Nullable @@ -218,7 +215,7 @@ public class ActionMetadataHandler implements MetadataHandler { if (!fileValue.exists()) { throw new FileNotFoundException(artifact.prettyPrint() + " does not exist"); } - return new Metadata(Preconditions.checkNotNull(fileValue.getDigest(), artifact)); + return FileArtifactValue.createNormalFile(fileValue); } // We do not cache exceptions besides nonexistence here, because it is unlikely that the file // will be requested from this cache too many times. @@ -256,7 +253,7 @@ public class ActionMetadataHandler implements MetadataHandler { if (isFile && !artifact.hasParent() && data.getDigest() != null) { // We do not need to store the FileArtifactValue separately -- the digest is in the file value // and that is all that is needed for this file's metadata. - return new Metadata(data.getDigest()); + return FileArtifactValue.createNormalFile(data); } // Unfortunately, the FileValue does not contain enough information for us to calculate the // corresponding FileArtifactValue -- either the metadata must use the modified time, which we 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 cfdcb187de..2f06471ce4 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 @@ -17,6 +17,9 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.cache.DigestUtils; +import com.google.devtools.build.lib.actions.cache.Metadata; +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.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.Path; @@ -39,8 +42,9 @@ import javax.annotation.Nullable; * </ul> */ // 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 { +@Immutable @ThreadSafe +public abstract class FileArtifactValue implements SkyValue, Metadata { + private static final class SingletonMarkerValue extends FileArtifactValue implements Singleton { @Nullable @Override public byte[] getDigest() { @@ -48,7 +52,7 @@ public abstract class FileArtifactValue implements SkyValue { } @Override - boolean isFile() { + public boolean isFile() { return false; } @@ -68,6 +72,33 @@ public abstract class FileArtifactValue implements SkyValue { } } + private static final class OmittedFileValue extends FileArtifactValue implements Singleton { + @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 String toString() { + return "OMITTED_FILE_MARKER"; + } + } + static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue(); /** Data that marks that a file is not present on the filesystem. */ @VisibleForTesting @@ -77,33 +108,7 @@ public abstract class FileArtifactValue implements SkyValue { * 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() { - @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"; - } - }; + static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue(); private static final class DirectoryArtifactValue extends FileArtifactValue { private final long mtime; @@ -134,18 +139,6 @@ public abstract class FileArtifactValue implements SkyValue { } @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(); } @@ -166,7 +159,7 @@ public abstract class FileArtifactValue implements SkyValue { } @Override - boolean isFile() { + public boolean isFile() { return true; } @@ -182,29 +175,6 @@ public abstract class FileArtifactValue implements SkyValue { } @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(); } @@ -239,10 +209,18 @@ public abstract class FileArtifactValue implements SkyValue { return createNormalFile(digest, size); } - static FileArtifactValue createNormalFile(byte[] digest, long size) { + public static FileArtifactValue createNormalFile(byte[] digest, long size) { return new RegularFileArtifactValue(digest, size); } + static FileArtifactValue createNormalFile(FileValue fileValue) { + return new RegularFileArtifactValue(fileValue.getDigest(), fileValue.getSize()); + } + + public static FileArtifactValue createDirectory(long mtime) { + return new DirectoryArtifactValue(mtime); + } + /** * Creates a FileArtifactValue used as a 'proxy' input for other ArtifactValues. * These are used in {@link com.google.devtools.build.lib.actions.ActionCacheChecker}. @@ -252,16 +230,48 @@ public abstract class FileArtifactValue implements SkyValue { return createNormalFile(digest, /*size=*/ 0); } - /** Returns the digest of this value. Null for non-files, non-null for files. */ + @Override + public abstract boolean isFile(); + @Nullable + @Override public abstract byte[] getDigest(); - /** @return true if this is a file or a symlink to an existing file */ - abstract boolean isFile(); - - /** Gets the size of the file. Non-files (including directories) have size 0. */ + @Override public abstract long getSize(); - /** Gets last modified time of file. Should only be called if this is not a file. */ - abstract long getModifiedTime(); + @Override + public abstract long getModifiedTime(); + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof Metadata)) { + return false; + } + if ((this instanceof Singleton) || (o instanceof Singleton)) { + return false; + } + Metadata m = (Metadata) o; + if (isFile()) { + return m.isFile() && Arrays.equals(getDigest(), m.getDigest()) && getSize() == m.getSize(); + } else { + return !m.isFile() && getModifiedTime() == m.getModifiedTime(); + } + } + + @Override + public int hashCode() { + if (this instanceof Singleton) { + return System.identityHashCode(this); + } + // Hash digest by content, not reference. + if (isFile()) { + return 37 * Long.hashCode(getSize()) + Arrays.hashCode(getDigest()); + } else { + return Long.hashCode(getModifiedTime()); + } + } } 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 5a132c362a..783899ab45 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 @@ -61,9 +61,7 @@ class TreeArtifactValue implements SkyValue { Map<String, Metadata> digestBuilder = Maps.newHashMapWithExpectedSize(childFileValues.size()); for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) { - digestBuilder.put( - e.getKey().getParentRelativePath().getPathString(), - new Metadata(e.getValue().getDigest())); + digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue()); } return new TreeArtifactValue( @@ -76,7 +74,7 @@ class TreeArtifactValue implements SkyValue { } Metadata getMetadata() { - return new Metadata(digest.clone()); + return getSelfData(); } Set<PathFragment> getChildPaths() { |