Make Metadata an interface for FileArtifactValue

Replace all previous uses of Metadata with FileArtifactValue (or a simple inner
class in the case of ActionCacheChecker.CONSTANT_METADATA).

Care was taken to make the equals method obey the equals contract, even in the
presence of multiple implementations.

PiperOrigin-RevId: 160115080

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() {