-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
libtest: Fix padding of benchmarks run as tests #118548
Conversation
As you can see the padding is wrong when running benches as tests. This will be fixed in the next commit. (Benches should only be padded when run as benches to make it easy to compare the benchmark numbers.)
This comment was marked as outdated.
This comment was marked as outdated.
r? @saethlin who last touched the code I am changing |
This is good work and very well-tested, but I'm not a member of the library team. I'm asking on Zulip about who should be approving this. |
r? @thomcc |
Seems good to me! Thanks! @bors r+ |
libtest: Fix padding of benchmarks run as tests ### Summary The first commit adds regression tests for libtest padding. The second commit fixes padding for benches run as tests and updates the blessed output of the regression tests to make it clear what effect the fix has on padding. Closes rust-lang#104092 which is **E-help-wanted** and **regression-from-stable-to-stable** ### More details Before this fix we applied padding _before_ manually doing what `convert_benchmarks_to_tests()` does which affects padding calculations. Instead use `convert_benchmarks_to_tests()` first if applicable and then apply padding afterwards so it becomes correct. Benches should only be padded when run as benches to make it easy to compare the benchmark numbers. Not when run as tests. r? `@ghost` until CI passes.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Before this fix we applied padding before manually doing what `convert_benchmarks_to_tests()` does. Instead use `convert_benchmarks_to_tests()` if applicable and then apply padding afterwards so it becomes correct. (Benches should only be padded when run as benches to make it easy to compare the benchmark numbers.)
809b2ad
to
12e6bcf
Compare
I had to replace @thomcc Can you r+ again please? Thanks! Diff from what you already approved. |
I'm happy to approve the diff. @bors r=thomcc,ChrisDenton |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5431404): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 666.091s -> 666.39s (0.04%) |
Summary
The first commit adds regression tests for libtest padding.
The second commit fixes padding for benches run as tests and updates the blessed output of the regression tests to make it clear what effect the fix has on padding.
Closes #104092 which is E-help-wanted and regression-from-stable-to-stable
More details
Before this fix we applied padding before manually doing what
convert_benchmarks_to_tests()
does which affects padding calculations. Instead useconvert_benchmarks_to_tests()
first if applicable and then apply padding afterwards so it becomes correct.Benches should only be padded when run as benches to make it easy to compare the benchmark numbers. Not when run as tests.
r? @ghost until CI passes.