Skip to content

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jun 10, 2025

Part of #135543.

Follow-up of #136264.

When working on #142098, I realized that i128 and u128 could also benefit from a distinct ToString implementation so here it.

The last commit is just me realizing that I forgot to add the format tests for usize and isize.

Here is the bench comparison:

bench name last nightly with this PR diff
bench_i128 29.25 ns/iter (+/- 0.66) 17.52 ns/iter (+/- 0.7) -40.1%
bench_u128 34.06 ns/iter (+/- 0.21) 16.1 ns/iter (+/- 0.6) -52.7%

I used this code to test:

#![feature(test)]

extern crate test;

use test::{Bencher, black_box};

#[inline(always)]
fn convert_to_string<T: ToString>(n: T) -> String {
    n.to_string()
}

macro_rules! decl_benches {
    ($($name:ident: $ty:ident,)+) => {
        $(
	    #[bench]
            fn $name(c: &mut Bencher) {
                c.iter(|| convert_to_string(black_box({ let nb: $ty = 20; nb })));
            }
	)+
    }
}

decl_benches! {
    bench_u128: u128,
    bench_i128: i128,
}

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2025

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 10, 2025
@bors
Copy link
Collaborator

bors commented Jun 12, 2025

☔ The latest upstream changes (presumably #136594) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the specialize-tostring-on-128-integers branch from b5b6654 to 53dc659 Compare June 13, 2025 12:11
@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts.

@workingjubilee workingjubilee changed the title Specialize ToString implementation on u128 and i128 Use a distinct ToString implementation for u128 and i128 Jun 13, 2025
@workingjubilee
Copy link
Member

Having done code review of things that use Rust's feature(min_specialization), I now consider "specialization" a dirty word.

@GuillaumeGomez
Copy link
Member Author

Noted, thanks for the title update then. :)

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've tested locally, is there any measurable perf/size impact here?

@GuillaumeGomez GuillaumeGomez force-pushed the specialize-tostring-on-128-integers branch from 53dc659 to 9b09948 Compare June 16, 2025 09:54
@GuillaumeGomez
Copy link
Member Author

@tgross35 I added the comparison in the first comment with the code I use to get the performance comparison.

@tgross35
Copy link
Contributor

Thanks!

I don't think this needs a pre-merge perf run since we didn't see any perf-visible impact in #136594, but it wouldn't hurt to get one after so

@bors r+ rollup=never

Fyi @pascaldekloe since you did the last round of perf boosts for these methods

@bors
Copy link
Collaborator

bors commented Jun 16, 2025

📌 Commit 9b09948 has been approved by tgross35

It is now in the queue for this repository.

@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 16, 2025
@tgross35 tgross35 assigned tgross35 and unassigned workingjubilee Jun 18, 2025
@bors
Copy link
Collaborator

bors commented Jun 20, 2025

⌛ Testing commit 9b09948 with merge 5b74275...

@bors
Copy link
Collaborator

bors commented Jun 20, 2025

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 5b74275 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 20, 2025
@bors bors merged commit 5b74275 into rust-lang:master Jun 20, 2025
11 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 20, 2025
Copy link
Contributor

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 255aa22 (parent) -> 5b74275 (this PR)

Test differences

Show 26 test diffs

Stage 1

  • num::test_isize_to_string: [missing] -> pass (J0)
  • num::test_usize_to_string: [missing] -> pass (J0)

Additionally, 24 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 5b74275f89b6041bf2e9dc2abcf332e206d4cfca --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-aarch64-apple: 4689.2s -> 6379.9s (36.1%)
  2. x86_64-apple-2: 3543.9s -> 4546.3s (28.3%)
  3. dist-x86_64-apple: 9048.3s -> 7868.7s (-13.0%)
  4. mingw-check-tidy: 77.1s -> 69.0s (-10.5%)
  5. x86_64-apple-1: 6181.5s -> 6732.2s (8.9%)
  6. x86_64-rust-for-linux: 2603.2s -> 2819.3s (8.3%)
  7. i686-gnu-1: 7677.0s -> 7174.4s (-6.5%)
  8. x86_64-gnu-llvm-20-3: 6777.2s -> 6353.4s (-6.3%)
  9. dist-aarch64-linux: 8178.3s -> 7691.0s (-6.0%)
  10. dist-x86_64-netbsd: 4763.9s -> 5035.2s (5.7%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5b74275): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.9%, secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
-1.9% [-2.4%, -1.3%] 5
Improvements ✅
(secondary)
-2.6% [-6.8%, -1.0%] 23
All ❌✅ (primary) -1.9% [-2.4%, -1.3%] 5

Cycles

Results (secondary 4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.2% [2.0%, 10.5%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.9% [-3.9%, -3.9%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 692.261s -> 692.705s (0.06%)
Artifact size: 371.96 MiB -> 372.00 MiB (0.01%)

@GuillaumeGomez GuillaumeGomez deleted the specialize-tostring-on-128-integers branch June 20, 2025 07:53
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jul 3, 2025
…n-128-integers, r=tgross35

Use a distinct `ToString` implementation for `u128` and `i128`

Part of rust-lang#135543.

Follow-up of rust-lang#136264.

When working on rust-lang#142098, I realized that `i128` and `u128` could also benefit from a distinct `ToString` implementation so here it.

The last commit is just me realizing that I forgot to add the format tests for `usize` and `isize`.

Here is the bench comparison:

| bench name | last nightly | with this PR | diff |
|-|-|-|-|
| bench_i128 | 29.25 ns/iter (+/- 0.66) | 17.52 ns/iter (+/- 0.7) | -40.1% |
| bench_u128 | 34.06 ns/iter (+/- 0.21) | 16.1 ns/iter (+/- 0.6) | -52.7% |

I used this code to test:

```rust
#![feature(test)]

extern crate test;

use test::{Bencher, black_box};

#[inline(always)]
fn convert_to_string<T: ToString>(n: T) -> String {
    n.to_string()
}

macro_rules! decl_benches {
    ($($name:ident: $ty:ident,)+) => {
        $(
	    #[bench]
            fn $name(c: &mut Bencher) {
                c.iter(|| convert_to_string(black_box({ let nb: $ty = 20; nb })));
            }
	)+
    }
}

decl_benches! {
    bench_u128: u128,
    bench_i128: i128,
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants