Skip to content

Faster fmt::Display of 128-bit integers, without unsafe pointer #136594

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 12, 2025
Merged

Faster fmt::Display of 128-bit integers, without unsafe pointer #136594

merged 2 commits into from
Jun 12, 2025

Conversation

pascaldekloe
Copy link
Contributor

In followup of #135265, hereby the 128-bit part.

  • Batches per 16 instead of 19 digits
  • Buffer access as array insteaf of unsafe pointer
  • Added test coverage for i128 and u128

r? tgross35 ChrisDenton

@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 Feb 5, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pascaldekloe
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Feb 5, 2025

@pascaldekloe: 🔑 Insufficient privileges: not in try users

@tgross35
Copy link
Contributor

tgross35 commented Feb 5, 2025

Do you have a link to the asm diff?

@bors
Copy link
Collaborator

bors commented Feb 6, 2025

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

@pascaldekloe
Copy link
Contributor Author

Do you have a link to the asm diff?

https://rust.godbolt.org/z/6Pq3hKT8d

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pascaldekloe
Copy link
Contributor Author

Constant calculation done with https://gist.github.com/pascaldekloe/df103c17ebf01920958053c76505aedf.

@tgross35
Copy link
Contributor

I don't have any concerns here but the tests need to be fixed of course :)

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pascaldekloe
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2025
@bors
Copy link
Collaborator

bors commented Jun 7, 2025

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

tgross35
tgross35 reviewed Jun 10, 2025
View reviewed changes
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.

Hey, I'm very sorry this PR kept falling off my todo list. I have now had a chance to read through thoroughly and it looks great. Left a few small comments, but with those, a rebase, and a final try job I think this should be good to go.

Any chance you could post local benchmarks as well?

@tgross35
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2025
@bors
Copy link
Collaborator

bors commented Jun 10, 2025

⌛ Trying commit 0483c93 with merge f791aa5...

bors added a commit that referenced this pull request Jun 10, 2025
@bors
Auto merge of #136594 - pascaldekloe:fmt-int128, r=<try>
f791aa5 
Faster fmt::Display of 128-bit integers, without unsafe pointer

In followup of #135265, hereby the 128-bit part.

* Batches per 16 instead of 19 digits
* Buffer access as array insteaf of unsafe pointer
* Added test coverage for i128 and u128

r? tgross35 ChrisDenton
@pascaldekloe
Copy link
Contributor Author

No problem @tgross35. This is no priority patch. Happy with these feedback loops. Getting to learn the language. Local benchmark will follow in an hour or so...

@bors
Copy link
Collaborator

bors commented Jun 10, 2025

☀️ Try build successful - checks-actions
Build commit: f791aa5 (f791aa5b7459654ec84452793d1bea1ddf0e6b32)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f791aa5): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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.4% [-0.4%, -0.4%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 0.8%)

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)		 8.5% [8.5%, 8.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.8% [-2.9%, -1.0%] 3
All ❌✅ (primary) - - 0

Cycles

Results (secondary 2.7%)

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.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

Results (primary -0.0%)

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)		 - - 0
Improvements ✅
(primary)		 -0.0% [-0.0%, -0.0%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.0% [-0.0%, -0.0%] 1

Bootstrap: 754.321s -> 755.157s (0.11%)
Artifact size: 372.15 MiB -> 372.14 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2025
@tgross35
Copy link
Contributor

One final comment request #136594 (comment) and it would be good if you could post benchmarks and/or an asm diff. After that, r=me

@pascaldekloe
Copy link
Contributor Author

The following was measured locally on Apple M1 @tgross35.

Master

    fmt::write_u128_max                                                   51.44ns/iter      +/- 0.44
    fmt::write_u128_min                                                   30.63ns/iter      +/- 0.92
    fmt::write_u64_max                                                    36.00ns/iter      +/- 0.55
    fmt::write_u64_min                                                    27.72ns/iter      +/- 0.20
    fmt::write_u8_max                                                     28.98ns/iter      +/- 0.34
    fmt::write_u8_min                                                     27.72ns/iter      +/- 0.38

Change

    fmt::write_u128_max                                                   46.65ns/iter     +/- 0.39
    fmt::write_u128_min                                                   26.77ns/iter     +/- 0.58
    fmt::write_u64_max                                                    35.68ns/iter     +/- 0.39
    fmt::write_u64_min                                                    27.72ns/iter     +/- 0.75
    fmt::write_u8_max                                                     28.71ns/iter     +/- 0.08
    fmt::write_u8_min                                                     27.39ns/iter     +/- 0.32

@tgross35
Copy link
Contributor

Thank you for all the work here!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 11, 2025

📌 Commit 042a271 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 11, 2025
@bors
Copy link
Collaborator

bors commented Jun 12, 2025

⌛ Testing commit 042a271 with merge 1434630...

@bors
Copy link
Collaborator

bors commented Jun 12, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2025
@bors bors merged commit 1434630 into rust-lang:master Jun 12, 2025
11 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 12, 2025
@github-actions GitHub Actions
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 31d0d21 (parent) -> 1434630 (this PR)

Test differences

Show 5 test diffs

Stage 1

  • fmt::num::test_format_int: pass -> [missing] (J0)
  • fmt::num::test_format_int_limits: [missing] -> pass (J0)
  • fmt::num::test_format_int_misc: [missing] -> pass (J0)
  • fmt::num::test_format_int_one: [missing] -> pass (J0)
  • fmt::num::test_format_int_twos_complement: pass -> [missing] (J0)

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 14346303d760027e53214e705109a62c0f00b214 --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. x86_64-apple-1: 7215.7s -> 9403.1s (30.3%)
  2. dist-x86_64-apple: 8674.0s -> 10174.7s (17.3%)
  3. mingw-check-1: 1656.4s -> 1911.2s (15.4%)
  4. aarch64-apple: 5855.4s -> 5214.8s (-10.9%)
  5. i686-gnu-1: 7603.4s -> 8357.3s (9.9%)
  6. x86_64-gnu-llvm-20-1: 3304.6s -> 3627.0s (9.8%)
  7. i686-gnu-2: 5685.3s -> 6230.8s (9.6%)
  8. armhf-gnu: 4535.6s -> 4966.5s (9.5%)
  9. x86_64-rust-for-linux: 2648.9s -> 2853.9s (7.7%)
  10. x86_64-gnu-debug: 5580.8s -> 5992.3s (7.4%)
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.

@bors bors mentioned this pull request Jun 12, 2025
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1434630): comparison URL.

Overall result: ❌ regressions - 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.2% [0.1%, 0.3%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 1.5%)

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)		 1.5% [1.5%, 1.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary 6.6%)

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)		 6.6% [6.6%, 6.6%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

Results (primary -0.0%)

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)		 - - 0
Improvements ✅
(primary)		 -0.0% [-0.0%, -0.0%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.0% [-0.0%, -0.0%] 1

Bootstrap: 754.746s -> 755.927s (0.16%)
Artifact size: 372.10 MiB -> 372.13 MiB (0.01%)

tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jun 17, 2025
@bors
Auto merge of rust-lang#136594 - pascaldekloe:fmt-int128, r=tgross35
1a90510 
Faster fmt::Display of 128-bit integers, without unsafe pointer

In followup of rust-lang#135265, hereby the 128-bit part.

* Batches per 16 instead of 19 digits
* Buffer access as array insteaf of unsafe pointer
* Added test coverage for i128 and u128

r? tgross35 ChrisDenton
@hanna-kruppe hanna-kruppe mentioned this pull request Jul 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

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
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.
6 participants
@pascaldekloe @rust-log-analyzer @rust-timer @bors @tgross35 @rustbot