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

Slight perf improvement on char::to_ascii_lowercase #81837

Merged
merged 5 commits into from
Feb 23, 2021
Merged

Slight perf improvement on char::to_ascii_lowercase #81837

merged 5 commits into from
Feb 23, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Feb 6, 2021 •
edited
Loading

char::to_ascii_lowercase() was checking if it was ascii and then if it was in the right range. Instead propose to check once (I think removing a compare and a shift in the process: godbolt ).

before:

        test char::methods::bench_to_ascii_lowercase                    ... bench:      11,196 ns/iter (+/- 632)
        test char::methods::bench_to_ascii_uppercase                    ... bench:      11,656 ns/iter (+/- 671)

after:

         test char::methods::bench_to_ascii_lowercase                    ... bench:       9,612 ns/iter (+/- 979)
         test char::methods::bench_to_ascii_uppercase                    ... bench:       8,241 ns/iter (+/- 701)

(calling u8::to_ascii_lowercase and letting that flip the 5th bit is also an option, but it's more instructions. I'm thinking for things around ascii and char we want to be as efficient as possible.)

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@gilescope gilescope mentioned this pull request Feb 7, 2021
Merged
@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue. A-Unicode Area: Unicode labels Feb 7, 2021
dtolnay
dtolnay approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This lgtm but from the last comment it sounds like you are still working on it. Let me know when ready for review.

@dtolnay
Copy link
Member

dtolnay commented Feb 8, 2021

@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author

@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 Feb 8, 2021
@gilescope
Copy link
Contributor Author

@dtolnay I think the dust is setttled on this one and is ready for review.

@dtolnay
Copy link
Member

dtolnay commented Feb 19, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2021

📌 Commit 33d8b04 has been approved by dtolnay

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 20, 2021
@Dylan-DPC
Rollup merge of rust-lang#81837 - gilescope:to_ascii_speedups, r=dtolnay
6468333 
Slight perf improvement on char::to_ascii_lowercase

`char::to_ascii_lowercase()` was checking if it was ascii and then if it was in the right range. Instead propose to check once (I think removing a compare and a shift in the process: [godbolt](https://godbolt.org/z/e5Tora) ).

before:
```
        test char::methods::bench_to_ascii_lowercase                    ... bench:      11,196 ns/iter (+/- 632)
        test char::methods::bench_to_ascii_uppercase                    ... bench:      11,656 ns/iter (+/- 671)
```
after:
```
         test char::methods::bench_to_ascii_lowercase                    ... bench:       9,612 ns/iter (+/- 979)
         test char::methods::bench_to_ascii_uppercase                    ... bench:       8,241 ns/iter (+/- 701)
```

(calling u8::to_ascii_lowercase and letting that flip the 5th bit is also an option, but it's more instructions. I'm thinking for things around ascii and char we want to be as efficient as possible.)
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 20, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 23, 2021
@Dylan-DPC
Rollup merge of rust-lang#81837 - gilescope:to_ascii_speedups, r=dtolnay
39d841d 
Slight perf improvement on char::to_ascii_lowercase

`char::to_ascii_lowercase()` was checking if it was ascii and then if it was in the right range. Instead propose to check once (I think removing a compare and a shift in the process: [godbolt](https://godbolt.org/z/e5Tora) ).

before:
```
        test char::methods::bench_to_ascii_lowercase                    ... bench:      11,196 ns/iter (+/- 632)
        test char::methods::bench_to_ascii_uppercase                    ... bench:      11,656 ns/iter (+/- 671)
```
after:
```
         test char::methods::bench_to_ascii_lowercase                    ... bench:       9,612 ns/iter (+/- 979)
         test char::methods::bench_to_ascii_uppercase                    ... bench:       8,241 ns/iter (+/- 701)
```

(calling u8::to_ascii_lowercase and letting that flip the 5th bit is also an option, but it's more instructions. I'm thinking for things around ascii and char we want to be as efficient as possible.)
This was referenced Feb 23, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
@bors
Auto merge of rust-lang#82430 - Dylan-DPC:rollup-nu4kfyc, r=Dylan-DPC
a4e595d 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#79423 (Enable smart punctuation)
 - rust-lang#81154 (Improve design of `assert_len`)
 - rust-lang#81235 (Improve suggestion for tuple struct pattern matching errors.)
 - rust-lang#81769 (Suggest `return`ing tail expressions that match return type)
 - rust-lang#81837 (Slight perf improvement on char::to_ascii_lowercase)
 - rust-lang#81969 (Avoid `cfg_if` in `std::os`)
 - rust-lang#81984 (Make WASI's `hard_link` behavior match other platforms.)
 - rust-lang#82091 (use PlaceRef abstractions more consistently)
 - rust-lang#82128 (add diagnostic items for OsString/PathBuf/Owned as well as to_vec on slice)
 - rust-lang#82166 (add s390x-unknown-linux-musl target)
 - rust-lang#82234 (Remove query parameters when skipping search results)
 - rust-lang#82255 (Make `treat_err_as_bug` Option<NonZeroUsize>)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4af965e into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@pickfire
Copy link
Contributor

Do we need not to do a perf run for this?

@lopopolo lopopolo mentioned this pull request Feb 23, 2021
Merged
@illicitonion illicitonion mentioned this pull request May 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pickfire pickfire pickfire left review comments

@bluss bluss bluss left review comments

@fbstj fbstj fbstj left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
A-Unicode Area: Unicode I-slow Issue: Problems and improvements with respect to performance of generated code. 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.52.0
Development

Successfully merging this pull request may close these issues.
10 participants
@gilescope @rust-highfive @rust-log-analyzer @dtolnay @bors @pickfire @fbstj @bluss @jyn514 @rustbot