Skip to content

Inline ty::Const::ty() and ty::Const::val() getters #96022

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
Merged
merged 1 commit into from
Apr 20, 2022
Merged

Inline ty::Const::ty() and ty::Const::val() getters #96022

merged 1 commit into from
Apr 20, 2022

Conversation

martingms
Copy link
Contributor

These were not inlined into super_relate_consts, which is one of the hottest functions in a callgrind profile of compiling bitmaps-3.1.0.

Yields some small speedups across various benchmarks locally:

Primary benchmarks

Benchmark Profile Scenario % Change Significance Factor?
unicode-normalization-0.1.19 check full -0.56% 2.78x
unicode-normalization-0.1.19 check incr-full -0.43% 2.15x
unicode-normalization-0.1.19 opt full -0.35% 1.77x
unicode-normalization-0.1.19 debug incr-full -0.31% 1.56x
unicode-normalization-0.1.19 debug full -0.30% 1.51x

Secondary benchmarks

Benchmark Profile Scenario % Change Significance Factor?
bitmaps-3.1.0 check full -1.88% 9.39x
bitmaps-3.1.0 debug full -1.79% 8.96x
bitmaps-3.1.0 opt full -1.69% 8.43x
bitmaps-3.1.0 check incr-full -1.54% 7.68x
bitmaps-3.1.0 debug incr-full -1.45% 7.27x
bitmaps-3.1.0 opt incr-full -1.39% 6.96x
tt-muncher opt full 1.28% 6.38x
nalgebra-0.30.1 check full -0.96% 4.81x
nalgebra-0.30.1 debug full -0.91% 4.54x
nalgebra-0.30.1 opt full -0.90% 4.52x
nalgebra-0.30.1 check incr-full -0.77% 3.86x
nalgebra-0.30.1 opt incr-full -0.76% 3.79x
nalgebra-0.30.1 debug incr-full -0.74% 3.72x
hex-0.4.3 check full -0.70% 3.50x
hex-0.4.3 debug full -0.59% 2.95x
hex-0.4.3 check incr-full -0.56% 2.80x
hex-0.4.3 opt full -0.56% 2.78x
wf-projection-stress-65510 opt full -0.48% 2.42x
hex-0.4.3 opt incr-full -0.48% 2.40x
hex-0.4.3 debug incr-full -0.45% 2.24x
wf-projection-stress-65510 check full -0.44% 2.18x
wf-projection-stress-65510 debug full -0.42% 2.08x
wf-projection-stress-65510 check incr-full -0.40% 2.01x
deep-vector debug incr-patched: add vec item -0.38% 1.88x
wf-projection-stress-65510 debug incr-full -0.37% 1.86x
wf-projection-stress-65510 opt incr-full -0.36% 1.81x
deep-vector debug incr-patched: println 0.33% 1.63x

r? @nnethercote

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2022
@compiler-errors
Copy link
Member

@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 Apr 13, 2022
@bors
Copy link
Collaborator

bors commented Apr 13, 2022

⌛ Trying commit 692bba6 with merge b2f31be77ed70401ca507b09dd41e49597ad4df6...

@bors
Copy link
Collaborator

bors commented Apr 13, 2022

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

@rust-timer
Copy link
Collaborator

Queued b2f31be77ed70401ca507b09dd41e49597ad4df6 with parent 0d13f6a, future comparison URL.

@nnethercote
Copy link
Contributor

LOL, r=me once the CI perf results come in.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b2f31be77ed70401ca507b09dd41e49597ad4df6): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 8 6 6 6
mean2 N/A 1.7% -1.7% -0.8% -1.7%
max N/A 2.4% -2.2% -0.9% -2.2%

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 may lead to changes in compiler perf.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2022
@nnethercote
Copy link
Contributor

deeply-nested-multi regressed again, supposedly? That seems suspicious for this simple change. I'm not sure what to make of it.

@nnethercote nnethercote mentioned this pull request Apr 19, 2022
Merged
@nnethercote
Copy link
Contributor

Now that #96020 has landed, let's try again.

@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 Apr 19, 2022
@bors
Copy link
Collaborator

bors commented Apr 19, 2022

⌛ Trying commit 692bba6 with merge 157d1bbcba9d4182b038493d964f4e7f8e332f9f...

@bors
Copy link
Collaborator

bors commented Apr 19, 2022

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

@rust-timer
Copy link
Collaborator

Queued 157d1bbcba9d4182b038493d964f4e7f8e332f9f with parent 4ca19e0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (157d1bbcba9d4182b038493d964f4e7f8e332f9f): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 6 6 7
mean2 2.7% N/A -1.6% -0.9% -1.0%
max 2.7% N/A -2.2% -1.0% 2.7%

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 may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 20, 2022
@nnethercote
Copy link
Contributor

Perf results look good. I think the syn-1.0.89 opt full result is just a random fluctuation of some kind, because there's no good reason why this PR would affect that one but none of the other syn-1.0.89 results.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 20, 2022
@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 20, 2022

📌 Commit 692bba6 has been approved by nnethercote

@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 20, 2022
@bors
Copy link
Collaborator

bors commented Apr 20, 2022

⌛ Testing commit 692bba6 with merge 0034bbc...

@bors
Copy link
Collaborator

bors commented Apr 20, 2022

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 0034bbc to master...

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

Finished benchmarking commit (0034bbc): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 7 6 7
mean2 N/A N/A -1.9% -0.7% -1.9%
max N/A N/A -3.0% -0.9% -3.0%

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the perf-regression Performance regression. label Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nnethercote nnethercote

Labels
merged-by-bors This PR was explicitly merged by bors. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
7 participants
@martingms @compiler-errors @rust-timer @bors @nnethercote @rust-highfive @rustbot