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

Also apply NestedSet optimizations to Depset #19379

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 31, 2023

If the construction of a NestedSet involves only a single non-empty transitive set, this set may be returned directly as an optimization. This commit checks for this case when constructing a Depset and reuses the corresponding Depset if possible.

Since Depsets that are direct elements of providers are usually unwrapped into their NestedSet, this optimization only applies to instances stored in structs as well as reduces allocations.

Realizes an optimization proposed by @brentleyjones in https://bazelbuild.slack.com/archives/CA31HN1T3/p1693487067454409?thread_ts=1693484609.534579&cid=CA31HN1T3.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 31, 2023
@fmeum fmeum requested a review from comius August 31, 2023 14:52
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 31, 2023

@comius Could you review this? It's a memory optimization of the kind you have been doing a lot of recently.

@iancha1992 iancha1992 added team-Rules-Java Issues for Java rules team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Rules-Java Issues for Java rules labels Sep 1, 2023
@comius comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 6, 2023
@fmeum fmeum requested a review from comius September 6, 2023 10:55
@brentleyjones brentleyjones added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 6, 2023
@comius comius 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 Sep 6, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 6, 2023

@bazel-io flag

@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 Sep 6, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 6, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 6, 2023

@comius Do you want me to resolve the merge conflicts or are you doing that in the imported change?

@comius
Copy link
Contributor

comius commented Sep 6, 2023

@comius Do you want me to resolve the merge conflicts or are you doing that in the imported change?

I’m not doing that. If you can resolve the conflicts that’s welcome. Thanks

If the construction of a `NestedSet` involves only a single non-empty
transitive set, this set may be returned directly as an optimization.
This commit checks for this case when constructing a `Depset` and reuses
the corresponding `Depset` if possible.
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 7, 2023

@comius Done

@fmeum fmeum requested a review from comius September 7, 2023 05:05
@Pavank1992 Pavank1992 added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Sep 8, 2023
@copybara-service copybara-service bot closed this in 216fce5 Sep 8, 2023
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 8, 2023
@fmeum fmeum deleted the optimize-depset branch September 10, 2023 08:59
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Sep 11, 2023
If the construction of a `NestedSet` involves only a single non-empty transitive set, this set may be returned directly as an optimization. This commit checks for this case when constructing a `Depset` and reuses the corresponding `Depset` if possible.

Since `Depset`s that are direct elements of providers are usually unwrapped into their `NestedSet`, this optimization only applies to instances stored in `struct`s as well as reduces allocations.

Realizes an optimization proposed by @brentleyjones in https://bazelbuild.slack.com/archives/CA31HN1T3/p1693487067454409?thread_ts=1693484609.534579&cid=CA31HN1T3.

Closes bazelbuild#19379.

PiperOrigin-RevId: 563679197
Change-Id: I542bbf56784832768ca3bbbd550cc78bfb981ffe
iancha1992 added a commit that referenced this pull request Sep 14, 2023
If the construction of a `NestedSet` involves only a single non-empty
transitive set, this set may be returned directly as an optimization.
This commit checks for this case when constructing a `Depset` and reuses
the corresponding `Depset` if possible.

Since `Depset`s that are direct elements of providers are usually
unwrapped into their `NestedSet`, this optimization only applies to
instances stored in `struct`s as well as reduces allocations.

Realizes an optimization proposed by @brentleyjones in
https://bazelbuild.slack.com/archives/CA31HN1T3/p1693487067454409?thread_ts=1693484609.534579&cid=CA31HN1T3.

Closes #19379.

Commit
216fce5

PiperOrigin-RevId: 563679197
Change-Id: I542bbf56784832768ca3bbbd550cc78bfb981ffe

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.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-API API for writing rules/aspects: providers, runfiles, actions, artifacts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants