Skip to content

Commit 74fe669

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Reland "Also path map transitive header jar paths with direct classpath optimization"
Rollforward of fd196bf, which was rolled back in f5d76b1. The analysis-time memory regression (newly retained `NestedSet` instances) has been resolved separately in 31fae9e. The execution-time memory regression (unconditionally flattened `NestedSet`s) is now only incurred with path mapping enabled. Original description: `JavaHeaderCompileAction` can apply an optimization where it compiles the source files only against direct dependencies, making use of the fact that Turbine includes sufficient information about transitive dependencies into the direct header jars in a special directory. In order to ensure that downstream consumers of header compilation action can use its `.jdeps` file regardless of which of these actions actually uses path mapping, all such paths to transitive jars have to be unmapped. With this commit, actions can declare additional artifacts whose paths they want to apply path mapping to. By always passing all transitive jars into the path mapper, even when only the direct jars are inputs to the action, the transitive header jar paths can be unmapped and stripped path collisions between them correctly result in a noop `PathMapper` being used for the current action. Closes #21401. PiperOrigin-RevId: 609010786 Change-Id: I0d9ea5b11430ee40be74fe582af84fedaa52ade6
1 parent e84d053 commit 74fe669

File tree

6 files changed

+151
-48
lines changed

6 files changed

+151
-48
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,4 +653,12 @@ public ImmutableMap<String, String> getExecProperties() {
653653
public PlatformInfo getExecutionPlatform() {
654654
return owner.getExecutionPlatform();
655655
}
656+
657+
/**
658+
* Returns artifacts that should be subject to path mapping (see {@link Spawn#getPathMapper()},
659+
* but aren't inputs of the action.
660+
*/
661+
public NestedSet<Artifact> getAdditionalArtifactsForPathMapping() {
662+
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
663+
}
656664
}

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

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package com.google.devtools.build.lib.analysis.actions;
1616

1717
import com.google.common.collect.ImmutableSet;
18+
import com.google.devtools.build.lib.actions.AbstractAction;
1819
import com.google.devtools.build.lib.actions.Action;
1920
import com.google.devtools.build.lib.actions.ActionKeyContext;
2021
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
@@ -63,8 +64,8 @@ public final class PathMappers {
6364
* Actions that support path mapping should call this method from {@link
6465
* Action#getKey(ActionKeyContext, ArtifactExpander)}.
6566
*
66-
* <p>Compared to {@link #create(Action, OutputPathsMode)}, this method does not flatten nested
67-
* sets and thus can't result in memory regressions.
67+
* <p>Compared to {@link #create(AbstractAction, OutputPathsMode)}, this method does not flatten
68+
* nested sets and thus can't result in memory regressions.
6869
*
6970
* @param mnemonic the mnemonic of the action
7071
* @param executionInfo the execution info of the action
@@ -103,22 +104,12 @@ public static void addToFingerprint(
103104
* OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)}
104105
* to ensure correct incremental builds.
105106
*
106-
* @param action the {@link Action} for which a {@link Spawn} is to be created
107+
* @param action the {@link AbstractAction} for which a {@link Spawn} is to be created
107108
* @param outputPathsMode the value of {@link CoreOptions#outputPathsMode}
108109
* @return a {@link PathMapper} that maps paths of the action's inputs and outputs. May be {@link
109110
* PathMapper#NOOP} if path mapping is not applicable to the action.
110111
*/
111-
public static PathMapper create(Action action, OutputPathsMode outputPathsMode) {
112-
if (getEffectiveOutputPathsMode(
113-
outputPathsMode, action.getMnemonic(), action.getExecutionInfo())
114-
!= OutputPathsMode.STRIP) {
115-
return PathMapper.NOOP;
116-
}
117-
return StrippingPathMapper.tryCreate(action).orElse(PathMapper.NOOP);
118-
}
119-
120-
public static PathMapper createPathMapperForTesting(
121-
Action action, OutputPathsMode outputPathsMode) {
112+
public static PathMapper create(AbstractAction action, OutputPathsMode outputPathsMode) {
122113
if (getEffectiveOutputPathsMode(
123114
outputPathsMode, action.getMnemonic(), action.getExecutionInfo())
124115
!= OutputPathsMode.STRIP) {
@@ -128,8 +119,8 @@ public static PathMapper createPathMapperForTesting(
128119
}
129120

130121
/**
131-
* Helper method to simplify calling {@link #create(Action, OutputPathsMode)} for actions that
132-
* store the configuration directly.
122+
* Helper method to simplify calling {@link #create(SpawnAction, OutputPathsMode)} for actions
123+
* that store the configuration directly.
133124
*
134125
* @param configuration the configuration
135126
* @return the value of

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

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package com.google.devtools.build.lib.analysis.actions;
1616

1717
import com.google.common.collect.ImmutableList;
18+
import com.google.common.collect.Iterables;
19+
import com.google.devtools.build.lib.actions.AbstractAction;
1820
import com.google.devtools.build.lib.actions.Action;
1921
import com.google.devtools.build.lib.actions.ActionInput;
2022
import com.google.devtools.build.lib.actions.ActionInputHelper.BasicActionInput;
@@ -26,9 +28,8 @@
2628
import com.google.devtools.build.lib.actions.PathMapper;
2729
import com.google.devtools.build.lib.actions.Spawn;
2830
import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode;
29-
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
3031
import com.google.devtools.build.lib.vfs.PathFragment;
31-
import java.util.HashSet;
32+
import java.util.HashMap;
3233
import java.util.List;
3334
import java.util.Objects;
3435
import java.util.Optional;
@@ -88,10 +89,15 @@ public final class StrippingPathMapper {
8889
* @param action the action to potentially strip paths from
8990
* @return a {@link StrippingPathMapper} if the action supports it, else {@link Optional#empty()}.
9091
*/
91-
static Optional<PathMapper> tryCreate(Action action) {
92+
static Optional<PathMapper> tryCreate(AbstractAction action) {
9293
// This is expected to always be "bazel-out", but we don't want to hardcode it here.
9394
PathFragment outputRoot = action.getPrimaryOutput().getExecPath().subFragment(0, 1);
94-
if (isPathStrippable(action.getInputs(), outputRoot)) {
95+
// Additional artifacts to map are not part of the action's inputs, but may still lead to
96+
// path collisions after stripping. It is thus important to include them in this check.
97+
if (isPathStrippable(
98+
Iterables.concat(
99+
action.getInputs().toList(), action.getAdditionalArtifactsForPathMapping().toList()),
100+
outputRoot)) {
95101
return Optional.of(
96102
create(action.getMnemonic(), action instanceof StarlarkAction, outputRoot));
97103
}
@@ -250,7 +256,7 @@ private static boolean isOutputPath(PathFragment pathFragment, PathFragment outp
250256
* <p>This method checks b).
251257
*/
252258
private static boolean isPathStrippable(
253-
NestedSet<? extends ActionInput> actionInputs, PathFragment outputRoot) {
259+
Iterable<? extends ActionInput> actionInputs, PathFragment outputRoot) {
254260
// For qualifying action types, check that no inputs or outputs would clash if paths were
255261
// removed, e.g. "bazel-out/k8-fastbuild/foo" and "bazel-out/host/foo".
256262
//
@@ -261,15 +267,15 @@ private static boolean isPathStrippable(
261267
// with configurations). While this would help more action instances qualify, it also blocks
262268
// caching the same action in host and target configurations. This could be mitigated by
263269
// stripping the host prefix *only* when the entire action is in the host configuration.
264-
HashSet<PathFragment> rootRelativePaths = new HashSet<>();
265-
for (ActionInput input : actionInputs.toList()) {
270+
HashMap<PathFragment, ActionInput> rootRelativePaths = new HashMap<>();
271+
for (ActionInput input : actionInputs) {
266272
if (!isOutputPath(input, outputRoot)) {
267273
continue;
268274
}
269275
// For "bazel-out/k8-fastbuild/foo/bar", get "foo/bar".
270-
if (!rootRelativePaths.add(input.getExecPath().subFragment(2))) {
271-
// TODO(bazel-team): don't fail on duplicate inputs, i.e. when the same exact exec path
272-
// (including config prefix) is included twice.
276+
if (!rootRelativePaths
277+
.computeIfAbsent(input.getExecPath().subFragment(2), k -> input)
278+
.equals(input)) {
273279
return false;
274280
}
275281
}

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java‎

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
import com.google.common.annotations.VisibleForTesting;
2626
import com.google.common.base.Joiner;
2727
import com.google.common.base.Strings;
28+
import com.google.common.collect.BiMap;
29+
import com.google.common.collect.HashBiMap;
2830
import com.google.common.collect.ImmutableList;
2931
import com.google.common.collect.ImmutableMap;
3032
import com.google.common.collect.ImmutableSet;
@@ -82,7 +84,6 @@
8284
import java.io.IOException;
8385
import java.io.InputStream;
8486
import java.util.ArrayList;
85-
import java.util.HashMap;
8687
import java.util.HashSet;
8788
import java.util.LinkedHashMap;
8889
import java.util.List;
@@ -289,7 +290,6 @@ static class ReducedClasspath {
289290
}
290291
}
291292

292-
293293
private JavaSpawn getReducedSpawn(
294294
ActionExecutionContext actionExecutionContext,
295295
ReducedClasspath reducedClasspath,
@@ -721,13 +721,16 @@ public NestedSet<Artifact> getPossibleInputsForTesting() {
721721
* @param spawnResult the executor action that created the possibly stripped .jdeps output
722722
* @param outputDepsProto path to the .jdeps output
723723
* @param actionInputs all inputs to the current action
724+
* @param additionalArtifactsForPathMapping any additional artifacts that may be referenced in the
725+
* .jdeps file by path
724726
* @param actionExecutionContext the action execution context
725727
* @return the full deps proto (also written to disk to satisfy the action's declared output)
726728
*/
727729
static Deps.Dependencies createFullOutputDeps(
728730
SpawnResult spawnResult,
729731
Artifact outputDepsProto,
730732
NestedSet<Artifact> actionInputs,
733+
NestedSet<Artifact> additionalArtifactsForPathMapping,
731734
ActionExecutionContext actionExecutionContext,
732735
PathMapper pathMapper)
733736
throws IOException {
@@ -739,36 +742,51 @@ static Deps.Dependencies createFullOutputDeps(
739742
return executorJdeps;
740743
}
741744

745+
// No paths to rewrite.
746+
if (executorJdeps.getDependencyCount() == 0) {
747+
return executorJdeps;
748+
}
749+
742750
// For each of the action's generated inputs, revert its mapped path back to its original path.
743-
Map<String, PathFragment> mappedToOriginalPath = new HashMap<>();
744-
for (Artifact actionInput : actionInputs.toList()) {
751+
BiMap<String, PathFragment> mappedToOriginalPath = HashBiMap.create();
752+
for (Artifact actionInput :
753+
Iterables.concat(actionInputs.toList(), additionalArtifactsForPathMapping.toList())) {
745754
if (actionInput.isSourceArtifact()) {
746755
continue;
747756
}
748757
String mappedPath = pathMapper.getMappedExecPathString(actionInput);
749-
if (mappedToOriginalPath.put(mappedPath, actionInput.getExecPath()) != null) {
750-
// If an entry already exists, that means different inputs reduce to the same stripped path.
751-
// That also means PathStripper would exempt this action from path stripping, so the
752-
// executor-produced .jdeps already includes full paths. No need to update it.
753-
return executorJdeps;
758+
PathFragment previousPath = mappedToOriginalPath.put(mappedPath, actionInput.getExecPath());
759+
if (previousPath != null && !previousPath.equals(actionInput.getExecPath())) {
760+
throw new IllegalStateException(
761+
String.format(
762+
"Duplicate mapped path %s derived from %s and %s",
763+
mappedPath, actionInput.getExecPath(), mappedToOriginalPath.get(mappedPath)));
754764
}
755765
}
756766

757-
// No paths to rewrite.
758-
if (executorJdeps.getDependencyCount() == 0) {
759-
return executorJdeps;
760-
}
761-
762767
// Rewrite the .jdeps proto with full paths.
763768
PathFragment outputRoot = outputDepsProto.getExecPath().subFragment(0, 1);
764769
Deps.Dependencies.Builder fullDepsBuilder = Deps.Dependencies.newBuilder(executorJdeps);
765770
for (Deps.Dependency.Builder dep : fullDepsBuilder.getDependencyBuilderList()) {
766771
PathFragment pathOnExecutor = PathFragment.create(dep.getPath());
767772
PathFragment originalPath = mappedToOriginalPath.get(pathOnExecutor.getPathString());
768-
if (originalPath == null && pathOnExecutor.subFragment(0, 1).equals(outputRoot)) {
769-
// The mapped path -> full path map failed, which means the paths weren't mapped. Fast-
770-
// return the original jdeps to save unnecessary CPU time.
771-
return executorJdeps;
773+
// Source files, which do not lie under the output root, are not mapped. It is also possible
774+
// that a jdeps file contains a reference to a transitive classpath element that isn't an
775+
// input to the current action (see
776+
// https://github.com/google/turbine/commit/f9f2decee04a3c651671f7488a7c9d7952df88c8), just an
777+
// additional artifact marked for path mapping, and itself wasn't built with path mapping
778+
// enabled (e .g. due to path collisions). In that case, the path will already be unmapped and
779+
// we can leave it as is. For entirely unexpected paths, we still report an error.
780+
if (originalPath == null
781+
&& pathOnExecutor.subFragment(0, 1).equals(outputRoot)
782+
&& !mappedToOriginalPath.containsValue(pathOnExecutor)) {
783+
throw new IllegalStateException(
784+
String.format(
785+
"Missing original path for mapped path %s in %s%njdeps: %s%npath map: %s",
786+
pathOnExecutor,
787+
outputDepsProto.getExecPath(),
788+
executorJdeps,
789+
mappedToOriginalPath));
772790
}
773791
dep.setPath(
774792
originalPath == null ? pathOnExecutor.getPathString() : originalPath.getPathString());
@@ -816,7 +834,12 @@ private Deps.Dependencies readFullOutputDeps(
816834
SpawnResult result = Iterables.getOnlyElement(results);
817835
try {
818836
return createFullOutputDeps(
819-
result, outputDepsProto, getInputs(), actionExecutionContext, pathMapper);
837+
result,
838+
outputDepsProto,
839+
getInputs(),
840+
getAdditionalArtifactsForPathMapping(),
841+
actionExecutionContext,
842+
pathMapper);
820843
} catch (IOException e) {
821844
throw ActionExecutionException.fromExecException(
822845
new EnvironmentalExecException(

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@
7676
public final class JavaHeaderCompileAction extends SpawnAction {
7777

7878
private final boolean insertDependencies;
79+
private final NestedSet<Artifact> additionalArtifactsForPathMapping;
7980

8081
private JavaHeaderCompileAction(
8182
ActionOwner owner,
@@ -89,7 +90,8 @@ private JavaHeaderCompileAction(
8990
CharSequence progressMessage,
9091
String mnemonic,
9192
OutputPathsMode outputPathsMode,
92-
boolean insertDependencies) {
93+
boolean insertDependencies,
94+
NestedSet<Artifact> additionalArtifactsForPathMapping) {
9395
super(
9496
owner,
9597
tools,
@@ -103,6 +105,12 @@ private JavaHeaderCompileAction(
103105
mnemonic,
104106
outputPathsMode);
105107
this.insertDependencies = insertDependencies;
108+
this.additionalArtifactsForPathMapping = additionalArtifactsForPathMapping;
109+
}
110+
111+
@Override
112+
public NestedSet<Artifact> getAdditionalArtifactsForPathMapping() {
113+
return additionalArtifactsForPathMapping;
106114
}
107115

108116
@Override
@@ -113,7 +121,12 @@ protected void afterExecute(
113121
try {
114122
Deps.Dependencies fullOutputDeps =
115123
JavaCompileAction.createFullOutputDeps(
116-
spawnResult, outputDepsProto, getInputs(), context, pathMapper);
124+
spawnResult,
125+
outputDepsProto,
126+
getInputs(),
127+
getAdditionalArtifactsForPathMapping(),
128+
context,
129+
pathMapper);
117130
JavaCompileActionContext javaContext = context.getContext(JavaCompileActionContext.class);
118131
if (insertDependencies && javaContext != null) {
119132
javaContext.insertDependencies(outputDepsProto, fullOutputDeps);
@@ -459,10 +472,20 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
459472
}
460473
if (useDirectClasspath) {
461474
NestedSet<Artifact> classpath;
475+
NestedSet<Artifact> additionalArtifactsForPathMapping;
462476
if (!directJars.isEmpty() || classpathEntries.isEmpty()) {
463477
classpath = directJars;
478+
// When using the direct classpath optimization, Turbine generates .jdeps entries based on
479+
// the transitive dependency information packages into META-INF/TRANSITIVE. When path
480+
// mapping is used, these entries may have been subject to it when they were generated.
481+
// Since the contents of that directory are not unmapped, we need to instead unmap the
482+
// paths emitted in the .jdeps file, which requires knowing the full list of artifact
483+
// paths even if they aren't inputs to the current action.
484+
// https://github.com/google/turbine/commit/f9f2decee04a3c651671f7488a7c9d7952df88c8
485+
additionalArtifactsForPathMapping = classpathEntries;
464486
} else {
465487
classpath = classpathEntries;
488+
additionalArtifactsForPathMapping = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
466489
}
467490
mandatoryInputsBuilder.addTransitive(classpath);
468491

@@ -494,7 +517,8 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
494517
// If classPathMode == BAZEL, also make sure to inject the dependencies to be
495518
// available to downstream actions. Else just do enough work to locally create the
496519
// full .jdeps from the .stripped .jdeps produced on the executor.
497-
/* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL));
520+
/* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL,
521+
additionalArtifactsForPathMapping));
498522
return;
499523
}
500524

‎src/test/shell/integration/config_stripped_outputs_test.sh‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,4 +328,55 @@ EOF
328328
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/libbase_lib-hjar.jdeps"
329329
}
330330

331+
function test_direct_classpath() {
332+
local -r pkg="${FUNCNAME[0]}"
333+
mkdir -p "$pkg/java/hello/" || fail "Expected success"
334+
# When compiling C, the direct classpath optimization in Turbine embeds information about the
335+
# dependency D into the header jar, which then results in the path to Ds header jar being included
336+
# in the jdeps file for B. The full compilation action for A requires the header jar for D and
337+
# thus the path to it in the jdeps file of B has to be unmapped properly for the reduced classpath
338+
# created for A to contain it.
339+
cat > "$pkg/java/hello/A.java" <<'EOF'
340+
package hello;
341+
public class A extends B {}
342+
EOF
343+
cat > "$pkg/java/hello/B.java" <<'EOF'
344+
package hello;
345+
public class B extends C {}
346+
EOF
347+
cat > "$pkg/java/hello/C.java" <<'EOF'
348+
package hello;
349+
public class C extends D {}
350+
EOF
351+
cat > "$pkg/java/hello/D.java" <<'EOF'
352+
package hello;
353+
public class D {}
354+
EOF
355+
cat > "$pkg/java/hello/BUILD" <<'EOF'
356+
java_library(name='a', srcs=['A.java'], deps = [':b'])
357+
java_library(name='b', srcs=['B.java'], deps = [':c'])
358+
java_library(name='c', srcs=['C.java'], deps = [':d'])
359+
java_library(name='d', srcs=['D.java'])
360+
EOF
361+
362+
bazel build --experimental_java_classpath=bazel \
363+
--experimental_output_paths=strip \
364+
//"$pkg"/java/hello:a -s 2>"$TEST_log" \
365+
|| fail "Expected success"
366+
367+
# java_library .jar compilation:
368+
assert_paths_stripped "$TEST_log" "$pkg/java/hello/liba.jar-0.params"
369+
# java_library header jar compilation:
370+
assert_paths_stripped "$TEST_log" "bin/$pkg/java/hello/libb-hjar.jar"
371+
# jdeps files should contain the original paths since they are read by downstream actions that may
372+
# not use path mapping.
373+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/liba.jdeps"
374+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libb.jdeps"
375+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libb-hjar.jdeps"
376+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libc.jdeps"
377+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libc-hjar.jdeps"
378+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libd.jdeps"
379+
assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libd-hjar.jdeps"
380+
}
381+
331382
run_suite "Tests stripping config prefixes from output paths for better action caching"

0 commit comments

Comments
 (0)