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.

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

optimize str::iter::Chars::advance_by #115331

Merged
merged 2 commits into from Nov 28, 2023
Merged

optimize str::iter::Chars::advance_by #115331

merged 2 commits into from Nov 28, 2023

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Aug 29, 2023 •
edited 

OLD: 
    str::iter::chars_advance_by_0001  0.00ns/iter  +/- 0.00ns
    str::iter::chars_advance_by_0010 13.00ns/iter  +/- 1.00ns
    str::iter::chars_advance_by_1000  1.20µs/iter +/- 15.00ns

NEW:
    str::iter::chars_advance_by_0001  0.00ns/iter +/- 0.00ns
    str::iter::chars_advance_by_0010  6.00ns/iter +/- 0.00ns
    str::iter::chars_advance_by_1000 75.00ns/iter +/- 1.00ns

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 29, 2023
@the8472
Copy link
Member Author

the8472 commented Aug 29, 2023

@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 Aug 29, 2023
@bors
Copy link
Contributor

bors commented Aug 29, 2023

⌛ Trying commit 41fc4b9 with merge 29c876884857a05f378862248faf8485bd98f05d...

scottmcm
scottmcm reviewed Aug 29, 2023
View reviewed changes
compiler/rustc_lexer/src/cursor.rs Outdated
{
// SAFETY: we checked it's not EOF therefore there must be
// at least 1 char present.
unsafe { self.chars.advance_by(1).unwrap_unchecked() };
Copy link
Member

Choose a reason for hiding this comment

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

YMMV: Given that we've seen unwrap_unchecked not actually remove the branches on slice iterators, consider implementing https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/core/iter/traits/unchecked_iterator/trait.UncheckedIterator.html and having this use that. (And maybe have next call it too.)

Copy link
Member Author

@the8472 the8472 Aug 29, 2023 •
edited

Choose a reason for hiding this comment

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

unwrap_unchecked does appear to work here.

https://rust.godbolt.org/z/3eod9e6je

Edit: Well, it does help inside advance_by. I have not yet tested whether it helps on advance_by 😅

@bors
Copy link
Contributor

bors commented Aug 29, 2023

☀️ Try build successful - checks-actions
Build commit: 29c876884857a05f378862248faf8485bd98f05d (29c876884857a05f378862248faf8485bd98f05d)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

This comment was marked as outdated.

Sign in to view
@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 29, 2023
@Dylan-DPC Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2023
@the8472 the8472 removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Nov 27, 2023
@the8472 the8472 marked this pull request as ready for review November 27, 2023 20:58
@the8472 the8472 changed the title optimize str::iter::Chars bumping optimize str::iter::Chars::advance_by Nov 27, 2023
@the8472
Copy link
Member Author

the8472 commented Nov 27, 2023

r? libs

@the8472 the8472 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
benchmarks for Chars::advance_by
3f55e86
@the8472
optimize str::iter::Chars::advance_by
40cf1f9 
this avoids part of the char decoding work by not looking at utf8 continuation bytes
@cuviper
Copy link
Member

cuviper commented Nov 28, 2023

FWIW, I couldn't reproduce such good results on 1000 until I set CGU=1, but that's how we ship std anyway.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 28, 2023

📌 Commit 40cf1f9 has been approved by cuviper

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 Nov 28, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 28, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 28, 2023
@bors
Auto merge of rust-lang#118405 - matthiaskrgr:rollup-3a2eevc, r=matth…
46a24ed 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#115331 (optimize str::iter::Chars::advance_by)
 - rust-lang#118236 (Update mod comment)
 - rust-lang#118299 (Update `OnceLock` documentation to give a concrete 'lazy static' example, and expand on the existing example.)
 - rust-lang#118314 (Rename `{collections=>alloc}{tests,benches}`)
 - rust-lang#118341 (Simplify indenting in THIR printing)
 - rust-lang#118366 (Detect and reject malformed `repr(Rust)` hints)
 - rust-lang#118397 (Fix comments for unsigned non-zero `checked_add`, `saturating_add`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4af1f99 into rust-lang:master Nov 28, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 28, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 28, 2023
@rust-timer
Unrolled build for rust-lang#115331
69f8605 
Rollup merge of rust-lang#115331 - the8472:chars_advance, r=cuviper

optimize str::iter::Chars::advance_by

```
OLD:
    str::iter::chars_advance_by_0001  0.00ns/iter  +/- 0.00ns
    str::iter::chars_advance_by_0010 13.00ns/iter  +/- 1.00ns
    str::iter::chars_advance_by_1000  1.20µs/iter +/- 15.00ns

NEW:
    str::iter::chars_advance_by_0001  0.00ns/iter +/- 0.00ns
    str::iter::chars_advance_by_0010  6.00ns/iter +/- 0.00ns
    str::iter::chars_advance_by_1000 75.00ns/iter +/- 1.00ns
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohe2015 mohe2015 mohe2015 left review comments

@scottmcm scottmcm scottmcm left review comments

Assignees

@cuviper cuviper

Labels
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.76.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@the8472 @rust-timer @bors @rust-log-analyzer @cuviper @mohe2015 @scottmcm @rustbot @Dylan-DPC