From c48392c54a92b7bc5b04883bd6499c7a5677205d 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 ed6889c1c611a3..f992f833628eea 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 571cdc72a5969f..c5e79a90ee5962 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 7c0f9e9fdcb6b3..62723330f9ccd8 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 @@ -1310,8 +1310,7 @@ private R accumulateInputs( topLevelFilesets, input, value, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } else if (!hasMissingInputs && input.hasKnownGeneratingAction()) { // Derived inputs are mandatory, but we did not detect any missing inputs. This is only // possible for indirect inputs (beneath an ArtifactNestedSetKey) when, between the time the 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 df6b92fdb22a6d..42ad29b2091958 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 @@ -343,10 +343,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) { .test(action.getMnemonic()); } - boolean requiresTreeMetadataWhenTreeFileIsInput() { - return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput(); - } - boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; }