Let SkyKey alone declare its value's shareability.

There are currently three methods for declaring shareability:

* `SkyFunctionName#getShareabilityOfValue`
* `SkyKey#getShareabilityOfValue`
* `SkyValue#dataIsShareable`

The former two are just "hints" - a return of `SOMETIMES` still requires checking the value. However, there is no enforcement of consistency - for example, we can have a particular `SkyFunctionName#getShareabilityOfValue` return `NEVER`, but that function may compute a value whose `SkyValue#dataIsShareable` returns `true`. This currently happens in practice.

My original plan was to check consistency in `SerializationCheckingGraph`, but it turns out that it's not too difficult to just make `SkyKey` the sole proprietor of shareability. This is strictly better than giving the responsibility to `SkyValue` because a remote storage fetch for a value need not be attempted if the key is not shareable (this is what the "hint" in `SkyKey` intended to achieve).

Replace `ShareabilityOfValue` with a simple boolean since the return of `SkyKey#valueIsShareable` is now definite.

PiperOrigin-RevId: 416937942

src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java

@@ -18,7 +18,6 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.skyframe.ExecutionPhaseSkyKey;
-import com.google.devtools.build.skyframe.ShareabilityOfValue;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 
 /** Data that uniquely identifies an action. */
@@ -44,7 +43,10 @@ public static ActionLookupData create(ActionLookupKey actionLookupKey, int actio
         : new ActionLookupData(actionLookupKey, actionIndex);
   }
 
-  /** Similar to {@link #create}, but the key will have {@link ShareabilityOfValue#NEVER}. */
+  /**
+   * Similar to {@link #create}, but the key will return {@code false} for {@link
+   * #valueIsShareable}.
+   */
   public static ActionLookupData createUnshareable(
       ActionLookupKey actionLookupKey, int actionIndex) {
     return new UnshareableActionLookupData(actionLookupKey, actionIndex);
@@ -58,21 +60,25 @@ public ActionLookupKey getActionLookupKey() {
    * Index of the action in question in the node keyed by {@link #getActionLookupKey}. Should be
    * passed to {@link ActionLookupValue#getAction}.
    */
-  public int getActionIndex() {
+  public final int getActionIndex() {
     return actionIndex;
   }
 
-  public Label getLabel() {
+  public final Label getLabel() {
     return actionLookupKey.getLabel();
   }
 
   @Override
-  public int hashCode() {
-    return 37 * actionLookupKey.hashCode() + actionIndex;
+  public final int hashCode() {
+    int hash = 1;
+    hash = 37 * hash + actionLookupKey.hashCode();
+    hash = 37 * hash + Integer.hashCode(actionIndex);
+    hash = 37 * hash + Boolean.hashCode(valueIsShareable());
+    return hash;
   }
 
   @Override
-  public boolean equals(Object obj) {
+  public final boolean equals(Object obj) {
     if (this == obj) {
       return true;
     }
@@ -81,7 +87,8 @@ public boolean equals(Object obj) {
     }
     ActionLookupData that = (ActionLookupData) obj;
     return this.actionIndex == that.actionIndex
-        && this.actionLookupKey.equals(that.actionLookupKey);
+        && this.actionLookupKey.equals(that.actionLookupKey)
+        && valueIsShareable() == that.valueIsShareable();
   }
 
   @Override
@@ -103,8 +110,8 @@ private UnshareableActionLookupData(ActionLookupKey actionLookupKey, int actionI
     }
 
     @Override
-    public ShareabilityOfValue getShareabilityOfValue() {
-      return ShareabilityOfValue.NEVER;
+    public boolean valueIsShareable() {
+      return false;
     }
   }
 }

src/main/java/com/google/devtools/build/lib/actions/Artifact.java

@@ -49,7 +49,6 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.skyframe.ShareabilityOfValue;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.protobuf.CodedInputStream;
@@ -1117,8 +1116,8 @@ public PathFragment getParentRelativePath() {
     }
 
     @Override
-    public ShareabilityOfValue getShareabilityOfValue() {
-      return isConstantMetadata() ? ShareabilityOfValue.NEVER : super.getShareabilityOfValue();
+    public boolean valueIsShareable() {
+      return !isConstantMetadata();
     }
   }
 

src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

@@ -201,48 +201,34 @@ public static FileArtifactValue createForSourceArtifact(Artifact artifact, FileV
         isFile,
         isFile ? fileValue.getSize() : 0,
         isFile ? fileValue.realFileStateValue().getContentsProxy() : null,
-        isFile ? fileValue.getDigest() : null,
-        /* isShareable=*/ true);
+        isFile ? fileValue.getDigest() : null);
   }
 
   public static FileArtifactValue createFromInjectedDigest(
-      FileArtifactValue metadata, @Nullable byte[] digest, boolean isShareable) {
-    return createForNormalFile(
-        digest, metadata.getContentsProxy(), metadata.getSize(), isShareable);
+      FileArtifactValue metadata, @Nullable byte[] digest) {
+    return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
   }
 
   @VisibleForTesting
   public static FileArtifactValue createForTesting(Artifact artifact) throws IOException {
-    Path path = artifact.getPath();
-    boolean isShareable = !artifact.isConstantMetadata();
-    // Caution: there's a race condition between stating the file and computing the
-    // digest. We need to stat first, since we're using the stat to detect changes.
-    // We follow symlinks here to be consistent with getDigest.
-    return createFromStat(path, path.stat(Symlinks.FOLLOW), isShareable);
+    return createForTesting(artifact.getPath());
   }
 
   @VisibleForTesting
   public static FileArtifactValue createForTesting(Path path) throws IOException {
-    /*isShareable=*/
-    // Caution: there's a race condition between stating the file and computing the
-    // digest. We need to stat first, since we're using the stat to detect changes.
-    // We follow symlinks here to be consistent with getDigest.
-    return createFromStat(path, path.stat(Symlinks.FOLLOW), true);
+    // Caution: there's a race condition between stating the file and computing the digest. We need
+    // to stat first, since we're using the stat to detect changes. We follow symlinks here to be
+    // consistent with getDigest.
+    return createFromStat(path, path.stat(Symlinks.FOLLOW));
   }
 
-  public static FileArtifactValue createFromStat(Path path, FileStatus stat, boolean isShareable)
-      throws IOException {
+  public static FileArtifactValue createFromStat(Path path, FileStatus stat) throws IOException {
     return create(
-        path, stat.isFile(), stat.getSize(), FileContentsProxy.create(stat), null, isShareable);
+        path, stat.isFile(), stat.getSize(), FileContentsProxy.create(stat), /*digest=*/ null);
   }
 
   private static FileArtifactValue create(
-      Path path,
-      boolean isFile,
-      long size,
-      FileContentsProxy proxy,
-      @Nullable byte[] digest,
-      boolean isShareable)
+      Path path, boolean isFile, long size, FileContentsProxy proxy, @Nullable byte[] digest)
       throws IOException {
     if (!isFile) {
       // In this case, we need to store the mtime because the action cache uses mtime for
@@ -254,7 +240,7 @@ private static FileArtifactValue create(
       digest = DigestUtils.getDigestWithManualFallback(path, size);
     }
     Preconditions.checkState(digest != null, path);
-    return createForNormalFile(digest, proxy, size, isShareable);
+    return createForNormalFile(digest, proxy, size);
   }
 
   public static FileArtifactValue createForVirtualActionInput(byte[] digest, long size) {
@@ -281,10 +267,8 @@ public static FileArtifactValue createForUnresolvedSymlink(Path symlink) throws
 
   @VisibleForTesting
   public static FileArtifactValue createForNormalFile(
-      byte[] digest, @Nullable FileContentsProxy proxy, long size, boolean isShareable) {
-    return isShareable
-        ? new RegularFileArtifactValue(digest, proxy, size)
-        : new UnshareableRegularFileArtifactValue(digest, proxy, size);
+      byte[] digest, @Nullable FileContentsProxy proxy, long size) {
+    return new RegularFileArtifactValue(digest, proxy, size);
   }
 
   /**
@@ -293,7 +277,7 @@ public static FileArtifactValue createForNormalFile(
    */
   public static FileArtifactValue createForNormalFileUsingPath(Path path, long size)
       throws IOException {
-    return create(path, true, size, null, null, true);
+    return create(path, /*isFile=*/ true, size, /*proxy=*/ null, /*digest=*/ null);
   }
 
   public static FileArtifactValue createForDirectoryWithHash(byte[] digest) {
@@ -310,7 +294,7 @@ public static FileArtifactValue createForDirectoryWithMtime(long mtime) {
    */
   public static FileArtifactValue createProxy(byte[] digest) {
     Preconditions.checkNotNull(digest);
-    return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0, /*isShareable=*/ true);
+    return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0);
   }
 
   private static String bytesToString(byte[] bytes) {
@@ -447,8 +431,7 @@ public String toString() {
     }
   }
 
-  private static class RegularFileArtifactValue extends FileArtifactValue {
-
+  private static final class RegularFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
     @Nullable private final FileContentsProxy proxy;
     private final long size;
@@ -462,20 +445,21 @@ private RegularFileArtifactValue(
 
     @Override
     public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
       if (!(o instanceof RegularFileArtifactValue)) {
         return false;
       }
-
       RegularFileArtifactValue that = (RegularFileArtifactValue) o;
       return Arrays.equals(digest, that.digest)
           && Objects.equals(proxy, that.proxy)
-          && size == that.size
-          && dataIsShareable() == that.dataIsShareable();
+          && size == that.size;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(Arrays.hashCode(digest), proxy, size, dataIsShareable());
+      return Objects.hash(Arrays.hashCode(digest), proxy, size);
     }
 
     @Override
@@ -535,18 +519,6 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue o) {
     }
   }
 
-  private static final class UnshareableRegularFileArtifactValue extends RegularFileArtifactValue {
-    private UnshareableRegularFileArtifactValue(
-        byte[] digest, @Nullable FileContentsProxy proxy, long size) {
-      super(digest, proxy, size);
-    }
-
-    @Override
-    public boolean dataIsShareable() {
-      return false;
-    }
-  }
-
   /** Metadata for remotely stored files. */
   public static class RemoteFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
@@ -575,14 +547,12 @@ public boolean equals(Object o) {
       return Arrays.equals(digest, that.digest)
           && size == that.size
           && locationIndex == that.locationIndex
-          && Objects.equals(actionId, that.actionId)
-          && dataIsShareable() == that.dataIsShareable();
+          && Objects.equals(actionId, that.actionId);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(
-          Arrays.hashCode(digest), size, locationIndex, actionId, dataIsShareable());
+      return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId);
     }
 
     @Override
@@ -682,7 +652,12 @@ public boolean wasModifiedSinceDigest(Path path) {
   }
 
   /** File stored inline in metadata. */
-  public static class InlineFileArtifactValue extends FileArtifactValue {
+  public static final class InlineFileArtifactValue extends FileArtifactValue {
+
+    public static InlineFileArtifactValue create(byte[] bytes, HashFunction hashFunction) {
+      return new InlineFileArtifactValue(bytes, hashFunction.hashBytes(bytes).asBytes());
+    }
+
     private final byte[] data;
     private final byte[] digest;
 
@@ -691,30 +666,21 @@ private InlineFileArtifactValue(byte[] data, byte[] digest) {
       this.digest = Preconditions.checkNotNull(digest);
     }
 
-    private InlineFileArtifactValue(byte[] bytes, HashFunction hashFunction) {
-      this(bytes, hashFunction.hashBytes(bytes).asBytes());
-    }
-
     @Override
     public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
       if (!(o instanceof InlineFileArtifactValue)) {
         return false;
       }
-
       InlineFileArtifactValue that = (InlineFileArtifactValue) o;
-      return Arrays.equals(digest, that.digest) && dataIsShareable() == that.dataIsShareable();
+      return Arrays.equals(digest, that.digest);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(Arrays.hashCode(digest), dataIsShareable());
-    }
-
-    public static InlineFileArtifactValue create(
-        byte[] bytes, boolean shareable, HashFunction hashFunction) {
-      return shareable
-          ? new InlineFileArtifactValue(bytes, hashFunction)
-          : new UnshareableInlineFileArtifactValue(bytes, hashFunction);
+      return Arrays.hashCode(digest);
     }
 
     public ByteArrayInputStream getInputStream() {
@@ -752,17 +718,6 @@ public boolean wasModifiedSinceDigest(Path path) {
     }
   }
 
-  private static final class UnshareableInlineFileArtifactValue extends InlineFileArtifactValue {
-    UnshareableInlineFileArtifactValue(byte[] bytes, HashFunction hashFunction) {
-      super(bytes, hashFunction);
-    }
-
-    @Override
-    public boolean dataIsShareable() {
-      return false;
-    }
-  }
-
   /**
    * Used to resolve source symlinks when diskless.
    *

src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java

@@ -60,7 +60,7 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException {
               Path path = ActionInputHelper.toInputPath(input, execRoot);
               FileArtifactValue metadata;
               try {
-                metadata = FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW), true);
+                metadata = FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW));
               } catch (IOException e) {
                 return new ActionInputMetadata(input, e);
               }

src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java

@@ -46,18 +46,12 @@ public boolean equals(Object o) {
         && getSize() == that.getSize()
         && getLocationIndex() == that.getLocationIndex()
         && Objects.equals(getActionId(), that.getActionId())
-        && isExecutable == that.isExecutable
-        && dataIsShareable() == that.dataIsShareable();
+        && isExecutable == that.isExecutable;
   }
 
   @Override
   public int hashCode() {
     return Objects.hash(
-        Arrays.hashCode(getDigest()),
-        getSize(),
-        getLocationIndex(),
-        getActionId(),
-        isExecutable,
-        dataIsShareable());
+        Arrays.hashCode(getDigest()), getSize(), getLocationIndex(), getActionId(), isExecutable);
   }
 }

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -382,7 +382,7 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env)
 
     // Remove action from state map in case it's there (won't be unless it discovers inputs).
     stateMap.remove(action);
-    if (sketch != null && result.dataIsShareable()) {
+    if (sketch != null && actionLookupData.valueIsShareable()) {
       topDownActionCache.put(sketch, result);
     }
     return result;
@@ -819,7 +819,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
               + "SkyframeAwareAction which should be re-executed unconditionally. Action: %s",
           action);
       return ActionExecutionValue.createFromOutputStore(
-          metadataHandler.getOutputStore(), /*outputSymlinks=*/ null, action, actionLookupData);
+          metadataHandler.getOutputStore(), /*outputSymlinks=*/ null, action);
     }
 
     metadataHandler.prepareForActionExecution();