-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[DO MERGE] PGO new solver #141453
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
[DO MERGE] PGO new solver #141453
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[DONT MERGE] PGO new solver 🤔 I hope I did this right...
Looks about right. I was planning to do this, so thanks! We'll see how much longer it takes to run these new benchmarks, maybe it will be fine to just have 1-2 of the new-solver crates in the training set. |
Yep, I'm not prepared to land this yet, I just want to see how much PGO actually matters for the new solver. I think just including a couple stressy crates like nalgebra would be sufficient :> |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Oh, sorry, this is essentially a bug in the rustc-perf CLI parser, it tries to detect unused benchmarks, but it doesn't handle well situations where one benchmark is a prefix of another. Removing |
8c50bba
to
e1d250f
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[DONT MERGE] PGO new solver 🤔 I hope I did this right...
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (1fde446): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -2.1%, secondary -3.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -1.1%, secondary -11.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 776.992s -> 774.276s (-0.35%) |
Yup, looks like it helps quite a lot! We should land the general benchmark update and rustc-perd bump separately from adding the new solver benchmarks. |
e1d250f
to
0a84dfa
Compare
☔ The latest upstream changes (presumably #141490) made this pull request unmergeable. Please resolve the merge conflicts. |
0a84dfa
to
f7d42c9
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (9ef33a0): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary -0.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -5.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
I think the results are basically similar. Let me squash this and this should be good? Or is this still affecting bootstrap time (idk how to check)? |
I think it's fine like this, thanks. |
0ab1217
to
3253de6
Compare
@rustbot ready |
rustbot has assigned @Mark-Simulacrum. Use |
r? kobzol @bors r+ |
Funny, apparently bors doesn't understand DONT MERGE xD |
lol |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing e0d014a (parent) -> 738c08b (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 738c08b63c4f9e3ebdaec5eece7b6fbc354f6467 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (738c08b): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -4.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.196s -> 776.294s (-0.12%) |
Regressions are acceptable given the wins and intent of the PR. |
🤔
I hope I did this right...