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

Share classpath NestedSet between full and header compile actions #21343

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 14, 2024

The Starlark Java rules already add the direct JARs to the transitive classpath depset and pass the same depset to the full and the header compile action. By allowing them to bypass the logic that prepends the direct jars to the compile-time classpath, both actions retain the same NestedSet instead of both retaining a new one with identical elements.

The Starlark Java rules already add the direct JARs to the transitive
classpath depset and pass the same depset to the full and the header
compile action. By allowing them to bypass the logic that prepends the
direct jars to the compile-time classpath, both actions retain the same
`NestedSet` instead of both retaining a new one with identical elements.
@fmeum fmeum force-pushed the 19872-deduplicate-nested-sets branch from ad0fc99 to 284d904 Compare February 14, 2024 13:36
@fmeum fmeum changed the title Share full classpath NestedSet between full and header compile actions Share classpath NestedSet between full and header compile actions Feb 14, 2024
@fmeum fmeum requested a review from hvadehra February 14, 2024 14:23
@fmeum fmeum marked this pull request as ready for review February 14, 2024 14:23
@fmeum fmeum requested a review from lberki as a code owner February 14, 2024 14:23
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 14, 2024

Cc @justinhorvitz

@github-actions github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Feb 14, 2024
@fmeum fmeum removed the request for review from lberki February 14, 2024 19:02
@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 15, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 15, 2024

@hvadehra If that information is easy to get, I would be very interested in learning how much memory this saves to help build up my intuition.

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 15, 2024

@bazel-io fork 7.1.0

@hvadehra
Copy link
Member

@hvadehra If that information is easy to get, I would be very interested in learning how much memory this saves to help build up my intuition.

Sure, I'll report back shortly.

@hvadehra
Copy link
Member

Going by the numbers in #19872 (comment), looks like this saves ~20 bytes / javac action

@justinhorvitz
Copy link
Contributor

Confirming, it's about 4MB (0.1%) for the benchmark target from #19872 (comment).

@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 16, 2024
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Feb 16, 2024
The Starlark Java rules already add the direct JARs to the transitive classpath depset and pass the same depset to the full and the header compile action. By allowing them to bypass the logic that prepends the direct jars to the compile-time classpath, both actions retain the same `NestedSet` instead of both retaining a new one with identical elements.

Closes bazelbuild#21343.

PiperOrigin-RevId: 607790056
Change-Id: Ia08c834ae3e5b1151607459408cdfea85d47314f
@fmeum fmeum deleted the 19872-deduplicate-nested-sets branch February 16, 2024 22:34
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2024
…ctions (#21389)

The Starlark Java rules already add the direct JARs to the transitive
classpath depset and pass the same depset to the full and the header
compile action. By allowing them to bypass the logic that prepends the
direct jars to the compile-time classpath, both actions retain the same
`NestedSet` instead of both retaining a new one with identical elements.

Closes #21343.

Commit
31fae9e

PiperOrigin-RevId: 607790056
Change-Id: Ia08c834ae3e5b1151607459408cdfea85d47314f

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@iancha1992
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants