Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.2.0] Automatically map paths in Args#map_each #22221

Closed
bazel-io opened this issue May 2, 2024 · 3 comments
Closed

[7.2.0] Automatically map paths in Args#map_each #22221

bazel-io opened this issue May 2, 2024 · 3 comments

Comments

@bazel-io
Copy link
Member

bazel-io commented May 2, 2024

Forked from #21952

@bazel-io bazel-io added this to the 7.2.0 release blockers milestone May 2, 2024
@bazel-io
Copy link
Member Author

bazel-io commented May 2, 2024

Cherry-pick was attempted but there were merge conflicts in the following file(s). Please resolve manually.

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java
src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

cc: @bazelbuild/triage

@iancha1992
Copy link
Member

Cherry-pick was attempted but there were merge conflicts in the following file(s). Please resolve manually.

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

cc: @bazelbuild/triage

cc: @fmeum @aranguyen @lberki

fmeum added a commit to fmeum/bazel that referenced this issue May 10, 2024
When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped.

Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping.

This is a reland of 955b31e, which got rolled back due to a 7% CPU time increase on a benchmark caused by frequent comparisons of equal but not reference equal `StarlarkSemantics` used as keys in the `StarlarkClassDescriptor` cache in `CallUtils`. Instead of overriding `equals` and `hashCode` for `PathMapper`, this change subclasses `StarlarkSemantics` to provide a different, reference equal instance as the cache key. This is safe since the value associated with the `path_mapper` key does not affect the availability of any Starlark field or method, just the behavior of their implementations.

Work towards bazelbuild#6526

Closes bazelbuild#21952.

PiperOrigin-RevId: 629546010
Change-Id: Ib21fa2371a28a02f0c868523b410c5a40c2c6c82

Closes bazelbuild#22221
@keertk
Copy link
Member

keertk commented May 10, 2024

Cherry-picked in #22322

fmeum added a commit to fmeum/bazel that referenced this issue May 10, 2024
When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped.

Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping.

This is a reland of 955b31e, which got rolled back due to a 7% CPU time increase on a benchmark caused by frequent comparisons of equal but not reference equal `StarlarkSemantics` used as keys in the `StarlarkClassDescriptor` cache in `CallUtils`. Instead of overriding `equals` and `hashCode` for `PathMapper`, this change subclasses `StarlarkSemantics` to provide a different, reference equal instance as the cache key. This is safe since the value associated with the `path_mapper` key does not affect the availability of any Starlark field or method, just the behavior of their implementations.

Work towards bazelbuild#6526

Closes bazelbuild#21952.

PiperOrigin-RevId: 629546010
Change-Id: Ib21fa2371a28a02f0c868523b410c5a40c2c6c82

Closes bazelbuild#22221
github-merge-queue bot pushed a commit that referenced this issue May 10, 2024
When path mapping is enabled, `File` objects accessed in a user-supplied
callback passed to `Args#map_each` automatically have their paths
mapped.

Automatic rewriting is preferable to e.g. passing a rewriter object into
the callback: All paths emitted into command lines must be rewritten
with path mapping as inputs and outputs are staged at the mapped
locations, so the user would need to manually map all paths - there is
no choice. As an added benefit, the automatic rewriting ensures that
existing rules relying on `map_each` do not need to be modified to use
path mapping.

This is a reland of 955b31e, which got
rolled back due to a 7% CPU time increase on a benchmark caused by
frequent comparisons of equal but not reference equal
`StarlarkSemantics` used as keys in the `StarlarkClassDescriptor` cache
in `CallUtils`. Instead of overriding `equals` and `hashCode` for
`PathMapper`, this change subclasses `StarlarkSemantics` to provide a
different, reference equal instance as the cache key. This is safe since
the value associated with the `path_mapper` key does not affect the
availability of any Starlark field or method, just the behavior of their
implementations.

Work towards #6526

Closes #21952.

PiperOrigin-RevId: 629546010
Change-Id: Ib21fa2371a28a02f0c868523b410c5a40c2c6c82

Closes #22221
@keertk keertk closed this as completed May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants