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.

Sign up for GitHub

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

Jump to bottom

Add #[inline] to trivial AsRef/AsMut impls #94372

Merged
merged 1 commit into from
Mar 20, 2022
Merged

Add #[inline] to trivial AsRef/AsMut impls #94372

merged 1 commit into from
Mar 20, 2022

Conversation

erikdesjardins
Copy link
Contributor

These appeared uninlined in some perf runs, but they're trivial.

r? @ghost

@erikdesjardins
Add #[inline] to trivial AsRef impls
4194d75 
These appeared uninlined in some perf runs, but they're trivial.
@erikdesjardins erikdesjardins mentioned this pull request Feb 25, 2022
Merged
@nikic
Copy link
Contributor

nikic commented Feb 25, 2022

@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 Feb 25, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

⌛ Trying commit 4194d75 with merge cf7838d4076c630a0ff2ad5a8acf6ee148956056...

@bors
Copy link
Contributor

bors commented Feb 25, 2022

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

@rust-timer
Copy link
Collaborator

Queued cf7838d4076c630a0ff2ad5a8acf6ee148956056 with parent d981633, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cf7838d4076c630a0ff2ad5a8acf6ee148956056): comparison url.

Summary: This benchmark run shows 13 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -0.7%
  • Largest improvement in instruction counts: -0.9% on incr-unchanged builds of externs 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.

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

@rustbot rustbot added 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 Feb 26, 2022
@Kobzol
Copy link
Contributor

Kobzol commented Feb 26, 2022

It's interesting that inline helps even on generic methods.

@erikdesjardins
Copy link
Contributor Author

A small but consistent win.

r? rust-lang/compiler

@erikdesjardins
Copy link
Contributor Author

erikdesjardins commented Feb 26, 2022 •
edited
Loading

It's interesting that inline helps even on generic methods.

I was wondering if someone would comment that :P

It is true that generic functions are codegen'd in downstream crates (without -Zshare-generics), which enables inlining.

But #[inline] also adds the inlinehint LLVM attribute, which increases the inlining cost threshold, making LLVM more willing to inline: https://github.com/llvm/llvm-project/blob/a524a12231efa6cae94843fabdf0ec4fdd42d65d/llvm/lib/Analysis/InlineCost.cpp#L1837-L1841

@michaelwoerister
Copy link
Member

It is true that generic functions are codegen'd in downstream crates (without -Zshare-generics), which enables inlining.

That's right. One thing to keep in mind is that downstream crates are often compiled with multiple codegen units, so even though a generic function will have its own downstream instantiation, it might still not be available for inlining in all except one of the codegen units. On the other hand ThinLTO takes care of this pretty reliably for small functions, so I suspect the additional inlinehint to make the difference here.

Since this is in libcore: r? rust-lang/libs

@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 18, 2022
@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Mar 18, 2022
dtolnay
dtolnay approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2022

📌 Commit 4194d75 has been approved by dtolnay

@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 Mar 18, 2022
@bors
Copy link
Contributor

bors commented Mar 19, 2022

⌛ Testing commit 4194d75 with merge 90f79882992b98ddba920ab7da0175a63df55346...

@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 Mar 19, 2022

💔 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 Mar 19, 2022
@dtolnay
Copy link
Member

dtolnay commented Mar 19, 2022

x86_64-apple-2 failed with no indication why.

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

bors commented Mar 19, 2022

⌛ Testing commit 4194d75 with merge 2947c05ec6c442c1790f86fe0aa04d3815362b9a...

@bors
Copy link
Contributor

bors commented Mar 19, 2022

💔 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 Mar 19, 2022
@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)

@ehuss
Copy link
Contributor

ehuss commented Mar 19, 2022

@bors retry

dist-x86_64-apple-alt was cancelled after running 1.5 hours.

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

bors commented Mar 19, 2022

⌛ Testing commit 4194d75 with merge f2661cf...

@bors
Copy link
Contributor

bors commented Mar 20, 2022

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing f2661cf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 20, 2022
@bors bors merged commit f2661cf into rust-lang:master Mar 20, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 20, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f2661cf): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@erikdesjardins erikdesjardins deleted the asrefinl branch March 21, 2022 00:13
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2022
@bors
Auto merge of rust-lang#97905 - nnethercote:revert-infallible-encoder…
c845946 
…, r=bjorn3

Revert part of rust-lang#94372 to improve performance

rust-lang#94732 was supposed to give small but widespread performance improvements, as judged from three per-merge performance runs. But the performance run that occurred after merging included a roughly equal number of improvements and regressions, for unclear reasons.

This PR is for a test run reverting those changes, to see what happens.

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.