Skip to content

Optimize std::str::Chars::next and std::str::Chars::next_back #142038

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

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

Conversation

Kmeakin
Copy link
Contributor

@Kmeakin Kmeakin commented Jun 4, 2025

Before/after for next: https://godbolt.org/z/9Te84YzhK
Before/after for next_back: https://godbolt.org/z/fTx1a7oz1

std::sys_common::wtf8::Wtf8CodePoints will also benefit from this, since it uses the same next_code_point and next_code_point_reverse functions internally.

I also added tests for all codepoints in the range 0..=char::MAX (including surrogats that can only appear in WTF-8), so the new implementations have been exhaustively tested

@rustbot
Copy link
Collaborator

rustbot commented Jun 4, 2025

r? @scottmcm

rustbot has assigned @scottmcm.
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 4, 2025
@Kmeakin
Copy link
Contributor Author

Kmeakin commented Jul 3, 2025

ping @scottmcm ?

@scottmcm
Copy link
Member

scottmcm commented Jul 4, 2025

So you know, you can make diff views in godbolt: https://godbolt.org/z/Thn1bf9qG

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

The general structure here does make sense to me, but overall I feel like it removed a bunch of helpers and constants unnecessarily. Not having utf8_first_byte, sure, but this ends up repeating the X << 6 | (Y & 0x3F) in a bunch of places, so keeping the utf8_acc_cont_byte to do that would make sense to me. The standard library is always compiled with optimizations, and the MIR inliner will inline it, so there's no reason to avoid the function call. Having the u32::from in there would also make the two functions more similar, since now the forward one is using as u32 in a different line instead with no obvious reason whey they should differ.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 4, 2025
Kmeakin added 2 commits July 8, 2025 00:48
There are only 0x10FFFF possible codepoints, so we can exhaustively test
all of them.
By reordering some operations, we can expose some opportunites for
CSE. Also convert the series of nested `if` branches to early return,
which IMO makes the code clearer.

Comparison of assembly before and after for `next_code_point`:
https://godbolt.org/z/9Te84YzhK

Comparison of assembly before and after for `next_code_point_reverse`:
https://godbolt.org/z/fTx1a7oz1
@Kmeakin Kmeakin force-pushed the km/optimize-str-chars-iterator branch from 26b614c to 54a699b Compare July 7, 2025 23:48
@Kmeakin
Copy link
Contributor Author

Kmeakin commented Jul 7, 2025

The general structure here does make sense to me, but overall I feel like it removed a bunch of helpers and constants unnecessarily. Not having utf8_first_byte, sure, but this ends up repeating the X << 6 | (Y & 0x3F) in a bunch of places, so keeping the utf8_acc_cont_byte to do that would make sense to me. The standard library is always compiled with optimizations, and the MIR inliner will inline it, so there's no reason to avoid the function call. Having the u32::from in there would also make the two functions more similar, since now the forward one is using as u32 in a different line instead with no obvious reason whey they should differ.

I could not get LLVM to produce the movzx even with various combinations of assume and disjoint_bitor. I'll file an issue against LLVM instead of putting micro-optimizations like that in Rust

@Kmeakin
Copy link
Contributor Author

Kmeakin commented Jul 16, 2025

r? @scottmcm

@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@Kmeakin Kmeakin requested a review from scottmcm August 9, 2025 20:54
@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 Aug 9, 2025
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.

3 participants