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

Use execution info instead of hard-coded mnemonics for Java path mapping #21093

Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jan 26, 2024

By removing Java rules from the hard-coded mnemonics allowlist for path mapping, users can rely on --modify_execution_info to selectively disable path mapping for them just like for Starlark actions.

This requires fixing two minor inconsistencies in how execution info is populated for Java actions:

  • In JavaCompilationHelper, buildKeepingLast is used instead of buildOrThrow to prevent a crash when a target sets supports-path-mapping via tags.
  • In JavaHeaderCompileAction, --experimental_inmemory_jdeps_files no longer causes all other execution info to be lost.

Fixes #21092

@fmeum fmeum force-pushed the path-mapper-modify-execution-info branch 3 times, most recently from 5831294 to 08333b5 Compare January 26, 2024 22:55
By removing Java rules from the hard-coded mnemonics allowlist for path
mapping, users can rely on `--modify_execution_info` to selectively
disable path mapping for them just like for Starlark actions.
@fmeum fmeum force-pushed the path-mapper-modify-execution-info branch from 08333b5 to c0a5049 Compare January 26, 2024 23:37
@fmeum fmeum marked this pull request as ready for review January 26, 2024 23:37
@fmeum fmeum requested review from a team and lberki as code owners January 26, 2024 23:37
@fmeum fmeum requested review from aranguyen and a team and removed request for a team and lberki January 26, 2024 23:37
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Jan 26, 2024
@fmeum fmeum requested review from hvadehra and removed request for a team and aranguyen January 26, 2024 23:38
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 26, 2024

@hvadehra Could you review this? While the aim is to remove tech debt related to the path mapping feature, the PR almost exclusively touches execution info logic for Java actions.

CC @aranguyen

@hvadehra hvadehra added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 12, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 12, 2024

@bazel-io fork 7.1.0

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 12, 2024
@iancha1992 iancha1992 removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 12, 2024
@iancha1992
Copy link
Member

Just FYI, this is being imported in cl/606319644

cc: @bazelbuild/triage @Wyverald @meteorcloudy

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 21, 2024
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Feb 21, 2024
By removing Java rules from the hard-coded mnemonics allowlist for path mapping, users can rely on `--modify_execution_info` to selectively disable path mapping for them just like for Starlark actions.

This requires fixing two minor inconsistencies in how execution info is populated for Java actions:
* In `JavaCompilationHelper`, `buildKeepingLast` is used instead of `buildOrThrow` to prevent a crash when a target sets `supports-path-mapping` via `tags`.
* In `JavaHeaderCompileAction`, `--experimental_inmemory_jdeps_files` no longer causes all other execution info to be lost.

Fixes bazelbuild#21092

Closes bazelbuild#21093.

PiperOrigin-RevId: 609064092
Change-Id: I6803e34a6861f19d185542e707b00029ee018a0a
github-merge-queue bot pushed a commit that referenced this pull request Feb 21, 2024
…ath mapping (#21461)

By removing Java rules from the hard-coded mnemonics allowlist for path
mapping, users can rely on `--modify_execution_info` to selectively
disable path mapping for them just like for Starlark actions.

This requires fixing two minor inconsistencies in how execution info is
populated for Java actions:
* In `JavaCompilationHelper`, `buildKeepingLast` is used instead of
`buildOrThrow` to prevent a crash when a target sets
`supports-path-mapping` via `tags`.
* In `JavaHeaderCompileAction`, `--experimental_inmemory_jdeps_files` no
longer causes all other execution info to be lost.

Fixes #21092

Closes #21093.

Commit
f8337c7

PiperOrigin-RevId: 609064092
Change-Id: I6803e34a6861f19d185542e707b00029ee018a0a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@fmeum fmeum deleted the path-mapper-modify-execution-info branch February 21, 2024 23:21
@justinhorvitz
Copy link
Contributor

I just traced a memory regression to this change. It only shows up in builds that use --experimental_inmemory_jdeps_files so I think it's actually due to the fix mentioned in the description:

In JavaHeaderCompileAction, --experimental_inmemory_jdeps_files no longer causes all other execution info to be lost.

The magnitude of the regression is about 0.2% on our benchmark java-heavy build. Given that it's actually a fix, we won't roll this back. Instead, I'll keep in mind a couple idea:

  1. Is there a more efficient representation for execution info than ImmutableSortedMap?
  2. For in-memory jdeps, we miss out on the benefit interning the execution info (see SpawnAction constructor) because we put a unique value in the map ( outputDepsProto.getExecPathString()). We can either find a way to avoid the unique entry, or just skip interning, since it has memory overhead without the benefit.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 12, 2024

Good find, I didn't think of that.

I'm not aware of any other execution info entry with a unique value. As a quick win, we could just not inline infos containing this particular key in the SpawnAction constructor.

Maybe a custom SortedMap implementation that stores the unique part separately from the interned constant part could improve the savings further?

@justinhorvitz
Copy link
Contributor

I have https://bazel-review.googlesource.com/c/bazel/+/242796 out for review, which reduces memory on the internal benchmark target by 1.0%. So it ends up even better than before your PR.

There might be more savings to be had by using something other than a map, but it also might be negligible so long as interning is effective.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 12, 2024

@bazel-io fork 7.1.1

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 12, 2024

@iancha1992 Forking again for the fix in #21093 (comment).

@justinhorvitz
Copy link
Contributor

Memory improvement submitted as e8d844e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--experimental_output_paths=strip cannot be disabled for built-in mnemonics
5 participants