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.)
rustbot
added
S-waiting-on-review
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+ |
bors
added
S-waiting-on-bors
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 |
bors
added
S-waiting-on-review
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.)
Enselic
force-pushed
the
bench-padding
branch
from
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 |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
☀️ 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%) |
8 participants
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.