rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`. by eddyb · Pull Request #59343 · rust-lang/rust

eddyb · 2019-03-21T16:33:29Z

The def_symbol_name query was an optimization to avoid recomputing the common part of a symbol name, as only the hash needs to be added to it for each symbol.

However, #57967 will add a new mangling scheme, which doesn't readily support this kind of reuse - while it's plausible, it requires a lot more effort, since you'd have to "symbolically evaluate" mangling, and keep it in a form where the backreference positions can be computed correctly in the final step.

So I want to see how much time we're actually saving with this def_symbol_name optimization, nowadays.

cc @michaelwoerister

rust-highfive · 2019-03-21T16:33:41Z

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2019-03-21T17:09:41Z

@bors try

bors · 2019-03-21T17:09:56Z

⌛ Trying commit 3590a46 with merge 7172250...

rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`. The `def_symbol_name` query was an optimization to avoid recomputing the common part of a symbol name, as only the hash needs to be added to it for each symbol. However, #57967 will add a new mangling scheme, which doesn't readily support this kind of reuse - while it's plausible, it requires a lot more effort, since you'd have to "symbolically evaluate" mangling, and keep it in a form where the backreference positions can be computed correctly in the final step. So I want to see how much time we're actually saving with this `def_symbol_name` optimization, nowadays.

bors · 2019-03-21T19:33:35Z

☀️ Try build successful - checks-travis
Build commit: 7172250

eddyb · 2019-03-21T21:14:09Z

@rust-timer build 7172250

rust-timer · 2019-03-21T21:14:11Z

Success: Queued 7172250 with parent 48e354d, comparison URL.

rust-timer · 2019-03-21T23:54:06Z

Finished benchmarking try commit 7172250

oli-obk · 2019-03-22T12:47:55Z

Perf is randomly better or worse, but all below 1%. Max-rss seems to mostly have improvements, but that might just be noise, I don't know what the usual variances are.

impl lgtm

michaelwoerister · 2019-03-25T09:15:10Z

Looks good to me, thanks for the PR, @eddyb!

@bors r+

bors · 2019-03-25T09:15:11Z

📌 Commit 3590a46 has been approved by michaelwoerister

bors · 2019-03-26T21:06:17Z

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

eddyb · 2019-03-29T05:46:32Z

@bors r=michaelwoerister

bors · 2019-03-29T05:46:34Z

📌 Commit 03639a2 has been approved by michaelwoerister

@michaelwoerister

…woerister rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`. The `def_symbol_name` query was an optimization to avoid recomputing the common part of a symbol name, as only the hash needs to be added to it for each symbol. However, rust-lang#57967 will add a new mangling scheme, which doesn't readily support this kind of reuse - while it's plausible, it requires a lot more effort, since you'd have to "symbolically evaluate" mangling, and keep it in a form where the backreference positions can be computed correctly in the final step. So I want to see how much time we're actually saving with this `def_symbol_name` optimization, nowadays. cc @michaelwoerister

@ghost

Rollup of 5 pull requests Successful merges: - #59343 (rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`.) - #59380 (Fix invalid DWARF for enums when using ThinLTO) - #59463 (skip dyn keyword lint under macros) - #59539 (Fix infinite recursion) - #59544 (manifest: only include miri on the nightly channel) Failed merges: r? @ghost

rust-highfive assigned oli-obk Mar 21, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2019

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 Mar 25, 2019

eddyb mentioned this pull request Mar 26, 2019
Introduce Rust symbol mangling scheme. #57967
Merged

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 Mar 26, 2019

rustc(codegen): uncache def_symbol_name prefix from symbol_name.
03639a2

eddyb force-pushed the rm-def-symbol-name branch from 3590a46 to 03639a2Compare March 29, 2019 05:46

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 Mar 29, 2019

Centril mentioned this pull request Mar 30, 2019
Rollup of 5 pull requests #59561
Merged

bors merged commit 03639a2 into rust-lang:masterMar 30, 2019

eddyb deleted the rm-def-symbol-name branch March 30, 2019 18:21

Uh oh!

Uh oh!

Conversation

eddyb commented Mar 21, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 21, 2019

Uh oh!

eddyb commented Mar 21, 2019

Uh oh!

bors commented Mar 21, 2019

Uh oh!

bors commented Mar 21, 2019

Uh oh!

eddyb commented Mar 21, 2019

Uh oh!

rust-timer commented Mar 21, 2019

Uh oh!

rust-timer commented Mar 21, 2019

Uh oh!

oli-obk commented Mar 22, 2019

Uh oh!

michaelwoerister commented Mar 25, 2019

Uh oh!

bors commented Mar 25, 2019

Uh oh!

bors commented Mar 26, 2019

Uh oh!

eddyb commented Mar 29, 2019

Uh oh!

bors commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eddyb commented Mar 21, 2019•
edited
Loading