Improve `char::is_ascii_*` codegen #67585

ranma42 · 2019-12-24T14:04:54Z

This PR is an attempt to fix #65127

A couple of warnings:

the generated code might be further improved (in LLVM and/or MIR) by emitting better comparison sequences; in particular, this would improve the performance of "complex" checks such as those in is_ascii_punctuation
the second commit is currently marked "DO NOT MERGE", because it regresses SIMD on u8 slices; this could likely be fixed by improving the computation/usage of demanded bits in LLVM

An alternative approach to remove the code duplication might be the use of macros, but currently most of the duplication is actually in the doc comments, so maybe just keeping the redundancy could be ok

rust-highfive · 2019-12-24T14:05:05Z

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

ranma42 · 2019-12-24T14:11:25Z

Some benchmark numbers:

tartaros:rust ranma42$ rustc -O is_ascii.rs --test
tartaros:rust ranma42$ ./is_ascii --bench

running 22 tests
test is_ascii::byte_bench              ... bench:          65 ns/iter (+/- 6)
test is_ascii::char_bench              ... bench:          63 ns/iter (+/- 5)
test is_ascii_alphabetic::byte_bench   ... bench:          81 ns/iter (+/- 9)
test is_ascii_alphabetic::char_bench   ... bench:         125 ns/iter (+/- 23)
test is_ascii_alphanumeric::byte_bench ... bench:         114 ns/iter (+/- 8)
test is_ascii_alphanumeric::char_bench ... bench:         110 ns/iter (+/- 11)
test is_ascii_control::byte_bench      ... bench:          94 ns/iter (+/- 8)
test is_ascii_control::char_bench      ... bench:         105 ns/iter (+/- 50)
test is_ascii_digit::byte_bench        ... bench:          70 ns/iter (+/- 2)
test is_ascii_digit::char_bench        ... bench:          95 ns/iter (+/- 10)
test is_ascii_graphic::byte_bench      ... bench:          69 ns/iter (+/- 6)
test is_ascii_graphic::char_bench      ... bench:         100 ns/iter (+/- 3)
test is_ascii_hexdigit::byte_bench     ... bench:         118 ns/iter (+/- 10)
test is_ascii_hexdigit::char_bench     ... bench:         137 ns/iter (+/- 5)
test is_ascii_lowercase::byte_bench    ... bench:          70 ns/iter (+/- 10)
test is_ascii_lowercase::char_bench    ... bench:          94 ns/iter (+/- 11)
test is_ascii_punctuation::byte_bench  ... bench:         282 ns/iter (+/- 33)
test is_ascii_punctuation::char_bench  ... bench:         366 ns/iter (+/- 17)
test is_ascii_uppercase::byte_bench    ... bench:          70 ns/iter (+/- 4)
test is_ascii_uppercase::char_bench    ... bench:         111 ns/iter (+/- 16)
test is_ascii_whitespace::byte_bench   ... bench:          85 ns/iter (+/- 6)
test is_ascii_whitespace::char_bench   ... bench:          85 ns/iter (+/- 12)

test result: ok. 0 passed; 0 failed; 0 ignored; 22 measured; 0 filtered out

tartaros:rust ranma42$ ./rust/build/x86_64-apple-darwin/stage2/bin/rustc -O is_ascii.rs --test
tartaros:rust ranma42$ ./is_ascii --bench

running 22 tests
test is_ascii::byte_bench              ... bench:          63 ns/iter (+/- 8)
test is_ascii::char_bench              ... bench:          63 ns/iter (+/- 7)
test is_ascii_alphabetic::byte_bench   ... bench:          81 ns/iter (+/- 8)
test is_ascii_alphabetic::char_bench   ... bench:          81 ns/iter (+/- 7)
test is_ascii_alphanumeric::byte_bench ... bench:         116 ns/iter (+/- 8)
test is_ascii_alphanumeric::char_bench ... bench:         116 ns/iter (+/- 9)
test is_ascii_control::byte_bench      ... bench:          94 ns/iter (+/- 2)
test is_ascii_control::char_bench      ... bench:          93 ns/iter (+/- 7)
test is_ascii_digit::byte_bench        ... bench:          70 ns/iter (+/- 2)
test is_ascii_digit::char_bench        ... bench:          69 ns/iter (+/- 3)
test is_ascii_graphic::byte_bench      ... bench:          69 ns/iter (+/- 2)
test is_ascii_graphic::char_bench      ... bench:          70 ns/iter (+/- 2)
test is_ascii_hexdigit::byte_bench     ... bench:         135 ns/iter (+/- 21)
test is_ascii_hexdigit::char_bench     ... bench:         124 ns/iter (+/- 18)
test is_ascii_lowercase::byte_bench    ... bench:          73 ns/iter (+/- 5)
test is_ascii_lowercase::char_bench    ... bench:          70 ns/iter (+/- 2)
test is_ascii_punctuation::byte_bench  ... bench:         298 ns/iter (+/- 14)
test is_ascii_punctuation::char_bench  ... bench:         274 ns/iter (+/- 44)
test is_ascii_uppercase::byte_bench    ... bench:          69 ns/iter (+/- 7)
test is_ascii_uppercase::char_bench    ... bench:          69 ns/iter (+/- 5)
test is_ascii_whitespace::byte_bench   ... bench:          86 ns/iter (+/- 10)
test is_ascii_whitespace::char_bench   ... bench:          72 ns/iter (+/- 3)

test result: ok. 0 passed; 0 failed; 0 ignored; 22 measured; 0 filtered out

Please do not take these measurement as absolute as they suffer from 2 limitations:

are on single elements (and SIMD codegen is affected by this change as shown in https://godbolt.org/z/sK6rKz )
are only on valid ASCII codepoints and uniformly distributed on each of them, while in the real world the frequencies are likely to be very skewed

joshtriplett · 2019-12-25T05:25:50Z

Nice improvement.

Do you have a plan to improve the second commit?

ranma42 · 2019-12-25T23:57:50Z

I would love some guidance regarding the second commit. What is the preferred direction?
Accept duplication in the code (just like in the docs)?
Re-use the ranges by means of a macro?

JohnCSimon · 2020-01-18T22:37:22Z

Ping from triage:
@ranma42 can you post your status on this PR or ping others for additional help? Thanks.

ranma42 · 2020-01-19T17:43:53Z

@joshtriplett short term I guess the safest option is to drop the second commit and accept the duplication (the code is trivial, the duplication in the doc comments is actually more heavy-weight than the implementation code). Would that be ok?

Mark-Simulacrum · 2020-01-20T16:07:30Z

Yes, I think the duplication is fine for now.

ranma42 · 2020-01-24T23:11:23Z

This is still marked as S-waiting-on-author.
Should I do something or just ping @joshtriplett ?

ranma42 · 2020-02-01T12:08:31Z

I am guessing it is being ignored because of the tags.
@JohnCSimon could you update them?

Dylan-DPC-zz · 2020-02-10T14:23:14Z

r? @KodrAus

bors · 2020-02-11T02:12:25Z

☔ The latest upstream changes (presumably #69030) made this pull request unmergeable. Please resolve the merge conflicts.

These methods explicitly check if a char is in a specific ASCII range, therefore the `is_ascii()` check is not needed, but LLVM seems to be unable to remove it. WARNING: this change improves the performance on ASCII `char`s, but complex checks such as `is_ascii_punctuation` become slower on non-ASCII `char`s.

ranma42 · 2020-02-11T09:24:10Z

rebased to resolve merge conflicts

Dylan-DPC-zz · 2020-02-12T00:14:21Z

r? @Amanieu

Amanieu · 2020-02-12T11:05:01Z

There is a way to improve performance even further, at the cost of uglier code. If you need to check both uppercase and lowercase, you can clear bit 6 (0x20) of the character code to convert to uppercase and then do a single range check on the uppercase value.

So for example, is_ascii_alphabetic could be implemented as 'A'..='Z'.contains(c & !0x20).

ranma42 · 2020-02-12T11:15:11Z

Yup, with further tweaking it might be possible to improve the performance.
This PR mainly aims at making char and u8 be on par.

Amanieu · 2020-02-12T11:47:23Z

Well, let's leave that for a future PR then.

@bors r+

bors · 2020-02-12T11:47:24Z

📌 Commit 4e7aeaf has been approved by Amanieu

…r=Amanieu Improve `char::is_ascii_*` codegen This PR is an attempt to fix rust-lang#65127 A couple of warnings: 1. the generated code might be further improved (in LLVM and/or MIR) by emitting better comparison sequences; in particular, this would improve the performance of "complex" checks such as those in `is_ascii_punctuation` 2. the second commit is currently marked "DO NOT MERGE", because it regresses SIMD on `u8` slices; this could likely be fixed by improving the computation/usage of demanded bits in LLVM An alternative approach to remove the code duplication might be the use of macros, but currently most of the duplication is actually in the doc comments, so maybe just keeping the redundancy could be ok

@ghost

Rollup of 8 pull requests Successful merges: - #67585 (Improve `char::is_ascii_*` codegen) - #68914 (Speed up `SipHasher128`.) - #68994 (rustbuild: include channel in sanitizers installed name) - #69032 (ICE in nightly-2020-02-08: handle TerminatorKind::Yield in librustc_mir::transform::promote_consts::Validator method) - #69034 (parser: Remove `Parser::prev_token_kind`) - #69042 (Remove backtrace header text) - #69059 (Remove a few unused objects) - #69089 (Properly use the darwin archive format on Apple targets) Failed merges: r? @ghost

rust-highfive assigned joshtriplett Dec 24, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 24, 2019

JohnCSimon 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 Jan 18, 2020

ranma42 force-pushed the fix/char-is-ascii-codegen branch from 7e6e0be to 84435dd Compare January 21, 2020 00:14

ranma42 marked this pull request as ready for review January 21, 2020 00:15

This comment has been minimized.

Sign in to view

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 Feb 1, 2020

rust-highfive assigned KodrAus and unassigned joshtriplett Feb 10, 2020

ranma42 force-pushed the fix/char-is-ascii-codegen branch from 84435dd to 4e7aeaf Compare February 11, 2020 09:23

rust-highfive assigned Amanieu and unassigned KodrAus Feb 12, 2020

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 Feb 12, 2020

Dylan-DPC-zz mentioned this pull request Feb 12, 2020

Rollup of 8 pull requests #69094

Merged

bors merged commit 4e7aeaf into rust-lang:master Feb 12, 2020

LeSeulArtichaut mentioned this pull request Feb 19, 2020

char::is_ascii_digit() is slower than char::is_digit(10) #68453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `char::is_ascii_*` codegen #67585

Improve `char::is_ascii_*` codegen #67585

ranma42 commented Dec 24, 2019

rust-highfive commented Dec 24, 2019

ranma42 commented Dec 24, 2019

joshtriplett commented Dec 25, 2019

ranma42 commented Dec 25, 2019

JohnCSimon commented Jan 18, 2020

ranma42 commented Jan 19, 2020

Mark-Simulacrum commented Jan 20, 2020

ranma42 commented Jan 24, 2020

ranma42 commented Feb 1, 2020

This comment has been minimized.

Dylan-DPC-zz commented Feb 10, 2020

bors commented Feb 11, 2020

ranma42 commented Feb 11, 2020

Dylan-DPC-zz commented Feb 12, 2020

Amanieu commented Feb 12, 2020

ranma42 commented Feb 12, 2020

Amanieu commented Feb 12, 2020

bors commented Feb 12, 2020

Improve char::is_ascii_* codegen #67585

Improve char::is_ascii_* codegen #67585

Conversation

ranma42 commented Dec 24, 2019

rust-highfive commented Dec 24, 2019

ranma42 commented Dec 24, 2019

joshtriplett commented Dec 25, 2019

ranma42 commented Dec 25, 2019

JohnCSimon commented Jan 18, 2020

ranma42 commented Jan 19, 2020

Mark-Simulacrum commented Jan 20, 2020

ranma42 commented Jan 24, 2020

ranma42 commented Feb 1, 2020

This comment has been minimized.

Dylan-DPC-zz commented Feb 10, 2020

bors commented Feb 11, 2020

ranma42 commented Feb 11, 2020

Dylan-DPC-zz commented Feb 12, 2020

Amanieu commented Feb 12, 2020

ranma42 commented Feb 12, 2020

Amanieu commented Feb 12, 2020

bors commented Feb 12, 2020

Improve `char::is_ascii_*` codegen #67585

Improve `char::is_ascii_*` codegen #67585