Generalize the concept of a FileArtifactValue with a resolved path.

The presence of a resolved path is now orthogonal to the remainder of the metadata, i.e., any existing metadata may be wrapped with a resolved path. The added flexibility will be useful in upcoming changes extending the concept of lazy materialization to non-remote artifacts.

This CL doesn't change preexisting behavior; it's just an API refactoring.

PiperOrigin-RevId: 722116113
Change-Id: I85234bf7363ea3361ff756bd65015010fff618a9

src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java

@@ -117,19 +117,18 @@ final class Entry {
     public record SerializableTreeArtifactValue(
         ImmutableMap<String, FileArtifactValue> childValues,
         Optional<FileArtifactValue> archivedFileValue,
-        Optional<PathFragment> materializationExecPath) {
+        Optional<PathFragment> resolvedPath) {
       public SerializableTreeArtifactValue {
         requireNonNull(childValues, "childValues");
         requireNonNull(archivedFileValue, "archivedFileValue");
-        requireNonNull(materializationExecPath, "materializationExecPath");
+        requireNonNull(resolvedPath, "resolvedPath");
       }
 
       public static SerializableTreeArtifactValue create(
           ImmutableMap<String, FileArtifactValue> childValues,
           Optional<FileArtifactValue> archivedFileValue,
-          Optional<PathFragment> materializationExecPath) {
-        return new SerializableTreeArtifactValue(
-            childValues, archivedFileValue, materializationExecPath);
+          Optional<PathFragment> resolvedPath) {
+        return new SerializableTreeArtifactValue(childValues, archivedFileValue, resolvedPath);
       }
 
       /**
@@ -154,17 +153,14 @@ public static Optional<SerializableTreeArtifactValue> createSerializable(
                 .filter(ar -> ar.archivedFileValue().isRemote())
                 .map(ar -> ar.archivedFileValue());
 
-        Optional<PathFragment> materializationExecPath = treeMetadata.getMaterializationExecPath();
+        Optional<PathFragment> resolvedPath = treeMetadata.getResolvedPath();
 
-        if (childValues.isEmpty()
-            && archivedFileValue.isEmpty()
-            && materializationExecPath.isEmpty()) {
+        if (childValues.isEmpty() && archivedFileValue.isEmpty() && resolvedPath.isEmpty()) {
           return Optional.empty();
         }
 
         return Optional.of(
-            SerializableTreeArtifactValue.create(
-                childValues, archivedFileValue, materializationExecPath));
+            SerializableTreeArtifactValue.create(childValues, archivedFileValue, resolvedPath));
       }
     }
 

src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java

@@ -77,7 +77,7 @@ public class CompactPersistentActionCache implements ActionCache {
 
   private static final int NO_INPUT_DISCOVERY_COUNT = -1;
 
-  private static final int VERSION = 17;
+  private static final int VERSION = 18;
 
   private static final class ActionMap extends PersistentMap<Integer, byte[]> {
     private final Clock clock;
@@ -357,9 +357,10 @@ public ActionCache.Entry get(String key) {
     return get(data);
   }
 
+  @Nullable
   private ActionCache.Entry get(byte[] data) {
     try {
-      return data != null ? decode(indexer, data) : null;
+      return data != null ? decode(data) : null;
     } catch (IOException e) {
       // return entry marked as corrupted.
       return ActionCache.Entry.CORRUPTED;
@@ -372,7 +373,7 @@ public void put(String key, ActionCache.Entry entry) {
     Integer index = indexer.getOrCreateIndex(key);
     byte[] content;
     try {
-      content = encode(indexer, entry);
+      content = encode(entry);
     } catch (IOException e) {
       logger.atWarning().withCause(e).log("Failed to save cache entry %s with key %s", entry, key);
       return;
@@ -441,7 +442,7 @@ public String toString() {
       }
       String content;
       try {
-        content = decode(indexer, entry.getValue()).toString();
+        content = decode(entry.getValue()).toString();
       } catch (IOException e) {
         content = e + "\n";
       }
@@ -473,7 +474,7 @@ public void dump(PrintStream out) {
       }
       String content;
       try {
-        content = decode(indexer, entry.getValue()).toString();
+        content = decode(entry.getValue()).toString();
       } catch (IOException e) {
         content = e + "\n";
       }
@@ -498,8 +499,7 @@ public int size() {
     return map.size();
   }
 
-  private static void encodeRemoteMetadata(
-      FileArtifactValue value, StringIndexer indexer, ByteArrayOutputStream sink)
+  private void encodeRemoteMetadata(FileArtifactValue value, ByteArrayOutputStream sink)
       throws IOException {
     checkArgument(value.isRemote(), "metadata is not remote: %s", value);
 
@@ -512,10 +512,10 @@ private static void encodeRemoteMetadata(
     VarInt.putVarLong(
         value.getExpirationTime() != null ? value.getExpirationTime().toEpochMilli() : -1, sink);
 
-    Optional<PathFragment> materializationExecPath = value.getMaterializationExecPath();
-    if (materializationExecPath.isPresent()) {
+    PathFragment resolvedPath = value.getResolvedPath();
+    if (resolvedPath != null) {
       VarInt.putVarInt(1, sink);
-      VarInt.putVarInt(indexer.getOrCreateIndex(materializationExecPath.get().toString()), sink);
+      VarInt.putVarInt(indexer.getOrCreateIndex(resolvedPath.toString()), sink);
     } else {
       VarInt.putVarInt(0, sink);
     }
@@ -526,10 +526,9 @@ private static void encodeRemoteMetadata(
           + VarInt.MAX_VARLONG_SIZE // size
           + VarInt.MAX_VARINT_SIZE // locationIndex
           + VarInt.MAX_VARINT_SIZE // expireAtEpochMilli
-          + VarInt.MAX_VARINT_SIZE; // materializationExecPath
+          + VarInt.MAX_VARINT_SIZE; // resolvedPath
 
-  private static FileArtifactValue decodeRemoteMetadata(StringIndexer indexer, ByteBuffer source)
-      throws IOException {
+  private FileArtifactValue decodeRemoteMetadata(ByteBuffer source) throws IOException {
     byte[] digest = MetadataDigestUtils.read(source);
 
     long size = VarInt.getVarLong(source);
@@ -538,32 +537,35 @@ private static FileArtifactValue decodeRemoteMetadata(StringIndexer indexer, Byt
 
     long expirationTimeEpochMilli = VarInt.getVarLong(source);
 
-    PathFragment materializationExecPath = null;
-    int numMaterializationExecPath = VarInt.getVarInt(source);
-    if (numMaterializationExecPath > 0) {
-      if (numMaterializationExecPath != 1) {
-        throw new IOException("Invalid presence marker for materialization path");
+    PathFragment resolvedPath = null;
+    int numResolvedPath = VarInt.getVarInt(source);
+    if (numResolvedPath > 0) {
+      if (numResolvedPath != 1) {
+        throw new IOException("Invalid presence marker for resolved path");
       }
-      materializationExecPath =
-          PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
+      resolvedPath = PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
+    }
+
+    FileArtifactValue metadata;
+    if (expirationTimeEpochMilli < 0) {
+      metadata = FileArtifactValue.createForRemoteFile(digest, size, locationIndex);
+    } else {
+      metadata =
+          FileArtifactValue.createForRemoteFileWithMaterializationData(
+              digest, size, locationIndex, Instant.ofEpochMilli(expirationTimeEpochMilli));
     }
 
-    if (expirationTimeEpochMilli < 0 && materializationExecPath == null) {
-      return FileArtifactValue.createForRemoteFile(digest, size, locationIndex);
+    if (resolvedPath != null) {
+      metadata = FileArtifactValue.createFromExistingWithResolvedPath(metadata, resolvedPath);
     }
 
-    return FileArtifactValue.createForRemoteFileWithMaterializationData(
-        digest,
-        size,
-        locationIndex,
-        Instant.ofEpochMilli(expirationTimeEpochMilli),
-        materializationExecPath);
+    return metadata;
   }
 
   /**
    * @return action data encoded as a byte[] array.
    */
-  private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) throws IOException {
+  private byte[] encode(ActionCache.Entry entry) throws IOException {
     Preconditions.checkState(!entry.isCorrupted());
 
     byte[] actionKeyBytes = entry.getActionKey().getBytes(ISO_8859_1);
@@ -592,8 +594,8 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr
           (1 + VarInt.MAX_VARINT_SIZE) // value.archivedFileValue() optional
               + value.archivedFileValue().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0);
       maxOutputTreesSize +=
-          (1 + VarInt.MAX_VARINT_SIZE) // value.materializationExecPath() optional
-              + value.materializationExecPath().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0);
+          (1 + VarInt.MAX_VARINT_SIZE) // value.resolvedPath() optional
+              + value.resolvedPath().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0);
     }
 
     // Estimate the size of the buffer:
@@ -631,7 +633,7 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr
     VarInt.putVarInt(entry.getOutputFiles().size(), sink);
     for (Map.Entry<String, FileArtifactValue> file : entry.getOutputFiles().entrySet()) {
       VarInt.putVarInt(indexer.getOrCreateIndex(file.getKey()), sink);
-      encodeRemoteMetadata(file.getValue(), indexer, sink);
+      encodeRemoteMetadata(file.getValue(), sink);
     }
 
     VarInt.putVarInt(entry.getOutputTrees().size(), sink);
@@ -645,23 +647,22 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr
       for (Map.Entry<String, FileArtifactValue> child :
           serializableTreeArtifactValue.childValues().entrySet()) {
         VarInt.putVarInt(indexer.getOrCreateIndex(child.getKey()), sink);
-        encodeRemoteMetadata(child.getValue(), indexer, sink);
+        encodeRemoteMetadata(child.getValue(), sink);
       }
 
       Optional<FileArtifactValue> archivedFileValue =
           serializableTreeArtifactValue.archivedFileValue();
       if (archivedFileValue.isPresent()) {
         VarInt.putVarInt(1, sink);
-        encodeRemoteMetadata(archivedFileValue.get(), indexer, sink);
+        encodeRemoteMetadata(archivedFileValue.get(), sink);
       } else {
         VarInt.putVarInt(0, sink);
       }
 
-      Optional<PathFragment> materializationExecPath =
-          serializableTreeArtifactValue.materializationExecPath();
-      if (materializationExecPath.isPresent()) {
+      Optional<PathFragment> resolvedPath = serializableTreeArtifactValue.resolvedPath();
+      if (resolvedPath.isPresent()) {
         VarInt.putVarInt(1, sink);
-        VarInt.putVarInt(indexer.getOrCreateIndex(materializationExecPath.get().toString()), sink);
+        VarInt.putVarInt(indexer.getOrCreateIndex(resolvedPath.get().toString()), sink);
       } else {
         VarInt.putVarInt(0, sink);
       }
@@ -682,7 +683,7 @@ private static String getStringForIndex(StringIndexer indexer, int index) throws
    * Creates new action cache entry using given compressed entry data. Data will stay in the
    * compressed format until entry is actually used by the dependency checker.
    */
-  private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) throws IOException {
+  private ActionCache.Entry decode(byte[] data) throws IOException {
     try {
       ByteBuffer source = ByteBuffer.wrap(data);
 
@@ -713,7 +714,7 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro
       Map<String, FileArtifactValue> outputFiles = Maps.newHashMapWithExpectedSize(numOutputFiles);
       for (int i = 0; i < numOutputFiles; i++) {
         String execPath = getStringForIndex(indexer, VarInt.getVarInt(source));
-        FileArtifactValue value = decodeRemoteMetadata(indexer, source);
+        FileArtifactValue value = decodeRemoteMetadata(source);
         outputFiles.put(execPath, value);
       }
 
@@ -727,7 +728,7 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro
         int numChildValues = VarInt.getVarInt(source);
         for (int j = 0; j < numChildValues; ++j) {
           String childKey = getStringForIndex(indexer, VarInt.getVarInt(source));
-          FileArtifactValue value = decodeRemoteMetadata(indexer, source);
+          FileArtifactValue value = decodeRemoteMetadata(source);
           childValues.put(childKey, value);
         }
 
@@ -737,23 +738,23 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro
           if (numArchivedFileValue != 1) {
             throw new IOException("Invalid presence marker for archived representation");
           }
-          archivedFileValue = Optional.of(decodeRemoteMetadata(indexer, source));
+          archivedFileValue = Optional.of(decodeRemoteMetadata(source));
         }
 
-        Optional<PathFragment> materializationExecPath = Optional.empty();
-        int numMaterializationExecPath = VarInt.getVarInt(source);
-        if (numMaterializationExecPath > 0) {
-          if (numMaterializationExecPath != 1) {
-            throw new IOException("Invalid presence marker for materialization path");
+        Optional<PathFragment> resolvedPath = Optional.empty();
+        int numResolvedPath = VarInt.getVarInt(source);
+        if (numResolvedPath > 0) {
+          if (numResolvedPath != 1) {
+            throw new IOException("Invalid presence marker for resolved path");
           }
-          materializationExecPath =
+          resolvedPath =
               Optional.of(
                   PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))));
         }
 
         SerializableTreeArtifactValue value =
             SerializableTreeArtifactValue.create(
-                childValues.buildOrThrow(), archivedFileValue, materializationExecPath);
+                childValues.buildOrThrow(), archivedFileValue, resolvedPath);
         outputTrees.put(treeKey, value);
       }
 

src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java

@@ -30,7 +30,6 @@
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.ArtifactExpander;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.FileArtifactValue.SymlinkToSourceFileArtifactValue;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -368,7 +367,8 @@ public static void maybeInjectMetadata(Action symlinkAction, ActionExecutionCont
           .injectFile(
               symlinkAction.getPrimaryOutput(),
               primaryInput instanceof SourceArtifact sourceArtifact
-                  ? SymlinkToSourceFileArtifactValue.toSourceArtifact(sourceArtifact, metadata)
+                  ? FileArtifactValue.createFromExistingWithResolvedPath(
+                      metadata, primaryInput.getPath().asFragment())
                   : metadata);
     }
   }

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -420,7 +420,7 @@ private ListenableFuture<Void> prefetchFile(
    *
    * <p>Some artifacts (notably, those created by {@code ctx.actions.symlink}) are materialized in
    * the output tree as a symlink to another artifact, as indicated by the {@link
-   * FileArtifactValue#getMaterializationExecPath()} field in their metadata.
+   * FileArtifactValue#getResolvedPath()} field in their metadata.
    */
   @Nullable
   private PathFragment maybeGetTreeRoot(
@@ -441,7 +441,11 @@ private PathFragment maybeGetTreeRoot(
       throw new IllegalStateException(
           String.format("input %s belongs to a tree artifact whose metadata is missing", treeFile));
     }
-    return treeMetadata.getMaterializationExecPath().orElse(treeArtifact.getExecPath());
+    PathFragment resolvedPath = treeMetadata.getResolvedPath();
+    if (resolvedPath != null) {
+      return resolvedPath.relativeTo(execRoot.asFragment());
+    }
+    return treeArtifact.getExecPath();
   }
 
   /**
@@ -450,8 +454,7 @@ private PathFragment maybeGetTreeRoot(
    *
    * <p>Some artifacts (notably, those created by {@code ctx.actions.symlink}) are materialized in
    * the output tree as a symlink to another artifact, as indicated by the {@link
-   * FileArtifactValue#getMaterializationExecPath()} field in their (or their parent tree
-   * artifact's) metadata.
+   * FileArtifactValue#getResolvedPath()} field in their (or their parent tree artifact's) metadata.
    */
   @Nullable
   private Symlink maybeGetSymlink(
@@ -477,9 +480,12 @@ private Symlink maybeGetSymlink(
       return maybeGetSymlink(action, treeArtifact, treeMetadata, metadataSupplier);
     }
     PathFragment execPath = input.getExecPath();
-    PathFragment materializationExecPath = metadata.getMaterializationExecPath().orElse(execPath);
-    if (!materializationExecPath.equals(execPath)) {
-      return Symlink.of(execPath, materializationExecPath);
+    PathFragment resolvedExecPath = execPath;
+    if (metadata.getResolvedPath() != null) {
+      resolvedExecPath = metadata.getResolvedPath().relativeTo(execRoot.asFragment());
+    }
+    if (!resolvedExecPath.equals(execPath)) {
+      return Symlink.of(execPath, resolvedExecPath);
     }
     return null;
   }
@@ -526,7 +532,7 @@ private Completable downloadFileNoCheckRx(
     // If the path to be prefetched is a non-dangling symlink, prefetch its target path instead.
     // Note that this only applies to symlinks created by spawns (or, currently, with the internal
     // version of BwoB); symlinks created in-process through an ActionFileSystem should have already
-    // been resolved into their materializationExecPath in maybeGetSymlink.
+    // been canonicalized by maybeGetSymlink.
     try {
       if (treeRoot != null) {
         var treeRootRelativePath = path.relativeTo(treeRoot);

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -306,11 +306,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, Instant expir
     }
     var metadata =
         FileArtifactValue.createForRemoteFileWithMaterializationData(
-            digest,
-            size,
-            /* locationIndex= */ 1,
-            expirationTime,
-            /* materializationExecPath= */ null);
+            digest, size, /* locationIndex= */ 1, expirationTime);
     remoteOutputTree.injectFile(path, metadata);
   }
 

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

@@ -818,8 +818,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
             ImmutableSet.copyOf(action.getOutputs()),
             skyframeActionExecutor.getXattrProvider(),
             tsgm.get(),
-            pathResolver,
-            skyframeActionExecutor.getExecRoot().asFragment());
+            pathResolver);
 
     // We only need to check the action cache if we haven't done it on a previous run.
     if (!state.hasCheckedActionCache()) {

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

@@ -109,7 +109,7 @@ static void addToMap(
       // Actions resulting from the expansion of an ActionTemplate consume only one of the files
       // in a tree artifact. However, the input prefetcher and the Linux sandbox require access to
       // the tree metadata to determine the prefetch location of a tree artifact materialized as a
-      // symlink (cf. TreeArtifactValue#getMaterializationExecPath()).
+      // symlink to another (cf. TreeArtifactValue#getResolvedPath()).
       if (key.isChildOfDeclaredDirectory()) {
         SpecialArtifact treeArtifact = key.getParent();
         TreeArtifactValue treeArtifactValue =