-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Optimize ToString
implementation for integers
#136264
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
Optimize ToString
implementation for integers
#136264
Conversation
Could not assign reviewer from: |
rustbot has assigned @Mark-Simulacrum. Use |
This comment has been minimized.
This comment has been minimized.
I'm very confused by the CI errors. Did I unfold a very weird bug somehow? Like |
specialization strikes again, perhaps |
It seems to me that the tests are testing for a known miscompilation: #107975 |
I'm guessing that this PR caused the compiler to be able to figure out that calling to_string on a number doesn't cause that number to change, allowing more optimizations to happen, causing the miscompilation to behave differently. |
lovely. |
Fixed CI. So now about benching the change: any suggestion @workingjubilee ? In the meantime: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…string, r=<try> Optimize `ToString` implementation for integers Part of rust-lang#135543. Follow-up of rust-lang#133247 and rust-lang#128204. Rather than writing pretty bad benchers like I did last time, `@workingjubilee:` do you have a suggestion on how to check the impact on performance for this PR? Thanks in advance! r? `@workingjubilee`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (97c5b4b): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 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 0.3%, secondary 2.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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.1%, 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.
Bootstrap: 776.287s -> 777.396s (0.14%) |
I don't believe the tests should be changed in this way. |
I'm not sure if we should keep the code and modify it (like I did) or just comment out the parts that are failing. Or maybe you see a third way? |
The point of these tests is the series of asserts, changing just one line or two that fails undermines the point. |
The point was testing that calling |
These are known-bug tests. They are testing for the existence of a bug. We did not fix the bug, because there are other ways to reach the compilation quirks in question. They have little to nothing to do with |
Oh I see. So in short, calling any method on the type should still trigger the original bug iiuc. Let me give it a try. In the meantime, any idea on how to bench this PR? |
83dc76e
to
1713773
Compare
You were right, just calling another method did the trick. Tests are now back to what they were. |
Thank you. I will think about that a bit. |
☔ The latest upstream changes (presumably #135265) made this pull request unmergeable. Please resolve the merge conflicts. |
…string, r=Amanieu Optimize `ToString` implementation for integers Part of rust-lang#135543. Follow-up of rust-lang#133247 and rust-lang#128204. The benchmark results are: | name| 1.87.0-nightly (3ea711f 2025-03-09) | With this PR | diff | |-|-|-|-| | bench_i16 | 32.06 ns/iter (+/- 0.12) | 17.62 ns/iter (+/- 0.03) | -45% | | bench_i32 | 31.61 ns/iter (+/- 0.04) | 15.10 ns/iter (+/- 0.06) | -52% | | bench_i64 | 31.71 ns/iter (+/- 0.07) | 15.02 ns/iter (+/- 0.20) | -52% | | bench_i8 | 13.21 ns/iter (+/- 0.14) | 14.93 ns/iter (+/- 0.16) | +13% | | bench_u16 | 31.20 ns/iter (+/- 0.06) | 16.14 ns/iter (+/- 0.11) | -48% | | bench_u32 | 33.27 ns/iter (+/- 0.05) | 16.18 ns/iter (+/- 0.10) | -51% | | bench_u64 | 31.44 ns/iter (+/- 0.06) | 16.62 ns/iter (+/- 0.21) | -47% | | bench_u8 | 10.57 ns/iter (+/- 0.30) | 13.00 ns/iter (+/- 0.43) | +22% | More information about it in [the original comment](rust-lang#136264 (comment)). r? `@workingjubilee`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Error is:
So I guess problem is still device space? cc @marcoieni 😉 |
Yes, looks like the same issue. |
The CI works again 👍 |
That was fast, let's try again. =D @bors retry |
☀️ 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 c4e05e5 (parent) -> d97326e (this PR) Test differencesShow 48 test diffs48 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard d97326eabfc3b2c33abcb08d6bc117aefa697cb7 --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 (d97326e): 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 (primary -1.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 3.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.
Binary sizeResults (primary 0.0%, secondary 0.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.
Bootstrap: 773.554s -> 773.215s (-0.04%) |
…=Amanieu Optimize `ToString` implementation for integers Part of rust-lang/rust#135543. Follow-up of rust-lang/rust#133247 and rust-lang/rust#128204. The benchmark results are: | name| 1.87.0-nightly (3ea711f17 2025-03-09) | With this PR | diff | |-|-|-|-| | bench_i16 | 32.06 ns/iter (+/- 0.12) | 17.62 ns/iter (+/- 0.03) | -45% | | bench_i32 | 31.61 ns/iter (+/- 0.04) | 15.10 ns/iter (+/- 0.06) | -52% | | bench_i64 | 31.71 ns/iter (+/- 0.07) | 15.02 ns/iter (+/- 0.20) | -52% | | bench_i8 | 13.21 ns/iter (+/- 0.14) | 14.93 ns/iter (+/- 0.16) | +13% | | bench_u16 | 31.20 ns/iter (+/- 0.06) | 16.14 ns/iter (+/- 0.11) | -48% | | bench_u32 | 33.27 ns/iter (+/- 0.05) | 16.18 ns/iter (+/- 0.10) | -51% | | bench_u64 | 31.44 ns/iter (+/- 0.06) | 16.62 ns/iter (+/- 0.21) | -47% | | bench_u8 | 10.57 ns/iter (+/- 0.30) | 13.00 ns/iter (+/- 0.43) | +22% | More information about it in [the original comment](rust-lang/rust#136264 (comment)). r? `@workingjubilee`
…=Amanieu Optimize `ToString` implementation for integers Part of rust-lang/rust#135543. Follow-up of rust-lang/rust#133247 and rust-lang/rust#128204. The benchmark results are: | name| 1.87.0-nightly (3ea711f17 2025-03-09) | With this PR | diff | |-|-|-|-| | bench_i16 | 32.06 ns/iter (+/- 0.12) | 17.62 ns/iter (+/- 0.03) | -45% | | bench_i32 | 31.61 ns/iter (+/- 0.04) | 15.10 ns/iter (+/- 0.06) | -52% | | bench_i64 | 31.71 ns/iter (+/- 0.07) | 15.02 ns/iter (+/- 0.20) | -52% | | bench_i8 | 13.21 ns/iter (+/- 0.14) | 14.93 ns/iter (+/- 0.16) | +13% | | bench_u16 | 31.20 ns/iter (+/- 0.06) | 16.14 ns/iter (+/- 0.11) | -48% | | bench_u32 | 33.27 ns/iter (+/- 0.05) | 16.18 ns/iter (+/- 0.10) | -51% | | bench_u64 | 31.44 ns/iter (+/- 0.06) | 16.62 ns/iter (+/- 0.21) | -47% | | bench_u8 | 10.57 ns/iter (+/- 0.30) | 13.00 ns/iter (+/- 0.43) | +22% | More information about it in [the original comment](rust-lang/rust#136264 (comment)). r? `@workingjubilee`
Regression on a single benchmark that is new and doesn't have proper noise bounds yet. Also a nice win on the @rustbot label: +perf-regression-triaged |
…=Amanieu Optimize `ToString` implementation for integers Part of rust-lang/rust#135543. Follow-up of rust-lang/rust#133247 and rust-lang/rust#128204. The benchmark results are: | name| 1.87.0-nightly (3ea711f17 2025-03-09) | With this PR | diff | |-|-|-|-| | bench_i16 | 32.06 ns/iter (+/- 0.12) | 17.62 ns/iter (+/- 0.03) | -45% | | bench_i32 | 31.61 ns/iter (+/- 0.04) | 15.10 ns/iter (+/- 0.06) | -52% | | bench_i64 | 31.71 ns/iter (+/- 0.07) | 15.02 ns/iter (+/- 0.20) | -52% | | bench_i8 | 13.21 ns/iter (+/- 0.14) | 14.93 ns/iter (+/- 0.16) | +13% | | bench_u16 | 31.20 ns/iter (+/- 0.06) | 16.14 ns/iter (+/- 0.11) | -48% | | bench_u32 | 33.27 ns/iter (+/- 0.05) | 16.18 ns/iter (+/- 0.10) | -51% | | bench_u64 | 31.44 ns/iter (+/- 0.06) | 16.62 ns/iter (+/- 0.21) | -47% | | bench_u8 | 10.57 ns/iter (+/- 0.30) | 13.00 ns/iter (+/- 0.43) | +22% | More information about it in [the original comment](rust-lang/rust#136264 (comment)). r? `@workingjubilee`
…string, r=Amanieu Optimize `ToString` implementation for integers Part of rust-lang#135543. Follow-up of rust-lang#133247 and rust-lang#128204. The benchmark results are: | name| 1.87.0-nightly (3ea711f 2025-03-09) | With this PR | diff | |-|-|-|-| | bench_i16 | 32.06 ns/iter (+/- 0.12) | 17.62 ns/iter (+/- 0.03) | -45% | | bench_i32 | 31.61 ns/iter (+/- 0.04) | 15.10 ns/iter (+/- 0.06) | -52% | | bench_i64 | 31.71 ns/iter (+/- 0.07) | 15.02 ns/iter (+/- 0.20) | -52% | | bench_i8 | 13.21 ns/iter (+/- 0.14) | 14.93 ns/iter (+/- 0.16) | +13% | | bench_u16 | 31.20 ns/iter (+/- 0.06) | 16.14 ns/iter (+/- 0.11) | -48% | | bench_u32 | 33.27 ns/iter (+/- 0.05) | 16.18 ns/iter (+/- 0.10) | -51% | | bench_u64 | 31.44 ns/iter (+/- 0.06) | 16.62 ns/iter (+/- 0.21) | -47% | | bench_u8 | 10.57 ns/iter (+/- 0.30) | 13.00 ns/iter (+/- 0.43) | +22% | More information about it in [the original comment](rust-lang#136264 (comment)). r? `@workingjubilee`
Part of #135543.
Follow-up of #133247 and #128204.
The benchmark results are:
More information about it in the original comment.
r? @workingjubilee