Skip to content

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

Merged
bors merged 2 commits intorust-lang:masterfrom
the8472:chars_advance
Nov 28, 2023
Merged

optimize str::iter::Chars::advance_by#115331
bors merged 2 commits intorust-lang:masterfrom
the8472:chars_advance

Conversation

@the8472
Copy link
Member

@the8472 the8472 commented Aug 29, 2023 •
edited
Loading 

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
mohe2015
mohe2015 reviewed Aug 29, 2023
View reviewed changes
mohe2015
mohe2015 reviewed Aug 29, 2023
View reviewed changes
@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
Collaborator

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

@scottmcm scottmcm Aug 29, 2023

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
Loading

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
Collaborator

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
Collaborator

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
@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

@scottmcm scottmcm scottmcm left review comments

+1 more reviewer

@mohe2015 mohe2015 mohe2015 left review comments

Reviewers whose approvals may not affect merge requirements

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.

9 participants

@the8472 @rust-timer @bors @rust-log-analyzer @cuviper @mohe2015 @scottmcm @rustbot @Dylan-DPC