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

Monomorphize calculate_dtor instead of using function pointers #77755

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 9, 2020

Change calculate_dtor to avoid dynamic dispatching. This change allows the empty functions to be optimized away.

Based on the discussion in #77754 (comment), the performance impact of this change was measured.

Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(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 Oct 9, 2020
@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2020
@jyn514 jyn514 changed the title [DO NOT MERGE] Performance impact of a function signature change [DO NOT MERGE] Performance impact of monomorphizing calculate_dtor instead of using function pointers Oct 9, 2020
@ecstatic-morse
Copy link
Contributor

The input closure is called in a loop for each relevant impl and is trivial for at least one plausibly hot call (adt_destructor). It's possible that this will speed things up enough to matter.

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 10, 2020

⌛ Trying commit fc17e91e14701a76c404bc05e44807db91e7261a with merge ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc...

@bors
Copy link
Contributor

bors commented Oct 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc (ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc)

@rust-timer
Copy link
Collaborator

Queued ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc with parent 7bc5839, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc): 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

@bugadani bugadani changed the title [DO NOT MERGE] Performance impact of monomorphizing calculate_dtor instead of using function pointers Monomorphize calculate_dtor instead of using function pointers Oct 11, 2020
@bugadani
Copy link
Contributor Author

@ecstatic-morse could you please take a look at the results? Do you think this change would make sense? Thanks!

@ecstatic-morse
Copy link
Contributor

Seems to be a very tiny win, only enough to matter on very small crates. Still, using a trait object here is saving like two monomorphizations, so I think a generic parameter is fine.

@bors rollup-
@bors r+

@bors
Copy link
Contributor

bors commented Oct 12, 2020

📌 Commit 0d27b76 has been approved by ecstatic-morse

@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 Oct 12, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 13, 2020
…-morse

Monomorphize `calculate_dtor` instead of using function pointers

Change `calculate_dtor` to avoid dynamic dispatching. This change allows the empty functions to be optimized away.

Based on the discussion in rust-lang#77754 (comment), the performance impact of this change was measured.

Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc
@bors
Copy link
Contributor

bors commented Oct 13, 2020

⌛ Testing commit 0d27b76 with merge a6aa039311c05d0f43e07f49a770cdac914daf0e...

@bors
Copy link
Contributor

bors commented Oct 13, 2020

💔 Test failed - checks-azure

@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 Oct 13, 2020
@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, 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 @rust-lang/infra. (Feature Requests)

@pietroalbini
Copy link
Member

Seems like a spurious failure.

@bors retry

@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 Oct 13, 2020
@bors
Copy link
Contributor

bors commented Oct 13, 2020

⌛ Testing commit 0d27b76 with merge 2d6eccd...

@bors
Copy link
Contributor

bors commented Oct 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: ecstatic-morse
Pushing 2d6eccd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2020
@bors bors merged commit 2d6eccd into rust-lang:master Oct 13, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 13, 2020
@bugadani bugadani deleted the perf-calc-dtor branch October 13, 2020 12:18
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Oct 22, 2020

This ended up as a 1% regression on the deeply-nested-async benchmark -- do we think that's likely to be spurious? The original perf results don't show this regression. Wall times also seem up across the board, but that could be noise.

I am somewhat surprised by these results -- this PR looks like it shouldn't cause such significant effects. That said, it does seem like the extra inlining here could end up harmful in the end perhaps? I am inclined to revert it (or at least benchmark a revert).

@bugadani
Copy link
Contributor Author

There is always a chance that this screwed up cache alignment, in combination with another PR that interacts with it. I wonder if a PR between the perf run and the merge pushed something out of alignment, maybe? If that is possible, I think this PR could cause unnecessary noise and should be reverted. But it would be nice to know for sure, what happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants