Skip to content

Commit e023bd3

Browse files
fmeumfmeum
authored
Don't eagerly flatten a NestedSet in RepoMappingManifestAction (#18419)
Delays the flattening from action creation to action execution in preparation for making this action accessible from Starlark. Work towards #17941 Closes #18349. PiperOrigin-RevId: 531165675 Change-Id: Ia2c175834d409c30303dd3ecba0dd276ea2cd905
1 parent a5c0d85 commit e023bd3

File tree

5 files changed

+183
-101
lines changed

5 files changed

+183
-101
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -995,9 +995,9 @@ java_library(
995995
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
996996
"//src/main/java/com/google/devtools/build/lib/cmdline",
997997
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
998+
"//src/main/java/com/google/devtools/build/lib/packages",
998999
"//src/main/java/com/google/devtools/build/lib/util",
9991000
"//src/main/java/net/starlark/java/eval",
1000-
"//third_party:auto_value",
10011001
"//third_party:guava",
10021002
"//third_party:jsr305",
10031003
],

‎src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java‎

Lines changed: 82 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -13,61 +13,69 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.analysis;
1515

16+
import static com.google.common.collect.ImmutableSet.toImmutableSet;
17+
import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
1618
import static java.nio.charset.StandardCharsets.ISO_8859_1;
1719
import static java.util.Comparator.comparing;
1820

19-
import com.google.auto.value.AutoValue;
20-
import com.google.common.collect.ImmutableList;
21+
import com.google.common.collect.ImmutableSet;
22+
import com.google.common.collect.ImmutableSortedMap;
2123
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2224
import com.google.devtools.build.lib.actions.ActionKeyContext;
2325
import com.google.devtools.build.lib.actions.ActionOwner;
2426
import com.google.devtools.build.lib.actions.Artifact;
2527
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
2628
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
29+
import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
2730
import com.google.devtools.build.lib.actions.ExecException;
2831
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
2932
import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
33+
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
3034
import com.google.devtools.build.lib.cmdline.RepositoryName;
35+
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
3136
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
3237
import com.google.devtools.build.lib.collect.nestedset.Order;
38+
import com.google.devtools.build.lib.packages.Package;
3339
import com.google.devtools.build.lib.util.Fingerprint;
3440
import java.io.PrintWriter;
35-
import java.util.List;
41+
import java.util.Map.Entry;
3642
import java.util.UUID;
3743
import javax.annotation.Nullable;
3844
import net.starlark.java.eval.EvalException;
3945

4046
/** Creates a manifest file describing the repos and mappings relevant for a runfile tree. */
41-
public class RepoMappingManifestAction extends AbstractFileWriteAction {
42-
private static final UUID MY_UUID = UUID.fromString("458e351c-4d30-433d-b927-da6cddd4737f");
43-
44-
private final ImmutableList<Entry> entries;
45-
private final String workspaceName;
47+
public final class RepoMappingManifestAction extends AbstractFileWriteAction {
4648

47-
/** An entry in the repo mapping manifest file. */
48-
@AutoValue
49-
public abstract static class Entry {
50-
public static Entry of(
51-
RepositoryName sourceRepo, String targetRepoApparentName, RepositoryName targetRepo) {
52-
return new AutoValue_RepoMappingManifestAction_Entry(
53-
sourceRepo, targetRepoApparentName, targetRepo);
54-
}
49+
private static final UUID MY_UUID = UUID.fromString("458e351c-4d30-433d-b927-da6cddd4737f");
5550

56-
public abstract RepositoryName sourceRepo();
51+
// Uses MapFn's args parameter just like Fingerprint#addString to compute a cacheable fingerprint
52+
// of just the repo name and mapping of a given Package.
53+
private static final MapFn<Package> REPO_AND_MAPPING_DIGEST_FN =
54+
(pkg, args) -> {
55+
args.accept(pkg.getPackageIdentifier().getRepository().getName());
5756

58-
public abstract String targetRepoApparentName();
57+
var mapping = pkg.getRepositoryMapping().entries();
58+
args.accept(String.valueOf(mapping.size()));
59+
mapping.forEach(
60+
(apparentName, canonicalName) -> {
61+
args.accept(apparentName);
62+
args.accept(canonicalName.getName());
63+
});
64+
};
5965

60-
public abstract RepositoryName targetRepo();
61-
}
66+
private final NestedSet<Package> transitivePackages;
67+
private final NestedSet<Artifact> runfilesArtifacts;
68+
private final String workspaceName;
6269

6370
public RepoMappingManifestAction(
64-
ActionOwner owner, Artifact output, List<Entry> entries, String workspaceName) {
71+
ActionOwner owner,
72+
Artifact output,
73+
NestedSet<Package> transitivePackages,
74+
NestedSet<Artifact> runfilesArtifacts,
75+
String workspaceName) {
6576
super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /*makeExecutable=*/ false);
66-
this.entries =
67-
ImmutableList.sortedCopyOf(
68-
comparing((Entry e) -> e.sourceRepo().getName())
69-
.thenComparing(Entry::targetRepoApparentName),
70-
entries);
77+
this.transitivePackages = transitivePackages;
78+
this.runfilesArtifacts = runfilesArtifacts;
7179
this.workspaceName = workspaceName;
7280
}
7381

@@ -78,7 +86,7 @@ public String getMnemonic() {
7886

7987
@Override
8088
protected String getRawProgressMessage() {
81-
return "writing repo mapping manifest for " + getOwner().getLabel();
89+
return "Writing repo mapping manifest for " + getOwner().getLabel();
8290
}
8391

8492
@Override
@@ -88,35 +96,61 @@ protected void computeKey(
8896
Fingerprint fp)
8997
throws CommandLineExpansionException, EvalException, InterruptedException {
9098
fp.addUUID(MY_UUID);
99+
actionKeyContext.addNestedSetToFingerprint(REPO_AND_MAPPING_DIGEST_FN, fp, transitivePackages);
100+
actionKeyContext.addNestedSetToFingerprint(fp, runfilesArtifacts);
91101
fp.addString(workspaceName);
92-
for (Entry entry : entries) {
93-
fp.addString(entry.sourceRepo().getName());
94-
fp.addString(entry.targetRepoApparentName());
95-
fp.addString(entry.targetRepo().getName());
96-
}
97102
}
98103

99104
@Override
100105
public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx)
101106
throws InterruptedException, ExecException {
102107
return out -> {
103-
PrintWriter writer = new PrintWriter(out, /*autoFlush=*/ false, ISO_8859_1);
104-
for (Entry entry : entries) {
105-
if (entry.targetRepoApparentName().isEmpty()) {
106-
// The apparent repo name can only be empty for the main repo. We skip this line as
107-
// Rlocation paths can't reference an empty apparent name anyway.
108-
continue;
109-
}
110-
// The canonical name of the main repo is the empty string, which is not a valid name for a
111-
// directory, so the "workspace name" is used the name of the directory under the runfiles
112-
// tree for it.
113-
String targetRepoDirectoryName =
114-
entry.targetRepo().isMain() ? workspaceName : entry.targetRepo().getName();
115-
writer.format(
116-
"%s,%s,%s\n",
117-
entry.sourceRepo().getName(), entry.targetRepoApparentName(), targetRepoDirectoryName);
118-
}
108+
PrintWriter writer = new PrintWriter(out, /* autoFlush= */ false, ISO_8859_1);
109+
110+
ImmutableSet<RepositoryName> reposContributingRunfiles =
111+
runfilesArtifacts.toList().stream()
112+
.filter(a -> a.getOwner() != null)
113+
.map(a -> a.getOwner().getRepository())
114+
.collect(toImmutableSet());
115+
transitivePackages.toList().stream()
116+
.collect(
117+
toImmutableSortedMap(
118+
comparing(RepositoryName::getName),
119+
pkg -> pkg.getPackageIdentifier().getRepository(),
120+
Package::getRepositoryMapping,
121+
// All packages in a given repository have the same repository mapping, so the
122+
// particular way of resolving duplicates does not matter.
123+
(first, second) -> first))
124+
.forEach(
125+
(repoName, mapping) ->
126+
writeRepoMapping(writer, reposContributingRunfiles, repoName, mapping));
119127
writer.flush();
120128
};
121129
}
130+
131+
private void writeRepoMapping(
132+
PrintWriter writer,
133+
ImmutableSet<RepositoryName> reposContributingRunfiles,
134+
RepositoryName repoName,
135+
RepositoryMapping repoMapping) {
136+
for (Entry<String, RepositoryName> mappingEntry :
137+
ImmutableSortedMap.copyOf(repoMapping.entries()).entrySet()) {
138+
if (mappingEntry.getKey().isEmpty()) {
139+
// The apparent repo name can only be empty for the main repo. We skip this line as
140+
// Rlocation paths can't reference an empty apparent name anyway.
141+
continue;
142+
}
143+
if (!reposContributingRunfiles.contains(mappingEntry.getValue())) {
144+
// We only write entries for repos that actually contribute runfiles.
145+
continue;
146+
}
147+
// The canonical name of the main repo is the empty string, which is not a valid name for a
148+
// directory, so the "workspace name" is used the name of the directory under the runfiles
149+
// tree for it.
150+
String targetRepoDirectoryName =
151+
mappingEntry.getValue().isMain() ? workspaceName : mappingEntry.getValue().getName();
152+
writer.format(
153+
"%s,%s,%s\n", repoName.getName(), mappingEntry.getKey(), targetRepoDirectoryName);
154+
}
155+
}
122156
}

‎src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java‎

Lines changed: 2 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -14,34 +14,27 @@
1414

1515
package com.google.devtools.build.lib.analysis;
1616

17-
import static com.google.common.collect.ImmutableSet.toImmutableSet;
18-
1917
import com.google.common.annotations.VisibleForTesting;
2018
import com.google.common.base.Preconditions;
21-
import com.google.common.collect.ImmutableList;
2219
import com.google.common.collect.ImmutableSet;
2320
import com.google.devtools.build.lib.actions.ActionEnvironment;
2421
import com.google.devtools.build.lib.actions.Artifact;
2522
import com.google.devtools.build.lib.actions.CommandLine;
26-
import com.google.devtools.build.lib.analysis.RepoMappingManifestAction.Entry;
2723
import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType;
2824
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
2925
import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction;
3026
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
3127
import com.google.devtools.build.lib.analysis.config.RunUnder;
32-
import com.google.devtools.build.lib.cmdline.RepositoryName;
3328
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
3429
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
3530
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
36-
import com.google.devtools.build.lib.packages.Package;
3731
import com.google.devtools.build.lib.packages.TargetUtils;
3832
import com.google.devtools.build.lib.packages.Type;
3933
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
4034
import com.google.devtools.build.lib.vfs.FileSystemUtils;
4135
import com.google.devtools.build.lib.vfs.Path;
4236
import com.google.devtools.build.lib.vfs.PathFragment;
4337
import java.util.Collection;
44-
import java.util.HashSet;
4538
import java.util.LinkedHashSet;
4639
import java.util.List;
4740
import java.util.Map;
@@ -558,45 +551,9 @@ private static Artifact createRepoMappingManifestAction(
558551
new RepoMappingManifestAction(
559552
ruleContext.getActionOwner(),
560553
repoMappingManifest,
561-
collectRepoMappings(
562-
Preconditions.checkNotNull(
563-
ruleContext.getTransitivePackagesForRunfileRepoMappingManifest()),
564-
runfiles),
554+
ruleContext.getTransitivePackagesForRunfileRepoMappingManifest(),
555+
runfiles.getAllArtifacts(),
565556
ruleContext.getWorkspaceName()));
566557
return repoMappingManifest;
567558
}
568-
569-
/** Returns the list of entries (unsorted) that should appear in the repo mapping manifest. */
570-
private static ImmutableList<Entry> collectRepoMappings(
571-
NestedSet<Package> transitivePackages, Runfiles runfiles) {
572-
// NOTE: It might appear that the flattening of `transitivePackages` is better suited to the
573-
// execution phase rather than here in the analysis phase, but we can't do that since it would
574-
// necessitate storing `transitivePackages` in an action, which breaks skyframe serialization
575-
// since packages cannot be serialized here.
576-
577-
ImmutableSet<RepositoryName> reposContributingRunfiles =
578-
runfiles.getAllArtifacts().toList().stream()
579-
.filter(a -> a.getOwner() != null)
580-
.map(a -> a.getOwner().getRepository())
581-
.collect(toImmutableSet());
582-
Set<RepositoryName> seenRepos = new HashSet<>();
583-
ImmutableList.Builder<Entry> entries = ImmutableList.builder();
584-
for (Package pkg : transitivePackages.toList()) {
585-
if (!seenRepos.add(pkg.getPackageIdentifier().getRepository())) {
586-
// Any package from the same repo would have the same repo mapping.
587-
continue;
588-
}
589-
for (Map.Entry<String, RepositoryName> repoMappingEntry :
590-
pkg.getRepositoryMapping().entries().entrySet()) {
591-
if (reposContributingRunfiles.contains(repoMappingEntry.getValue())) {
592-
entries.add(
593-
Entry.of(
594-
pkg.getPackageIdentifier().getRepository(),
595-
repoMappingEntry.getKey(),
596-
repoMappingEntry.getValue()));
597-
}
598-
}
599-
}
600-
return entries.build();
601-
}
602559
}

‎src/test/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -361,16 +361,14 @@ java_test(
361361
srcs = ["RunfilesRepoMappingManifestTest.java"],
362362
deps = [
363363
"//src/main/java/com/google/devtools/build/lib/actions",
364-
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
364+
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
365365
"//src/main/java/com/google/devtools/build/lib/analysis:repo_mapping_manifest_action",
366-
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
367366
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl",
368367
"//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options",
369368
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
370-
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
369+
"//src/main/java/com/google/devtools/build/lib/util",
371370
"//src/main/java/com/google/devtools/build/lib/vfs",
372-
"//src/main/java/com/google/devtools/build/skyframe",
373-
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
371+
"//src/main/java/net/starlark/java/eval",
374372
"//src/test/java/com/google/devtools/build/lib/analysis/util",
375373
"//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
376374
"//third_party:guava",

0 commit comments

Comments
 (0)