-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Closure capture cleanup & refactor #89861
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 691f14023ffcb4e6591655228cdc358b00f282b9 with merge 141f36eb94d72f67dfd9c393c50026f544b48579... |
@rust-timer build 141f36eb94d72f67dfd9c393c50026f544b48579 |
Queued 141f36eb94d72f67dfd9c393c50026f544b48579 with parent eeb16a2, future comparison URL. |
Finished benchmarking commit (141f36eb94d72f67dfd9c393c50026f544b48579): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
Rebased to address conflict with #89933 since both introduces some use of let_else. Weird that bors didn't drop a message this time. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #91945) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #92099) made this pull request unmergeable. Please resolve the merge conflicts. |
This span is unused and is superseded by capture_kind_expr_id in CaptureInfo
Region info is completely unnecessary for upvar capture kind computation and is only needed to create the final upvar tuple ty. Doing so makes creation of UpvarCapture very cheap and expose further cleanup opportunity.
`adjust_upvar_deref` and friends are implemented so that they reuse existing region so new region vars don't have to be generated. Since now we don't have to generate region vars in `InferBorrowKind`, creating a new capture info would be cheap and we can just use `determine_capture_info`. syn is updated so that let_else syntax can be used in fn with `#[instrument]` attribute. `restrict_repr_packed_field_ref_capture` is changed to take place by value since cloning is needed anyway.
Min capture computation can already handle the same place appearing twice, and previous commits made CaptureInfo construction very cheap, so just delegate all work to min capture and let InferBorrowKind and process_collected_capture_information handle everything linearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in review, this looks really great!
@bors r+ |
📌 Commit c84cea9 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (22e491a): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Closure capture cleanup & refactor Follow up of rust-lang#89648 Each commit is self-contained and the rationale/changes are documented in the commit message, so it's advisable to review commit by commit. The code is significantly cleaner (at least IMO), but that could have some perf implication, so I'd suggest a perf run. r? `@wesleywiser` cc `@arora-aman`
Follow up of #89648
Each commit is self-contained and the rationale/changes are documented in the commit message, so it's advisable to review commit by commit.
The code is significantly cleaner (at least IMO), but that could have some perf implication, so I'd suggest a perf run.
r? @wesleywiser
cc @arora-aman