Name the captured upvars for closures/generators in debuginfo #85020

lrh2000 · 2021-05-07T04:02:58Z

Previously, debuggers print closures as something like

y::main::closure-0 (0x7fffffffdd34)

The pointer actually references to an upvar. It is not very obvious, especially for beginners.

It's because upvars don't have names before, as they are packed into a tuple. This PR names the upvars, so we can expect to see something like

y::main::closure-0 {_captured_ref__b: 0x[...]}

r? @tmandry
Discussed at #84752 (comment) .

tmandry

Looks good! I left a few comments.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

compiler/rustc_middle/src/ty/closure.rs

src/test/debuginfo/captured-fields.rs

compiler/rustc_middle/src/ty/closure.rs

lrh2000 · 2021-05-15T15:57:04Z

I haven't split closures from TupleMemberDescriptionFactory because now generators are also mixed in EnumMemberDescriptionFactory. I guess a full refactor of the code may split generators from EnumMemberDescriptionFactory as well?

I have addressed other comments. Besides, now upvar names are stored in mir::Body::var_debug_info. So they can be referenced at breakpoints inside the closure (as shown by a new test case). And it solves issues against external closures (#85020 (comment)), but there are not test cases since I don't think our test system can test multiple crates (right?).

tmandry · 2021-06-16T01:07:53Z

Looks good, thanks. Building a new symbol for every capture in mir-build could impact perf, so

@bors try
@rust-timer queue

rust-timer · 2021-06-16T01:07:54Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-06-16T01:08:04Z

⌛ Trying commit fc900b3a86b03476941f4a4b81ab4bfd7000a62b with merge 572ac0ed693592bcc032c44e9272d9dd52db6385...

bors · 2021-06-16T01:56:08Z

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

rust-timer · 2021-06-16T01:56:09Z

Queued 572ac0ed693592bcc032c44e9272d9dd52db6385 with parent 607d6b0, future comparison URL.

rust-timer · 2021-06-16T04:41:22Z

Finished benchmarking try commit (572ac0ed693592bcc032c44e9272d9dd52db6385): 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 label: +S-waiting-on-review -S-waiting-on-perf

tmandry · 2021-06-16T19:08:54Z

A couple of incremental configurations regressed. I'd be willing to land the change with this if we had no clear path to improving the results, but I suspect that making to_symbol a query for in-memory caching would make a significant improvement for cases where the same MIR gets monomorphized many times.

@lrh2000 Would you be able to make to_symbol wrap a query? If not I can. We can do another perf run then and compare.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

tmandry · 2021-06-16T19:17:45Z

The code comments I just left should also have a significant impact. EDIT: I'm wondering if we should try those before adding the complexity of a new query, which can also have some negative effects on perf.

lrh2000 · 2021-06-19T15:55:54Z

@lrh2000 Would you be able to make to_symbol wrap a query? If not I can. We can do another perf run then and compare.

Thanks for your review. Unfortunately, I've got a very busy June, so I do not have time to address the comments before early July. But once I have some free time, I am willing to address them.

Sorry for the delay. (And if you want to push some new commits, feel free to do that.)

Previously, debuggers print closures as something like ``` y::main::closure-0 (0x7fffffffdd34) ``` The pointer actually references to an upvar. It is not very obvious, especially for beginners. It's because upvars don't have names before, as they are packed into a tuple. This commit names the upvars, so we can expect to see something like ``` y::main::closure-0 {_captured_ref__b: 0x[...]} ```

- Closures in external crates may get compiled in because of monomorphization. We should store names of captured variables in `optimized_mir`, so that they are written into the metadata file and we can use them to generate debuginfo. - If there are breakpoints inside closures, the names of captured variables stored in `optimized_mir` can be used to print them. Now the name is more precise when disjoint fields are captured.

lrh2000 · 2021-07-28T15:52:55Z

@rustbot ready
I think now we should check the pref results again?

rustbot · 2021-07-28T15:52:57Z

@tmandry

tmandry · 2021-08-07T00:46:23Z

@bors try
@rust-timer queue

rust-timer · 2021-08-07T00:46:25Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-07T00:46:33Z

⌛ Trying commit cf5eda1 with merge f262e7955412a89f47cbb17eb9b2d1332e30a8fa...

bors · 2021-08-07T01:27:16Z

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

rust-timer · 2021-08-07T01:27:17Z

Queued f262e7955412a89f47cbb17eb9b2d1332e30a8fa with parent 5ad7389, future comparison URL.

tmandry · 2021-08-14T02:06:07Z

Not sure what happened.
@rust-timer queue

tmandry · 2021-08-14T02:13:27Z

@rust-timer build f262e7955412a89f47cbb17eb9b2d1332e30a8fa

rust-timer · 2021-08-14T02:13:28Z

Queued f262e7955412a89f47cbb17eb9b2d1332e30a8fa with parent 5ad7389, future comparison URL.

tmandry · 2021-08-14T02:28:04Z

Code looks good, just awaiting perf results!

rust-timer · 2021-08-14T04:33:05Z

Finished benchmarking try commit (f262e7955412a89f47cbb17eb9b2d1332e30a8fa): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -0.6% on incr-unchanged builds of coercions)
Large regression in instruction counts (up to 4.7% on incr-patched: println builds of webrender-wrench)

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.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

tmandry · 2021-08-14T05:09:19Z

So adding the query did improve things quite a bit overall, though not by much on the biggest regression. I'm a little surprised and concerned about why that one benchmark regresses so much, but debuginfo can be huge so maybe it's not that surprising. And there are many more (small) improvements than regressions now.

This does improve debuginfo significantly so I'd say the regression is probably justified.

@rustbot label: +perf-regression-triaged
@bors r+

Thanks for your work on this!

bors · 2021-08-14T05:09:20Z

📌 Commit cf5eda1 has been approved by tmandry

bors · 2021-08-14T07:01:43Z

⌛ Testing commit cf5eda1 with merge 99efc51...

bors · 2021-08-14T09:25:28Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 99efc51 to master...

rust-highfive assigned tmandry May 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2021

lrh2000 force-pushed the named-upvars branch 2 times, most recently from 2f7b6b7 to 7a0bd3d Compare May 7, 2021 16:12

tmandry reviewed May 14, 2021

View reviewed changes

lrh2000 force-pushed the named-upvars branch from 7a0bd3d to fc900b3 Compare May 15, 2021 11:18

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2021

tmandry reviewed Jun 16, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved

tmandry reviewed Jun 16, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Show resolved Hide resolved

jackh726 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2021

lrh2000 added 3 commits July 9, 2021 23:06

Avoid unnecessary String::clone

0cb6f07

lrh2000 force-pushed the named-upvars branch from d3ea255 to 99bb376 Compare July 9, 2021 15:46

Add a query for CapturedPlace::to_symbol

cf5eda1

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 7, 2021

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 14, 2021

rustbot added the perf-regression-triaged The performance regression has been triaged. label Aug 14, 2021

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 14, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 14, 2021

bors merged commit 99efc51 into rust-lang:master Aug 14, 2021

rustbot added this to the 1.56.0 milestone Aug 14, 2021

dtolnay mentioned this pull request Aug 15, 2021

LLVM crash: Basic Block does not have terminator! #88043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name the captured upvars for closures/generators in debuginfo #85020

Name the captured upvars for closures/generators in debuginfo #85020

lrh2000 commented May 7, 2021

tmandry left a comment

lrh2000 commented May 15, 2021

tmandry commented Jun 16, 2021

rust-timer commented Jun 16, 2021

bors commented Jun 16, 2021

bors commented Jun 16, 2021

rust-timer commented Jun 16, 2021

rust-timer commented Jun 16, 2021

tmandry commented Jun 16, 2021 •

edited

Loading

tmandry commented Jun 16, 2021 •

edited

Loading

lrh2000 commented Jun 19, 2021

lrh2000 commented Jul 28, 2021

rustbot commented Jul 28, 2021

tmandry commented Aug 7, 2021

rust-timer commented Aug 7, 2021

bors commented Aug 7, 2021

bors commented Aug 7, 2021

rust-timer commented Aug 7, 2021

tmandry commented Aug 14, 2021

This comment has been minimized.

tmandry commented Aug 14, 2021

rust-timer commented Aug 14, 2021

tmandry commented Aug 14, 2021

rust-timer commented Aug 14, 2021

tmandry commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

Name the captured upvars for closures/generators in debuginfo #85020

Name the captured upvars for closures/generators in debuginfo #85020

Conversation

lrh2000 commented May 7, 2021

tmandry left a comment

Choose a reason for hiding this comment

lrh2000 commented May 15, 2021

tmandry commented Jun 16, 2021

rust-timer commented Jun 16, 2021

bors commented Jun 16, 2021

bors commented Jun 16, 2021

rust-timer commented Jun 16, 2021

rust-timer commented Jun 16, 2021

tmandry commented Jun 16, 2021 • edited Loading

tmandry commented Jun 16, 2021 • edited Loading

lrh2000 commented Jun 19, 2021

lrh2000 commented Jul 28, 2021

rustbot commented Jul 28, 2021

tmandry commented Aug 7, 2021

rust-timer commented Aug 7, 2021

bors commented Aug 7, 2021

bors commented Aug 7, 2021

rust-timer commented Aug 7, 2021

tmandry commented Aug 14, 2021

This comment has been minimized.

tmandry commented Aug 14, 2021

rust-timer commented Aug 14, 2021

tmandry commented Aug 14, 2021

rust-timer commented Aug 14, 2021

tmandry commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

tmandry commented Jun 16, 2021 •

edited

Loading

tmandry commented Jun 16, 2021 •

edited

Loading