Skip to content

Inline hot part of PatStack::head_ctor #81978

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

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Feb 10, 2021

When building rustc with -Codegen-units=1 this inline hint ensures
that obtaining already initialized head constructor does not involve
a function call overhead and reduces the instruction count in
match-stress-enum-check full benchmark from 11.9G to 9.8G.

It shouldn't have significant impact on the currently default
configuration where it reflects existing inlining decisions.

@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 10, 2021

@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 10, 2021
@bors
Copy link
Collaborator

bors commented Feb 10, 2021

⌛ Trying commit 6a4fed86c78337ae6c5457f2467becbe0d97dcef with merge e775c2bd00d7e18512f927f472d53becf0876862...

@bors
Copy link
Collaborator

bors commented Feb 10, 2021

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

@rust-timer
Copy link
Collaborator

Queued e775c2bd00d7e18512f927f472d53becf0876862 with parent 07194ff, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e775c2bd00d7e18512f927f472d53becf0876862): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@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 11, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 11, 2021

When building rustc with a single CGU, there is a +32% regression in instruction counts on match-stress-enum-check benchmark. This reduces the regression to +9%. Direct comparison between two 1 CGU builds https://perf.rust-lang.org/compare.html?start=9660b75856b1db922bcfccaba516abaa9d1169fb&end=e775c2bd00d7e18512f927f472d53becf0876862&stat=instructions%3Au

BTW. 17% of instructions are in rustc_middle::ty::AdtDef::variant_index_with_id.

cc @Mark-Simulacrum

Lets verify impact on the current CGU configuration:

@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 11, 2021
@bors
Copy link
Collaborator

bors commented Feb 11, 2021

⌛ Trying commit 9bf28cfba7b6c39241aec9e2742faeafdf001954 with merge 46cd44235c63130eddf04cbeccfc81bbdd292a97...

@bors
Copy link
Collaborator

bors commented Feb 11, 2021

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

@rust-timer
Copy link
Collaborator

Queued 46cd44235c63130eddf04cbeccfc81bbdd292a97 with parent 1efd804, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (46cd44235c63130eddf04cbeccfc81bbdd292a97): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 11, 2021

As described in #81978 (comment), this helps obtaining favourable inlining results when building codege-units=1. This is not important for the current build of rustc, but it might be worth landing to avoid having investigate the same issue in the future.

r? @Mark-Simulacrum or @Nadrieril

@Nadrieril
Copy link
Member

As far as the code is concerned, this is definitely a super-hot spot that seems sensible to inline. I don't understand why codegen-units=1 is a relevant context but the perf gain there is impressive.
So LGTM but I'll let @Mark-Simulacrum decide, I don't feel knowledgeable enough about perf matters.

@Mark-Simulacrum
Copy link
Member

Could you put the explanation for why this is beneficial in the PR description? I'd also like to see comments in the code justifying the inline(never) in particular. It might be worth trying with cold, too, but I'm not sure if that's actually true?

@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 17, 2021

Updated commit message and PR description. I left inline hint only, which was sufficient to get desired results.

The implementation of Once in the core might merit some changes to do the right thing by default.

When building rustc with `-Codegen-units=1` this inline hint ensures
that obtaining already initialized head constructor does not involve
a function call overhead and reduces the instruction count in
match-stress-enum-check full benchmark from 11.9G to 9.8G.

It shouldn't have significant impact on the currently default
configuration where it reflects existing inlining decisions.
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 20, 2021

📌 Commit a3659bb has been approved by Mark-Simulacrum

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 20, 2021
@bors
Copy link
Collaborator

bors commented Feb 22, 2021

⌛ Testing commit a3659bb with merge 5cda2c855d1b15d29cda9263de9105dffd331b91...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Build completed successfully in 0:01:02
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
error: miri maintainer @oli-obk is not assignable in the rust-lang/rust repo
error: rustfmt maintainer @topecongiro is not assignable in the rust-lang/rust repo
error: nomicon maintainer @Gankra is not assignable in the rust-lang/rust repo
error: reference maintainer @ehuss is not assignable in the rust-lang/rust repo
error: reference maintainer @Havvy is not assignable in the rust-lang/rust repo
error: reference maintainer @matthewjasper is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @therealprof is not assignable in the rust-lang/rust repo
error: edition-guide maintainer @ehuss is not assignable in the rust-lang/rust repo
error: rustc-dev-guide maintainer @amanjeev is not assignable in the rust-lang/rust repo

  To be assignable, a person needs to be explicitly listed as a
  collaborator in the repository settings. The simple way to
  fix this is to ask someone with 'admin' privileges on the repo
  to add the person or whole team as a collaborator with 'read'
  privileges. Those privileges don't grant any extra permissions
  so it's safe to apply them.
The build will fail due to this.

@bors
Copy link
Collaborator

bors commented Feb 22, 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 Feb 22, 2021
@ehuss
Copy link
Contributor

ehuss commented Feb 22, 2021

@bors retry
publish-toolstate maintainer check is failing

@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 Feb 22, 2021
@bors
Copy link
Collaborator

bors commented Feb 22, 2021

⌛ Testing commit a3659bb with merge 11f838d...

@bors
Copy link
Collaborator

bors commented Feb 23, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 11f838d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 23, 2021
@bors bors merged commit 11f838d into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@tmiasko tmiasko deleted the head-ctor branch February 23, 2021 06:43
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. 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.

8 participants