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

[debuginfo] Emit associated type bindings in trait object type names. #87153

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Jul 15, 2021

This PR updates debuginfo type name generation for trait objects to include associated type bindings and auto trait bounds -- so that, for example, the debuginfo type name of &dyn Iterator<Item=Foo> and &dyn Iterator<Item=Bar> don't both map to just &dyn Iterator anymore.

The following table shows examples of debuginfo type names before and after the PR:

type before after
&dyn Iterator<Item=u32>> &dyn Iterator &dyn Iterator<Item=u32>
&(dyn Iterator<Item=u32>> + Sync) &dyn Iterator &(dyn Iterator<Item=u32> + Sync)
&(dyn SomeTrait<bool, i8, Bar=u32>> + Send) &dyn SomeTrait<bool, i8> &(dyn SomeTrait<bool, i8, Bar=u32>> + Send)

For targets that need C++-like type names, we use assoc$<Item,u32> instead of Item=u32:

type before after
&dyn Iterator<Item=u32>> ref$<dyn$<Iterator> > ref$<dyn$<Iterator<assoc$<Item,u32> > > >
&(dyn Iterator<Item=u32>> + Sync) ref$<dyn$<Iterator> > ref$<dyn$<Iterator<assoc$<Item,u32> >,Sync> >
&(dyn SomeTrait<bool, i8, Bar=u32>> + Send) ref$<dyn$<SomeTrait<bool, i8> > > ref$<dyn$<SomeTrait<bool,i8,assoc$<Bar,u32> > >,Send> >

The PR also adds self-profiling measurements for debuginfo type name generation (re. #86431). It looks like the compiler spends up to 0.5% of its time in that task, so the potential for optimizing it via caching seems limited.

However, the perf run also shows the biggest regression in a test case that does not even invoke the code in question. This suggests that the length of the names we generate here can affect performance by influencing how much data the linker has to copy around.

Fixes #86134.

@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2021
@michaelwoerister
Copy link
Member Author

r? @ghost

@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 15, 2021
@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Trying commit 2e51462552a7c6255d83c22bbc040d63f7b231bb with merge 4ae522214b1d4d553b48707b8400f2b22a2ec4a2...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 15, 2021

💔 Test failed - checks-actions

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2021
@michaelwoerister michaelwoerister force-pushed the debuginfo-names-dyn-trait-projection-bounds branch from 2e51462 to 8fa22dd Compare July 15, 2021 10:23
@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Trying commit 8fa22dd with merge 585e91c718b0b2c5319e1fffd0ff1e62aaf7ccc2...

@bors
Copy link
Contributor

bors commented Jul 15, 2021

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

@rust-timer
Copy link
Collaborator

Queued 585e91c718b0b2c5319e1fffd0ff1e62aaf7ccc2 with parent b919797, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (585e91c718b0b2c5319e1fffd0ff1e62aaf7ccc2): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

  • Moderate regression in instruction counts (up to 1.1% on incr-unchanged builds of tokio-webpush-simple-debug)

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

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 15, 2021
@michaelwoerister michaelwoerister changed the title (wip) [debuginfo] Emit associated type bindings in trait object type names. [debuginfo] Emit associated type bindings in trait object type names. Jul 15, 2021
@bors
Copy link
Contributor

bors commented Jul 19, 2021

⌛ Testing commit 55820a340ce2a8a98d125b113f28a53037f25d69 with merge a1d2ba9f68b4be6d4ed7c2064a0fce42fc7550f9...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 19, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 19, 2021
@michaelwoerister michaelwoerister force-pushed the debuginfo-names-dyn-trait-projection-bounds branch from 55820a3 to 1757542 Compare July 19, 2021 10:39
@michaelwoerister
Copy link
Member Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jul 19, 2021

📌 Commit 17575427abcfcc044dbefb36ac0ab1c3431d9ac9 has been approved by wesleywiser

@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 19, 2021
@bors
Copy link
Contributor

bors commented Jul 19, 2021

⌛ Testing commit 17575427abcfcc044dbefb36ac0ab1c3431d9ac9 with merge c8f87beb0908293dabbbe73d7c7818b556dd891a...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 19, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 19, 2021
@michaelwoerister michaelwoerister force-pushed the debuginfo-names-dyn-trait-projection-bounds branch from 1757542 to 5b1bfae Compare July 19, 2021 14:00
@michaelwoerister
Copy link
Member Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jul 19, 2021

📌 Commit 5b1bfae has been approved by wesleywiser

@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 19, 2021
@bors
Copy link
Contributor

bors commented Jul 19, 2021

⌛ Testing commit 5b1bfae with merge 014026d...

@bors
Copy link
Contributor

bors commented Jul 19, 2021

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 014026d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2021
@bors bors merged commit 014026d into rust-lang:master Jul 19, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 19, 2021
@rylev
Copy link
Member

rylev commented Jul 27, 2021

Given the comments on how one might address this performance issue, and the fact that it doesn't seem to have caused performance regressions after merging, I'm going to add the perf-regression-triaged label so this is not considered an active performance regression.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 27, 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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.

Debuginfo type names for dyn types are incomplete
10 participants