bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Reland "Also path map transitive header jar paths with direct classpath optimization"

Open fmeum opened this issue 1 year ago • 1 comments

Rollforward of fd196bf03f4301cc3fbfc307570637f4de71c82c, which was rolled back in f5d76b1777ce861a7e551342fe25363517d53ff5.

The analysis-time memory regression (newly retained NestedSet instances) has been resolved separately in 31fae9e8e6687cbaf0dfe55a466696210c80be96. The execution-time memory regression (unconditionally flattened NestedSets) 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.

fmeum avatar Feb 17 '24 16:02 fmeum

FYI @aranguyen

fmeum avatar Feb 17 '24 18:02 fmeum

LGTM, and I can confirm most of the regression is gone (other than the small amount caused by the new field).

However since @aranguyen has an alternative change in progress, I'd like them to confirm which way they'd like to proceed with.

hvadehra avatar Feb 19 '24 12:02 hvadehra

@hvadehra I spoke with both @fmeum and @cushon about both fixes and if this updated fix does not have any serious regression, let's go ahead with this one because this makes sure the .jdeps files will only contained unmapped paths and not mixed paths. The other alternative fixing only getReducedClassPath won't be needed if the .jdeps files do not contain mix paths.

aranguyen avatar Feb 20 '24 13:02 aranguyen

@bazel-io fork 7.1.0

fmeum avatar Feb 20 '24 14:02 fmeum

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc. Thanks!

iancha1992 avatar Feb 22 '24 21:02 iancha1992