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

avoid computing liveness for locals that escape into statics #52991

Merged
merged 7 commits into from
Aug 5, 2018

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 2, 2018

Fixes #52713

I poked at this on the plane and I think it's working -- but I want to do a bit more investigation and double check. The idea is to identify those local variables where the entire value will "escape" into the return -- for them, we don't need to compute liveness, since we know that the outlives relations from the return type will force those regions to be equal to free regions. This should help with html5ever in particular.

  • test performance
  • verify correctness
  • add comments

r? @pnkfelix
cc @lqd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2018
@nikomatsakis nikomatsakis force-pushed the nll-escaping-into-return branch from 2de5658 to 341a07c Compare August 2, 2018 19:03
@nikomatsakis
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 2, 2018

⌛ Trying commit 341a07c with merge da90b26...

bors added a commit that referenced this pull request Aug 2, 2018
[WIP] avoid computing liveness for locals that escape into statics

Fixes #52713

I poked at this on the plane and I think it's working -- but I want to do a bit more investigation and double check. The idea is to identify those local variables where the entire value will "escape" into the return -- for them, we don't need to compute liveness, since we know that the outlives relations from the return type will force those regions to be equal to free regions. This should help with html5ever in particular.

r? @pnkfelix
cc @lqd
@bors
Copy link
Contributor

bors commented Aug 2, 2018

☀️ Test successful - status-travis
State: approved= try=True

@nikomatsakis
Copy link
Contributor Author

@rust-timer build da90b26

@rust-timer
Copy link
Collaborator

Success: Queued da90b26 with parent 03da14b, comparison URL.

@nikomatsakis
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 3, 2018

⌛ Trying commit 75504ee with merge 24a313ef05eff74c552335159aa02fbbfcf7e8b3...

@nikomatsakis
Copy link
Contributor Author

without support for X = &*Y reborrows, this won't work so well :)

@nikomatsakis
Copy link
Contributor Author

@rust-timer build 24a313ef05eff74c552335159aa02fbbfcf7e8b3

@rust-timer
Copy link
Collaborator

Success: Queued 24a313ef05eff74c552335159aa02fbbfcf7e8b3 with parent 7e8ca9f, comparison URL.

@bors
Copy link
Contributor

bors commented Aug 3, 2018

☀️ Test successful - status-travis
State: approved= try=True

@nikomatsakis
Copy link
Contributor Author

@rust-timer build 24a313ef05eff74c552335159aa02fbbfcf7e8b3

@rust-timer
Copy link
Collaborator

Success: Queued 24a313ef05eff74c552335159aa02fbbfcf7e8b3 with parent 7e8ca9f, comparison URL.

@nikomatsakis
Copy link
Contributor Author

The comparison URL doesn't work yet but you can view the NLL dashboard results for 24a313e here:

https://perf.rust-lang.org/nll-dashboard.html?commit=24a313ef05eff74c552335159aa02fbbfcf7e8b3&stat=instructions%3Au

html5ever 11,036,800,000.00 241,127,686,144.00 2,184.76
tuple-stress 24,313,452,544.00 69,815,214,080.00 287.15
keccak 47,153,451,008.00 85,099,872,256.00 180.47
clap-rs 21,144,303,616.00 30,054,019,072.00 142.14
sentry-cli 12,483,297,280.00 15,415,211,008.00 123.49
inflate 13,858,590,720.00 16,881,270,784.00 121.81

versus

html5ever 11,049,374,720.00 359,718,027,264.00 3,255.55
tuple-stress 24,456,257,536.00 69,911,584,768.00 285.86
keccak 47,174,316,032.00 85,679,489,024.00 181.62
clap-rs 21,200,508,928.00 31,260,983,296.00 147.45
inflate 13,863,846,912.00 17,317,126,144.00 124.91

@nikomatsakis
Copy link
Contributor Author

Seems to be a good win on html5ever, as predicted =)

@nikomatsakis nikomatsakis changed the title [WIP] avoid computing liveness for locals that escape into statics avoid computing liveness for locals that escape into statics Aug 4, 2018
@nikomatsakis
Copy link
Contributor Author

@pnkfelix ready for review now I think

@pnkfelix
Copy link
Member

pnkfelix commented Aug 4, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2018

📌 Commit 5e2f337 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Aug 4, 2018

🌲 The tree is currently closed for pull requests below priority 12, this pull request will be tested once the tree is reopened

@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 Aug 4, 2018
@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm
Copy link
Member

kennytm commented Aug 4, 2018

@bors r-

[00:13:14] �[0m�[1m�[38;5;9merror�[0m�[0m�[1m: field is never used: `tcx`�[0m
[00:13:14] �[0m  �[0m�[0m�[1m�[38;5;12m--> �[0m�[0mlibrustc_mir/borrow_check/nll/escaping_locals.rs:79:5�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m|�[0m
[00:13:14] �[0m�[1m�[38;5;12m79�[0m�[0m �[0m�[0m�[1m�[38;5;12m| �[0m�[0m    tcx: TyCtxt<'cx, 'gcx, 'tcx>,�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m| �[0m�[0m    �[0m�[0m�[1m�[38;5;9m^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m|�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m= �[0m�[0m�[1mnote�[0m�[0m: `-D dead-code` implied by `-D warnings`�[0m
[00:13:14] 
[00:13:14] �[0m�[1m�[38;5;9merror�[0m�[0m�[1m: field is never used: `mir`�[0m
[00:13:14] �[0m  �[0m�[0m�[1m�[38;5;12m--> �[0m�[0mlibrustc_mir/borrow_check/nll/escaping_locals.rs:80:5�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m|�[0m
[00:13:14] �[0m�[1m�[38;5;12m80�[0m�[0m �[0m�[0m�[1m�[38;5;12m| �[0m�[0m    mir: &'cx Mir<'tcx>,�[0m
[00:13:14] �[0m   �[0m�[0m�[1m�[38;5;12m| �[0m�[0m    �[0m�[0m�[1m�[38;5;9m^^^^^^^^^^^^^^^^^^^�[0m
[00:13:14] 
[00:13:14] �[0m�[1m�[38;5;9merror�[0m�[0m�[1m: aborting due to 2 previous errors�[0m
[00:13:14] 
[00:13:14] �[0m�[0m�[1m�[31merror:�[0m Could not compile `rustc_mir`.
[00:13:14] 

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 4, 2018
@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Aug 5, 2018

📌 Commit 2e2ea26 has been approved by pnkfelix

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2018
@nikomatsakis
Copy link
Contributor Author

@bors p=1 -- NLL is Rust 2018 critical and this is a perf building block.

@bors
Copy link
Contributor

bors commented Aug 5, 2018

⌛ Testing commit 2e2ea26 with merge b47c314...

bors added a commit that referenced this pull request Aug 5, 2018
avoid computing liveness for locals that escape into statics

Fixes #52713

I poked at this on the plane and I think it's working -- but I want to do a bit more investigation and double check. The idea is to identify those local variables where the entire value will "escape" into the return -- for them, we don't need to compute liveness, since we know that the outlives relations from the return type will force those regions to be equal to free regions. This should help with html5ever in particular.

- [x] test performance
- [x] verify correctness
- [x] add comments

r? @pnkfelix
cc @lqd
@bors
Copy link
Contributor

bors commented Aug 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing b47c314 to master...

@bors bors merged commit 2e2ea26 into rust-lang:master Aug 5, 2018
@nikomatsakis nikomatsakis deleted the nll-escaping-into-return branch August 6, 2018 10:12
@nikomatsakis nikomatsakis mentioned this pull request Aug 6, 2018
nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Aug 6, 2018
bors added a commit that referenced this pull request Aug 7, 2018
…r=nikomatsakis

revert #52991

Reverts #52991 which is flawed. I have an idea how to fix it but might as well revert first since it is so wildly flawed. That's what I get for opening PRs while on PTO =)

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants