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

Optimize RcInnerPtr::inc_strong()/inc_weak() instruction count #95224

Merged
merged 3 commits into from
Apr 16, 2022

Conversation

mjbshaw
Copy link
Contributor

@mjbshaw mjbshaw commented Mar 23, 2022

Inspired by this internals thread: https://internals.rust-lang.org/t/rc-optimization-on-64-bit-targets/16362

The generated assembly is a bit smaller and is a more efficient usage of the CPU's instruction cache. unlikely doesn't impact any of the small artificial tests I've done, but I've included it in case it might help more complex scenarios when this is inlined.

Inspired by this internals thread: https://internals.rust-lang.org/t/rc-optimization-on-64-bit-targets/16362

[The generated assembly is a bit smaller](https://rust.godbolt.org/z/TeTnf6144) and is a more efficient usage of the CPU's instruction cache. `unlikely` doesn't impact any of the small artificial tests I've done, but I've included it in case it might help more complex scenarios when this is inlined.
@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 Mar 23, 2022
library/alloc/src/rc.rs Show resolved Hide resolved
library/alloc/src/rc.rs Show resolved Hide resolved
@yaahc yaahc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 23, 2022
@yaahc
Copy link
Member

yaahc commented Apr 12, 2022

Looks great, thank you for the PR!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 12, 2022

📌 Commit 8d14c03 has been approved by yaahc

@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 Apr 12, 2022
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Apr 15, 2022
Optimize RcInnerPtr::inc_strong()/inc_weak() instruction count

Inspired by this internals thread: https://internals.rust-lang.org/t/rc-optimization-on-64-bit-targets/16362

[The generated assembly is a bit smaller](https://rust.godbolt.org/z/TeTnf6144) and is a more efficient usage of the CPU's instruction cache. `unlikely` doesn't impact any of the small artificial tests I've done, but I've included it in case it might help more complex scenarios when this is inlined.
@Mark-Simulacrum
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Apr 15, 2022

⌛ Testing commit 8d14c03 with merge 78bca4dd0d02b7d3ec42f55ff0511b4a7894811d...

@Dylan-DPC
Copy link
Member

@bors retry (letting rollup go first since queue is long)

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Apr 15, 2022

⌛ Testing commit 8d14c03 with merge 2761af47284e3ef7ac9a027a40e98da242fb60ac...

@Mark-Simulacrum
Copy link
Member

@bors retry -- gha-self-hosted restart

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Apr 15, 2022

⌛ Testing commit 8d14c03 with merge bb1a03c...

@bors
Copy link
Contributor

bors commented Apr 16, 2022

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing bb1a03c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2022
@bors bors merged commit bb1a03c into rust-lang:master Apr 16, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 16, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bb1a03c): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 4 0 1 0
mean2 N/A 1.9% N/A -0.3% N/A
max N/A 3.9% N/A -0.3% N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label Apr 16, 2022
@pnkfelix
Copy link
Member

  • The bulk of this regression is attached to secondary benchmark regression-31157 opt (incr-patched: println, incr-full, full), by up to 3.86%
  • the self-profile data seems to indicate that the bulk of the cost here is spent in LLVM.
  • the cachegrind output seconds that: https://gist.github.com/4fa7403ee2e812e7712c5046e9eb4d72
  • This PR is motivated by an improvement to the object code itself generated by the compiler. I guess that improvement did not result in a net win for the compiler itself.
  • regression-31157 (rust#31157) is encoding a case where we were seeing a 20x slowdown. So we should not be worrying about 2% or 4% performance losses there.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 21, 2022
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.