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

2229: Reduce the size of closures with capture_disjoint_fields #86701

Merged
merged 3 commits into from
Jul 9, 2021

Conversation

arora-aman
Copy link
Member

One key observation while going over the closure size profile of rustc
was that we are disjointly capturing one or more fields starting at an
immutable reference.

Disjoint capture over immutable reference doesn't add too much value
because the fields can either be borrowed immutably or copied.

One possible edge case of the optimization is when a fields of a struct
have a longer lifetime than the structure, therefore we can't completely
get rid of all the accesses on top of sharef refs, only the rightmost
one. Here is a possible example:

struct MyStruct<'a> {
   a: &'static A,
   b: B,
   c: C<'a>,
}

fn foo<'a, 'b>(m: &'a MyStruct<'b>) -> impl FnMut() + 'static {
    let c = || drop(&*m.a.field_of_a);
    // Here we really do want to capture `*m.a` because that outlives `'static`

    // If we capture `m`, then the closure no longer outlives `'static'
    // it is constrained to `'a`
}

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2021
@arora-aman
Copy link
Member Author

arora-aman commented Jun 28, 2021

I profile the change in closure size before and after the change on the rust compiler:

Baseline: https://docs.google.com/spreadsheets/d/1U5vaXu490wn8Ae1LB1mSfoP_EZrGRHbUU6ADbeY8bn4/edit#gid=545238654
After optimization:
https://docs.google.com/spreadsheets/d/1zOOeTFz4AvprEVsiPNFlkt4x_93N7WQe2uv2pX3BzXQ/edit#gid=545238654

Some details about the sheets:

  1. First sheet is data that was collected and then we compute the change in closure size and % change in closure size
  2. Second sheet is metrics computed based on sheet 1 over different ranges of the original pre-feature closure size
  3. Third sheet is a chart the plots %change of each closure in the profiling data

I had to output the data to stdout to get data from each dependency and crate that is compiled and ended up making a one off error while filtering out the boostrap output.

I have another PR that allows profiling using a compiler flag: #86695

@@ -0,0 +1,31 @@
#![feature(capture_disjoint_fields)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waiting on the editon PR to go in for this to work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To change this to use edition*

@arora-aman
Copy link
Member Author

Notes from 2229 sync: https://hackmd.io/hpjgfcwaSK20uFtZibwhIw

@bors
Copy link
Contributor

bors commented Jun 29, 2021

☔ The latest upstream changes (presumably #86704) made this pull request unmergeable. Please resolve the merge conflicts.

One key observation while going over the closure size profile of rustc
was that we are disjointly capturing one or more fields starting at an
immutable reference.

Disjoint capture over immutable reference doesn't add too much value
because the fields can either be borrowed immutably or copied.

One possible edge case of the optimization is when a fields of a struct
have a longer lifetime than the structure, therefore we can't completely
get rid of all the accesses on top of sharef refs, only the rightmost
one. Here is a possible example:

```rust
struct MyStruct<'a> {
   a: &'static A,
   b: B,
   c: C<'a>,
}

fn foo<'a, 'b>(m: &'a MyStruct<'b>) -> impl FnMut() + 'static {
    let c = || drop(&*m.a.field_of_a);
    // Here we really do want to capture `*m.a` because that outlives `'static`

    // If we capture `m`, then the closure no longer outlives `'static'
    // it is constrained to `'a`
}
```
@nikomatsakis
Copy link
Contributor

@bors delegate+

r=me with comments added

@bors
Copy link
Contributor

bors commented Jul 5, 2021

✌️ @arora-aman can now approve this pull request

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 7, 2021

📌 Commit 38dcae2 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2021
@bors
Copy link
Contributor

bors commented Jul 9, 2021

⌛ Testing commit 38dcae2 with merge fdfe819...

@bors
Copy link
Contributor

bors commented Jul 9, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing fdfe819 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2021
@bors bors merged commit fdfe819 into rust-lang:master Jul 9, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants