From addea4722537874036929803fb4f54e155ff0be2 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 8 Jan 2024 07:26:18 -0800 Subject: [PATCH] Always add containing trees of input tree file artifacts to the metadata map. This happens for action templates. The reason is that the Linux sandbox needs to know where the tree file artifact is materialized to, which information is stored in its parent. The reason for making this a flag was performance, but it's only at most one tree artifact per action, it's a constant time operation per tree artifact and it only happens rarely (on templated actions) so I think this time, simplicity is better than performance. The surprising change to ActionInputMapHelper was needed because before this change, the tree artifact was a discovered input and therefore was processed by the code path in ActionExecutionFunction.addDiscoveredInputs(), which populated archivedTreeArtifacts but addToMap() did not. It was presumably the bug. The depOwner argument of putTreeArtifact may also be wrong there. It's at least inconsistent with the other two call sites, but I don't want to add more polish at some risk to this change so I decided to not add it to addArchivedTreeArtifactMaybe(). Progress towards #20753. RELNOTES: None. PiperOrigin-RevId: 596586625 Change-Id: Ib690a84508da07560ec376d4e87ed8fb2211979f --- .../build/lib/actions/ActionInputHelper.java | 17 +++++- .../devtools/build/lib/actions/Artifact.java | 12 ++-- .../lib/skyframe/ActionExecutionFunction.java | 3 +- .../lib/skyframe/ActionInputMapHelper.java | 55 +++++++++++-------- .../lib/skyframe/CompletionFunction.java | 6 +- .../lib/skyframe/SkyframeActionExecutor.java | 4 -- 6 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index 9c57f73edf3ade..5bcb26aad5d2f3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -21,7 +21,10 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.TreeSet; /** Helper utility to create ActionInput instances. */ public final class ActionInputHelper { @@ -122,14 +125,24 @@ public static List expandArtifacts( ArtifactExpander artifactExpander, boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); + Set emptyTreeArtifacts = new TreeSet<>(); + Set treeFileArtifactParents = new HashSet<>(); for (ActionInput input : inputs.toList()) { if (input instanceof Artifact) { - Artifact.addExpandedArtifact( - (Artifact) input, result, artifactExpander, keepEmptyTreeArtifacts); + Artifact inputArtifact = (Artifact) input; + Artifact.addExpandedArtifact(inputArtifact, result, artifactExpander, emptyTreeArtifacts); + if (inputArtifact.isChildOfDeclaredDirectory()) { + treeFileArtifactParents.add(inputArtifact.getParent()); + } } else { result.add(input); } } + + if (keepEmptyTreeArtifacts) { + emptyTreeArtifacts.removeAll(treeFileArtifactParents); + result.addAll(emptyTreeArtifacts); + } return result; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 1775144cfd400a..1a0b1ba55483cb 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -59,6 +59,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.UnaryOperator; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable /** * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact. * - *

A middleman artifact is never added to the collection. If {@code keepEmptyTreeArtifacts} is - * true, a tree artifact will be added to the collection when it expands into zero file artifacts. - * Otherwise, only the file artifacts the tree artifact expands into will be added. + *

The middleman or tree artifact is never added to the output collection. If a tree artifact + * expands into zero file artifacts, it is added to emptyTreeArtifacts. */ static void addExpandedArtifact( Artifact artifact, Collection output, ArtifactExpander artifactExpander, - boolean keepEmptyTreeArtifacts) { + Set emptyTreeArtifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { List expandedArtifacts = new ArrayList<>(); artifactExpander.expand(artifact, expandedArtifacts); output.addAll(expandedArtifacts); - if (keepEmptyTreeArtifacts && artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) { - output.add(artifact); + if (artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) { + emptyTreeArtifacts.add(artifact); } } else { output.add(artifact); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 79f864f541aba4..eef14e85af0ff8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -1246,8 +1246,7 @@ private R accumulateInputs( topLevelFilesets, input, value, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java index 8971039c3a0a69..515056c489094b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; @@ -48,8 +49,7 @@ static void addToMap( Map> topLevelFilesets, Artifact key, SkyValue value, - Environment env, - boolean requiresTreeMetadataWhenTreeFileIsInput) + Environment env) throws InterruptedException { addToMap( inputMap, @@ -60,8 +60,7 @@ static void addToMap( key, value, env, - MetadataConsumerForMetrics.NO_OP, - requiresTreeMetadataWhenTreeFileIsInput); + MetadataConsumerForMetrics.NO_OP); } /** @@ -77,8 +76,7 @@ static void addToMap( Artifact key, SkyValue value, Environment env, - MetadataConsumerForMetrics consumer, - boolean requiresTreeMetadataWhenTreeFileIsInput) + MetadataConsumerForMetrics consumer) throws InterruptedException { if (value instanceof RunfilesArtifactValue) { // Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that @@ -125,15 +123,17 @@ static void addToMap( /*depOwner=*/ key); consumer.accumulate(treeArtifactValue); } else if (value instanceof ActionExecutionValue) { - if (requiresTreeMetadataWhenTreeFileIsInput && key.isChildOfDeclaredDirectory()) { - // Actions resulting from the expansion of an ActionTemplate consume only one of the files - // in a tree artifact. However, the input prefetcher requires access to the tree metadata - // to determine the prefetch location of a tree artifact materialized as a symlink - // (cf. TreeArtifactValue#getMaterializationExecPath()). + // 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()). + if (key.isChildOfDeclaredDirectory()) { SpecialArtifact treeArtifact = key.getParent(); TreeArtifactValue treeArtifactValue = ((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact); inputMap.putTreeArtifact(treeArtifact, treeArtifactValue, /* depOwner= */ treeArtifact); + addArchivedTreeArtifactMaybe( + treeArtifact, treeArtifactValue, archivedTreeArtifacts, inputMap, key); consumer.accumulate(treeArtifactValue); } FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key); @@ -221,20 +221,27 @@ private static void expandTreeArtifactAndPopulateArtifactData( return; } - inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner); expandedArtifacts.put(treeArtifact, value.getChildren()); + inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner); + addArchivedTreeArtifactMaybe( + (SpecialArtifact) treeArtifact, value, archivedTreeArtifacts, inputMap, depOwner); + } + + private static void addArchivedTreeArtifactMaybe( + SpecialArtifact treeArtifact, + TreeArtifactValue value, + Map archivedTreeArtifacts, + ActionInputMapSink inputMap, + Artifact depOwner) { + if (!value.getArchivedRepresentation().isPresent()) { + return; + } - value - .getArchivedRepresentation() - .ifPresent( - archivedRepresentation -> { - inputMap.put( - archivedRepresentation.archivedTreeFileArtifact(), - archivedRepresentation.archivedFileValue(), - depOwner); - archivedTreeArtifacts.put( - (SpecialArtifact) treeArtifact, - archivedRepresentation.archivedTreeFileArtifact()); - }); + ArchivedRepresentation archivedRepresentation = value.getArchivedRepresentation().get(); + inputMap.put( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue(), + depOwner); + archivedTreeArtifacts.put(treeArtifact, archivedRepresentation.archivedTreeFileArtifact()); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 9bd62c7d886f97..127fc19a374a99 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -205,8 +205,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) input, artifactValue, env, - currentConsumer, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + currentConsumer); if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) { // Calling #addToMap a second time with `input` and `artifactValue` will perform no-op // updates to the secondary collections passed in (eg. expandedArtifacts, @@ -220,8 +219,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) topLevelFilesets, input, artifactValue, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 81a2ef092f6ecf..6809ca303aa19c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -350,10 +350,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) { .test(action.getMnemonic()); } - boolean requiresTreeMetadataWhenTreeFileIsInput() { - return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput(); - } - boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; }