Automatically map paths in `Args#map_each` #21952

fmeum · 2024-04-10T08:08:48Z

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

aranguyen · 2024-04-10T18:34:53Z

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

+   */
+  @CheckReturnValue
+  default StarlarkSemantics storeIn(StarlarkSemantics semantics) {
+    return new StarlarkSemantics(semantics.toBuilder().set(SEMANTICS_KEY, this).build()) {


is it possible to avoid creating new instance of StarlarkSemantics for NOOP case?

I can do that, but could you confirm that the benchmarks are good before we make that change?

The benchmark is still running. I will let you know asap

Here is the result

metric median Δ 1-pval cpu: 16852.300s ±505.3s +442.1s, +2.7% 0.92 (weakly significant) memory: 11110MB ±1.6MB -2.0MB, -0.0% 0.13 (not significant) system: 1078.910s ±55.8s -5.8s, -0.5% 0.13 (not significant) wall: 1402.026s ±53.6s +3.5s, +0.3% 0.13 (not significant)

Wondering if preventing new instances of StarlarkSemantics for NOOP case would help reduce the regression even further?

Do you have a flamegraph that can attribute the additional CPU time? We can try with the default semantics, but if that avoids the regression, we should also be able to do better for the path mapping case (we may have missed another cache).

fmeum · 2024-04-10T19:13:43Z

Unfortunately map_each support opens us up for some tricky action key staleness issues (see e.g. https://github.com/bazelbuild/bazel/pull/21954/files#diff-7c8f74b8e12ce2ab6535801dce30a69f90a28c990ceef6780fc4ae457b6b7b9bR3667).

Since Starlark map_each fingerprinting currently works by just evaluating the map_each and hashing the result, we need to ensure that the path mapper used for fingerprinting is the same one used during execution - but that would require knowing whether there are any collisions between stripped paths, which in turn requires flattening a depset.

I don't yet know what to do about this.

fmeum · 2024-04-15T10:35:48Z

@aranguyen I decided to first submit a fix for the action key staleness issue for existing functionality in #21999. When that has been merged and we are happy with the benchmarks here, I will add more logic and tests here to ensure that we don't run into the same kind of issue.

When a command line item is subject to special default stringification behavior (currently, `Label` and `File` instances are), it does not suffice to fingerprint the information that computes the custom stringification; it is also necessary to track whether each individual item is subject to this special formatting. Otherwise, as demonstrated by numerous new test cases, command line items have identical fingerprints to their naive stringification, but may result in distinct command lines. This is fixed by tracking element types via additional UUID markers, either individually or for entire `NestedSet`s based on the `Depset` type.

fmeum · 2024-05-02T15:11:04Z

@bazel-io fork 7.2.0

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

keertk · 2024-05-09T22:20:54Z

Hi all, is this still needed for 7.2?
If so, could you please send a PR to cherry-pick? Looks like there are a bunch of merge conflicts that need to be resolved. We're aiming to have the first RC out on Monday 5/13.

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

fmeum · 2024-05-10T16:00:03Z

@keertk Submitted in #22322

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

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

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

fmeum requested review from a team, brandjon and tetromino as code owners April 10, 2024 08:08

fmeum requested review from aranguyen and removed request for a team, tetromino and brandjon April 10, 2024 08:09

github-actions bot added team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Apr 10, 2024

fmeum force-pushed the fix-semantics branch from 42f51af to 4c8c41a Compare April 10, 2024 08:09

fmeum requested a review from a team as a code owner April 10, 2024 08:09

fmeum added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Apr 10, 2024

fmeum removed the request for review from a team April 10, 2024 08:09

aranguyen reviewed Apr 10, 2024

View reviewed changes

fmeum force-pushed the fix-semantics branch from 4c8c41a to 143fc11 Compare April 17, 2024 07:11

fmeum added 7 commits April 17, 2024 09:11

Address review feedback

55aa804

Address second round of review feedback

4bf2730

Fix NPE

1dbdecb

Reland map_each

eb9ba81

Add tests

013ab20

Fix staleness issues and command line map each adapter impl

1577c25

fmeum force-pushed the fix-semantics branch from 143fc11 to 1577c25 Compare April 17, 2024 09:29

fmeum requested a review from lberki as a code owner April 17, 2024 09:29

Cache ArtifactRoot in PathMapper

0646945

fmeum added 3 commits April 17, 2024 14:09

Improve test

e470a1e

Fix root comparison

9d24bef

Hash additional inputs and optimize semantics

c7e04da

copybara-service bot closed this in d474fcc Apr 30, 2024

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 30, 2024

fmeum deleted the fix-semantics branch May 2, 2024 15:10

bazel-io mentioned this pull request May 2, 2024

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

Closed

fmeum restored the fix-semantics branch May 2, 2024 15:11

iancha1992 mentioned this pull request May 2, 2024

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

Closed

fmeum mentioned this pull request May 10, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically map paths in `Args#map_each` #21952

Automatically map paths in `Args#map_each` #21952

fmeum commented Apr 10, 2024

aranguyen Apr 10, 2024

fmeum Apr 10, 2024

aranguyen Apr 10, 2024

aranguyen Apr 10, 2024

fmeum Apr 10, 2024

fmeum commented Apr 10, 2024

fmeum commented Apr 15, 2024

fmeum commented May 2, 2024

keertk commented May 9, 2024

fmeum commented May 10, 2024

Automatically map paths in Args#map_each #21952

Automatically map paths in Args#map_each #21952

Conversation

fmeum commented Apr 10, 2024

aranguyen Apr 10, 2024

Choose a reason for hiding this comment

fmeum Apr 10, 2024

Choose a reason for hiding this comment

aranguyen Apr 10, 2024

Choose a reason for hiding this comment

aranguyen Apr 10, 2024

Choose a reason for hiding this comment

fmeum Apr 10, 2024

Choose a reason for hiding this comment

fmeum commented Apr 10, 2024

fmeum commented Apr 15, 2024

fmeum commented May 2, 2024

keertk commented May 9, 2024

fmeum commented May 10, 2024

Automatically map paths in `Args#map_each` #21952

Automatically map paths in `Args#map_each` #21952