Use a visitor pattern to consume fileset symlinks.

Instead of constructing a `FilesetManifest`, callers use a `Visitor` that is invoked for every symlink in the fileset. This allows for an optimization: when the fileset has no relative symlinks (or we are ignoring relative symlinks), we can do a single-pass visitation without building up maps. Additionally, callers are responsible for concatenating the fileset-relative and execroot-relative paths to their roots. This reduces garbage from concatenating paths, since some callers don't need them. PiperOrigin-RevId: 683222046 Change-Id: I80f04306e7a617d3c3e630c56d96b2c968eb1d42
bazelbuild · Oct 7, 2024 · dfe0aee · dfe0aee
1 parent ae02744
commit dfe0aee
Show file tree

Hide file tree

Showing 9 changed files with 263 additions and 350 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD
@@ -407,6 +407,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
         "//third_party:flogger",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 

diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java
@@ -23,7 +23,6 @@
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.bugreport.BugReporter;
@@ -175,19 +174,12 @@ public void visitArtifacts(Iterable<Artifact> artifacts, ArtifactReceiver receiv
 
   private void visitFileset(Artifact filesetArtifact, ArtifactReceiver receiver) {
     FilesetOutputTree filesetOutput = filesets.get(filesetArtifact);
-    FilesetManifest filesetManifest =
-        filesetOutput.constructFilesetManifestWithoutError(
-            PathFragment.EMPTY_FRAGMENT,
-            fullyResolveFilesetLinks
-                ? RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY
-                : RelativeSymlinkBehaviorWithoutError.RESOLVE);
-
-    for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) {
-      String targetFile = mapping.getValue();
-      PathFragment locationInFileset = mapping.getKey();
-      receiver.acceptFilesetMapping(
-          filesetArtifact, locationInFileset, execRoot.getRelative(targetFile));
-    }
+    filesetOutput.visitSymlinks(
+        fullyResolveFilesetLinks
+            ? RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY
+            : RelativeSymlinkBehaviorWithoutError.RESOLVE,
+        (name, target, metadata) ->
+            receiver.acceptFilesetMapping(filesetArtifact, name, execRoot.getRelative(target)));
   }
 
   @Override

diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java
@@ -38,7 +38,6 @@
 import com.google.devtools.build.lib.actions.CommandLines;
 import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
 import com.google.devtools.build.lib.actions.FilesetOutputTree;
-import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.PathMapper;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
@@ -498,15 +497,15 @@ private static void expandFileset(
                 fileset),
             e);
       }
-      FilesetManifest filesetManifest =
-          filesetOutput.constructFilesetManifestWithoutError(
-              fileset.getExecPath(), RelativeSymlinkBehaviorWithoutError.IGNORE);
-      for (PathFragment relativePath : filesetManifest.getEntries().keySet()) {
-        PathFragment mappedRelativePath = pathMapper.map(relativePath);
-        expandedValues.add(new FilesetSymlinkFile(fileset, mappedRelativePath));
-      }
+      PathFragment mappedExecPath = pathMapper.map(fileset.getExecPath());
+      filesetOutput.visitSymlinks(
+          RelativeSymlinkBehaviorWithoutError.IGNORE,
+          (name, target, metadata) ->
+              expandedValues.add(
+                  new FilesetSymlinkFile(fileset, mappedExecPath.getRelative(name))));
     }
 
+
     private int addToFingerprint(
         List<Object> arguments,
         int argi,

diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
@@ -28,7 +28,6 @@
 import com.google.devtools.build.lib.actions.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ArtifactExpander.MissingExpansionException;
 import com.google.devtools.build.lib.actions.FilesetOutputTree;
-import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetOutputTree.ForbiddenRelativeSymlinkException;
 import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior;
 import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
@@ -188,15 +187,14 @@ private void addFilesetManifest(
       PathFragment baseDirectory)
       throws ForbiddenRelativeSymlinkException {
     Preconditions.checkArgument(filesetArtifact.isFileset(), filesetArtifact);
-    FilesetManifest filesetManifest =
-        filesetOutput.constructFilesetManifest(location, relSymlinkBehavior);
-
-    for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) {
-      String value = mapping.getValue();
-      ActionInput artifact = ActionInputHelper.fromPath(execRoot.getRelative(value).asFragment());
-      // TODO(bazel-team): Add path mapping support for filesets.
-      addMapping(inputMap, mapping.getKey(), artifact, baseDirectory);
-    }
+    filesetOutput.visitSymlinks(
+        relSymlinkBehavior,
+        (name, target, metadata) ->
+            addMapping(
+                inputMap,
+                location.getRelative(name),
+                ActionInputHelper.fromPath(execRoot.getRelative(target).asFragment()),
+                baseDirectory));
   }
 
   private void addInputs(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java
@@ -24,7 +24,6 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FilesetOutputTree;
-import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.RunfilesArtifactValue;
@@ -64,24 +63,20 @@ final class ActionInputMetadataProvider implements InputMetadataProvider {
       Map<Artifact, FilesetOutputTree> filesets) {
     this.execRoot = execRoot;
     this.inputArtifactData = inputArtifactData;
-    this.filesetMapping = Suppliers.memoize(() -> createFilesetMapping(filesets, execRoot));
+    this.filesetMapping = Suppliers.memoize(() -> createFilesetMapping(filesets));
   }
 
   private static ImmutableMap<String, FileArtifactValue> createFilesetMapping(
-      Map<Artifact, FilesetOutputTree> filesets, PathFragment execRoot) {
+      Map<Artifact, FilesetOutputTree> filesets) {
     Map<String, FileArtifactValue> filesetMap = new HashMap<>();
     for (FilesetOutputTree filesetOutput : filesets.values()) {
-      FilesetManifest manifest =
-          filesetOutput.constructFilesetManifestWithoutError(
-              execRoot, RelativeSymlinkBehaviorWithoutError.RESOLVE);
-      manifest
-          .getArtifactValues()
-          .forEach(
-              (targetPath, metadata) -> {
-                if (metadata.getDigest() != null) {
-                  filesetMap.put(targetPath, metadata);
-                }
-              });
+      filesetOutput.visitSymlinks(
+          RelativeSymlinkBehaviorWithoutError.RESOLVE,
+          (name, target, metadata) -> {
+            if (metadata != null && metadata.getDigest() != null) {
+              filesetMap.put(target.getPathString(), metadata);
+            }
+          });
     }
     return ImmutableMap.copyOf(filesetMap);
   }
@@ -96,9 +91,7 @@ public FileArtifactValue getInputMetadata(ActionInput actionInput) throws IOExce
       return filesetMapping.get().get(filesetKeyPath.getPathString());
     }
 
-    FileArtifactValue value;
-
-    value = inputArtifactData.getInputMetadata(artifact);
+    FileArtifactValue value = inputArtifactData.getInputMetadata(artifact);
     if (value != null) {
       return checkExists(value, artifact);
     }