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

Obligation forest tweaks #97674

Merged
merged 3 commits into from Jun 20, 2022
Merged

Obligation forest tweaks #97674

merged 3 commits into from Jun 20, 2022

Conversation

nnethercote
Copy link
Contributor

A few minor improvements to the code.

r? @nikomatsakis

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

I'm not expecting much performance change, maybe a small improvement to keccak and one or two others.

@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 Jun 3, 2022
@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Trying commit f673eade735b3e85b1703b11f12c00d715a29c44 with merge 1e46fe42354430de9aca5fb2769e2766f4853903...

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Trying commit bb5005ff758306b0ed470915ff5cb0508d161967 with merge fbe2676ec277a531d1e97d27b8ab76f06129f797...

@bors
Copy link
Contributor

bors commented Jun 3, 2022

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

@rust-timer
Copy link
Collaborator

Queued fbe2676ec277a531d1e97d27b8ab76f06129f797 with parent f5507aa, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fbe2676ec277a531d1e97d27b8ab76f06129f797): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.9% 0.9% 2
Regressions 😿
(secondary)		 2.1% 2.4% 6
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 0.9% 0.9% 2

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 4.1% 5.8% 2
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 4.1% 5.8% 2

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 3.1% 3.1% 1
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -3.6% -3.6% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -0.2% -3.6% 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.

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. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 3, 2022
@nnethercote
Streamline active_cache to done_cache transfer.
cdb446f
@nnethercote
Handle stalling within ObligationForest.
281229a 
It is simpler if `ObligationForest` does this itself, rather than the
caller having to manage it.
@nnethercote
Split process_obligation in two.
32741d5 
Because it really has two halves:
- A read-only part that checks if further work is needed.
- The further work part, which is much less hot.

This makes things a bit clearer and nicer.
@nnethercote
Copy link
Contributor Author

Ugh, this was a tiny perf win for me locally. PGO's effects again, I suspect. I will try a few more CI perf runs, try to work out the problem.

@nnethercote
Copy link
Contributor Author

@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 Jun 6, 2022
@bors
Copy link
Contributor

bors commented Jun 6, 2022

⌛ Trying commit 32741d5 with merge 991b508f88d9c528d5dd375f43c1cececea50473...

@bors
Copy link
Contributor

bors commented Jun 6, 2022

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

@rust-timer
Copy link
Collaborator

Queued 991b508f88d9c528d5dd375f43c1cececea50473 with parent 760237f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (991b508f88d9c528d5dd375f43c1cececea50473): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -0.5% -0.7% 3
Improvements 🎉
(secondary)		 -1.5% -2.0% 7
All 😿🎉 (primary) -0.5% -0.7% 3

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 1.6% 3.1% 2
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 1.6% 3.1% 2

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.2% 2.2% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -3.8% -4.0% 2
All 😿🎉 (primary) N/A N/A 0

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. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jun 6, 2022
@nnethercote
Copy link
Contributor Author

Hooray, those new perf results are much better. @nikomatsakis, review away please! :)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit 32741d5 has been approved by nikomatsakis

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

bors commented Jun 20, 2022

⌛ Testing commit 32741d5 with merge 1d60108...

@bors
Copy link
Contributor

bors commented Jun 20, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 1d60108 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 20, 2022
@bors bors merged commit 1d60108 into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
@bors bors mentioned this pull request Jun 20, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1d60108): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.8% 0.9% 2
Regressions 😿
(secondary)		 1.8% 2.4% 7
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -0.3% -0.3% 1
All 😿🎉 (primary) 0.8% 0.9% 2

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 3.7% 5.9% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -3.6% -3.6% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

This benchmark run did not return any relevant results for this metric.

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. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@rustbot rustbot added the perf-regression Performance regression. label Jun 20, 2022
@nnethercote nnethercote deleted the oblig-forest-tweaks branch June 20, 2022 21:45
@nnethercote
Copy link
Contributor Author

Sigh, performance regression here. keccak stresses the process obligation code, and it's very easy for minor, seemingly inconsequential changes to change it a couple of percent, because e.g. register allocation in the hot paths change a tiny bit. E.g. here are the results for the past month:

Screen Shot 2022-06-21 at 7 52 23 am

Let's look at the check full results, particularly the instruction counts.

This PR was looking like a win when measured 15 days ago (2022-06-06):

keccak check full -1.96% 10.03x 20,749,612,998 20,343,711,976

Since then, three intervening PRs landed that improved keccak.

After #97447 (2022-06-08):

keccak check full -2.06% 10.83x 20,718,223,737 20,291,756,873

After #97778 (2022-06-11):

keccak check full -0.79% 6.05x 20,284,132,956 20,123,775,316

After #96591 (2022-06-14):

keccak check full -1.61% 13.79x 20,111,191,751 19,787,718,379

So that was nice.

And now, after this PR merged yesterday (2022-06-20):

keccak check full 2.38% 24.28x 19,797,304,578 20,268,024,059

(All of the above is also true for cranelift-codegen-0.82.1, to a lesser extent.)

I'm inclined to accept this and move on, because:

  • this PR made the code nicer, and
  • keccak's instruction count is still lower than it was three weeks ago (3 steps forward, 1 step back, and I was responsible for two of those steps), and
  • the results here are so easy to perturb, and
  • this PR didn't affect cycle counts.

I may try one more time to recover the loss, but this code has already been optimized to the nth degree, so I probably won't spend long on it.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@nnethercote @rust-timer @bors @nikomatsakis @rust-highfive @rustbot