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.

Sign up for GitHub

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

Jump to bottom

[Don't merge] Perf test for using closure precise capture #79696

Closed
wants to merge 20 commits into from
Closed

[Don't merge] Perf test for using closure precise capture #79696

wants to merge 20 commits into from

Conversation

arora-aman
Copy link
Member

@arora-aman arora-aman commented Dec 4, 2020 •
edited
Loading

Enables capture_disjoint_fields by default. This PR includes a couple of hacks to get around know bugs when the feature is enabled. Most failing tests required UI changes or are known issues and it didn't make sense to update all of them just for the perf run.

r? @ghost

@arora-aman arora-aman changed the title Perf test for using closure min capture [Don't merge] Perf test for using closure min capture Dec 4, 2020
@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 4, 2020

⌛ Trying commit 009113acaec7512b6913d43abcdfa0d9cbdca8ce with merge 6fe642cd1853de8dbc94a494cc72f3f9c0d32899...

@bors
Copy link
Contributor

bors commented Dec 4, 2020

💔 Test failed - checks-actions

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 4, 2020
@bjorn3
Copy link
Member

bjorn3 commented Dec 5, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 5, 2020

⌛ Trying commit e77ca454427a1c3a54ec47def6f44c339e8ff554 with merge 06e8ed264fa6db879bda64c2bc3b2215911a1b7c...

@bors
Copy link
Contributor

bors commented Dec 5, 2020

💔 Test failed - checks-actions

@bors
Copy link
Contributor

bors commented Dec 5, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

arora-aman and others added 17 commits December 5, 2020 15:44
@arora-aman @jenniferwills @logmosier
Writeback min_capture map to TypeckResults
c7b4a08 
- Derive TypeFoldable on `hir::place::Place` and associated
  structs, to them to be written into typeck results.

Co-authored-by: Jennifer Wills <wills.jenniferg@gmail.com>
Co-authored-by: Logan Mosier <logmosier@gmail.com>
@arora-aman @roxelo
Use min_captures for creating UpvarSusbts::tupled_upvar_tys
3c3900b 
- final_upvar_tys now reads types from places instead of using `node_ty`

Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
@arora-aman @roxelo
Move handling UpvarRef to PlaceBuilder
b57d7e6 
- This allows us to delay figuring out the index of a capture
  in the closure structure when all projections to atleast form
  a capture have been applied to the builder

Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
@arora-aman @roxelo
Use Places for captures in MIR
5e2bbb4 
- Use closure_min_capture maps to capture precise paths
- PlaceBuilder now searches for ancestors in min_capture list
- Add API to `Ty` to allow access to the n-th element in a
  tuple in O(1) time.

Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
@arora-aman @roxelo
Use precise places when lowering Closures in THIR
a1122f8 
- Closures now use closure_min_captures to figure out captured paths
- Build upvar_mutbls using closure_min_captures
- Change logic in limit_capture_mutability to differentiate b/w
  capturing parent's local variable or capturing a variable that is
  captured by the parent (in case of nested closure) using PlaceBase.

Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
@arora-aman
Use closure_min_captures in borrow checker
4308846 
- Use closure_min_captures to generate the Upvar structure that
  stores information for diagnostics and information about
  mutability of captures.
@arora-aman
Run pass test cases for RFC 2229
2bdc568
@arora-aman
Compute mutability of closure captures
fae4d2e 
- Store this in `ty::CapturedPlace`
- Use `ty::CapturedPlace::mutability` to in mir_build and borrow_check
@arora-aman
Truncate projections of captured place
549f78a 
- No Derefs in move closure, this will result in value behind a reference getting moved.
- No projections are applied to raw pointers, since these require unsafe blocks. We capture
  them completely.
- No Index projections are captured, since arrays are captured completely.
@arora-aman
Test bless
e2927ee
@arora-aman
Remove failing tests :p
4ff6388
@arora-aman
Enable feature
76488e7
@arora-aman
Make the compiler compile 😅
8c64f01
@arora-aman
Remove more failing tests :p
a88fb4a
@arora-aman
error code doc change
69cba8c
@arora-aman
Remove it to be safe
e2c96e9
@arora-aman
Rust analyzer migration for 2229
fda70bd
@arora-aman arora-aman changed the title [Don't merge] Perf test for using closure min capture [Don't merge] Perf test for using closure precise capture Dec 5, 2020
@bjorn3
Copy link
Member

bjorn3 commented Dec 5, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 5, 2020

⌛ Trying commit 20692cb with merge d5c47437deb5a77afed53e0bba2a050fbeaf974c...

@bors
Copy link
Contributor

bors commented Dec 5, 2020

☀️ Try build successful - checks-actions
Build commit: d5c47437deb5a77afed53e0bba2a050fbeaf974c (d5c47437deb5a77afed53e0bba2a050fbeaf974c)

@rust-timer
Copy link
Collaborator

Queued d5c47437deb5a77afed53e0bba2a050fbeaf974c with parent 15eaa00, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d5c47437deb5a77afed53e0bba2a050fbeaf974c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2020
@arora-aman
Copy link
Member Author

^ This was feature enabled + mutability fix + move closures drop deref captures

@lqd
Copy link
Member

lqd commented Dec 6, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 6, 2020

⌛ Trying commit 015c6a2 with merge aee064ad665521e03371fbb810437a813fbdd365...

@bors
Copy link
Contributor

bors commented Dec 6, 2020

☀️ Try build successful - checks-actions
Build commit: aee064ad665521e03371fbb810437a813fbdd365 (aee064ad665521e03371fbb810437a813fbdd365)

@rust-timer
Copy link
Collaborator

Queued aee064ad665521e03371fbb810437a813fbdd365 with parent 0f6f2d6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (aee064ad665521e03371fbb810437a813fbdd365): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@arora-aman
Copy link
Member Author

right before this commit was feature enabled + mutability fix + all moves drop derefs

@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Trying commit acd732a with merge 80f976ebf93cfeeb1f52bbd43eedb03b4568b8bc...

@bors
Copy link
Contributor

bors commented Dec 7, 2020

☀️ Try build successful - checks-actions
Build commit: 80f976ebf93cfeeb1f52bbd43eedb03b4568b8bc (80f976ebf93cfeeb1f52bbd43eedb03b4568b8bc)

@rust-timer
Copy link
Collaborator

Queued 80f976ebf93cfeeb1f52bbd43eedb03b4568b8bc with parent 0f6f2d6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (80f976ebf93cfeeb1f52bbd43eedb03b4568b8bc): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@arora-aman
Copy link
Member Author

I'm done experimenting :)

@arora-aman arora-aman closed this Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@arora-aman @wesleywiser @rust-timer @bors @bjorn3 @lqd @rustbot