Skip to content
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

Display of integers without unsafe pointer #135265

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pascaldekloe
Copy link

The benchmarks as is measure formatting speed of literals. The first commit black_box-es input to simulate runtime speed instead.

The second commit replaces unsafe pointer optimizations with plain array indices. The performance is equivalent on Apple M1. Needs peer review on Intel.

Happy to do the 128-bit version too if such change is welcome.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Noratrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Jan 8, 2025
@pascaldekloe pascaldekloe changed the title Fmt int speed Display of integers without unsafe pointer Jan 8, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

r? tgross35 since I'm doing some work in this area anyway

You should cd src/etc/test-float-parse and run cargo +stage0 run --release, those tests are technically for parsing but do invoke the print routines quite a bit. I'll take a closer look at the changes soon.

@rustbot rustbot assigned tgross35 and unassigned Noratrieb Jan 8, 2025
@pascaldekloe
Copy link
Author

No problem if this one gets rejected in favour of anything else @tgross35. I was just pleased to see the compiler doing this well.

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

You should cd src/etc/test-float-parse and run cargo +stage0 run --release, those tests are technically for parsing but do invoke the print routines quite a bit. I'll take a closer look at the changes soon.

Actually ignore this, I was thinking this was in flt2dec and not integers.

No problem if this one gets rejected in favour of anything else @tgross35. I was just pleased to see the compiler doing this well.

No conflict :) Replacing unsafe code is always nice if there are no drawbacks.

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

It looks like there may be some small regressions here, the new code doesn't elide a couple calls to panic_bounds_check https://rust.godbolt.org/z/hjqhqsnbb. You could probably play around on godbolt a bit to figure out where this is coming from, a call to hint::assert_unchecked in the right place would likely make this disappear.

Soon it will be possible to write_copy_of_slice on the MaybeUninit slices, I don't expect this to do anything for optimization but it would allow keeping the "write two bytes at a time" feature from the original #129259.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants