Optimize is_ascii_digit() and is_ascii_hexdigit() #67143

DarkKirb · 2019-12-08T09:27:22Z

The current implementation of these two functions does redundant checks
that are currently not optimized away by LLVM. The is_digit() method
on char only matches ASCII digits. It does not do the extra check of
whether the character is in ASCII range.

This change optimizes the code in both debug and release mode on various
major architectures. It removes all conditional branches on x86 and all
conditional branches and returns on ARM.

Pseudocode version of LLVM's optimized output:

old version:

if c >= 128 { return false; }
c -= 48;
if c > 9 { return false; }
true

new version:

c -= 48;
if c < 10 { return true; }
false

The is_ascii_hexdigit() change similarly shortens the emitted code in
release mode. The optimized version uses bit manipulation to remove one
comparison and conditional branch. It would be possible to add a similar
change to the is_digit() code, but that has not been done yet.

Godbolt comparison between the two implementations:

https://godbolt.org/z/gDwfSF

The current implementation of these two functions does redundant checks that are currently not optimized away by LLVM. The is_digit() method on char only matches ASCII digits. It does not do the extra check of whether the character is in ASCII range. This change optimizes the code in both debug and release mode on various major architectures. It removes all conditional branches on x86 and all conditional branches and returns on ARM. Pseudocode version of LLVM's optimized output: old version: ```rust if c >= 128 { return false; } c -= 48; if c > 9 { return false; } true ``` ```rust c -= 48; if c < 10 { return true; } false ``` The is_ascii_hexdigit() change similarly shortens the emitted code in release mode. The optimized version uses bit manipulation to remove one comparison and conditional branch. It would be possible to add a similar change to the is_digit() code, but that has not been done yet. Godbolt comparison between the two implementations: https://godbolt.org/z/gDwfSF

rust-highfive · 2019-12-08T09:27:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

I forgot to invert the constant to 0x20 after putting an inversion operator in front of it.

rust-highfive · 2019-12-08T09:41:07Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-08T09:34:50.8323339Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-08T09:34:50.8500206Z ##[command]git config gc.auto 0
2019-12-08T09:34:50.8590112Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-08T09:34:50.8643357Z ##[command]git config --get-all http.proxy
2019-12-08T09:34:50.8791238Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67143/merge:refs/remotes/pull/67143/merge
---
2019-12-08T09:39:44.8346780Z     Checking core v0.0.0 (/checkout/src/libcore)
2019-12-08T09:39:52.7691255Z error[E0061]: this function takes 1 parameter but 0 parameters were supplied
2019-12-08T09:39:52.7692474Z     --> src/libcore/char/methods.rs:1251:17
2019-12-08T09:39:52.7692939Z      |
2019-12-08T09:39:52.7693485Z 60   |     pub fn is_digit(self, radix: u32) -> bool {
2019-12-08T09:39:52.7695003Z ...
2019-12-08T09:39:52.7696157Z 1251 |         if self.is_digit() {
2019-12-08T09:39:52.7696782Z      |                 ^^^^^^^^ expected 1 parameter
2019-12-08T09:39:52.7696970Z 
2019-12-08T09:39:52.7696970Z 
2019-12-08T09:39:52.7728692Z error[E0606]: casting `&char` as `u8` is invalid
2019-12-08T09:39:52.7729750Z      |
2019-12-08T09:39:52.7729750Z      |
2019-12-08T09:39:52.7730195Z 1254 |         let code = (self as u8) & !0x20; // 0x20 is the case bit
2019-12-08T09:39:52.7731398Z      |
2019-12-08T09:39:52.7731986Z      = help: cast through a raw pointer first
2019-12-08T09:39:52.7732167Z 
2019-12-08T09:39:53.3960579Z    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
---
2019-12-08T09:39:57.0082595Z   local time: Sun Dec  8 09:39:57 UTC 2019
2019-12-08T09:39:57.2870828Z   network time: Sun, 08 Dec 2019 09:39:57 GMT
2019-12-08T09:39:57.2875246Z == end clock drift check ==
2019-12-08T09:40:10.7295533Z 
2019-12-08T09:40:10.7405103Z ##[error]Bash exited with code '1'.
2019-12-08T09:40:10.7434155Z ##[section]Starting: Checkout
2019-12-08T09:40:10.7436131Z ==============================================================================
2019-12-08T09:40:10.7436186Z Task         : Get sources
2019-12-08T09:40:10.7436250Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Centril · 2019-12-08T10:02:23Z

@bors try @rust-timer queue

rust-timer · 2019-12-08T10:02:24Z

Awaiting bors try build completion

bors · 2019-12-08T10:02:33Z

⌛ Trying commit 34f89fd with merge 0d5360f...

Optimize is_ascii_digit() and is_ascii_hexdigit() The current implementation of these two functions does redundant checks that are currently not optimized away by LLVM. The is_digit() method on char only matches ASCII digits. It does not do the extra check of whether the character is in ASCII range. This change optimizes the code in both debug and release mode on various major architectures. It removes all conditional branches on x86 and all conditional branches and returns on ARM. Pseudocode version of LLVM's optimized output: old version: ```rust if c >= 128 { return false; } c -= 48; if c > 9 { return false; } true ``` new version: ```rust c -= 48; if c < 10 { return true; } false ``` The is_ascii_hexdigit() change similarly shortens the emitted code in release mode. The optimized version uses bit manipulation to remove one comparison and conditional branch. It would be possible to add a similar change to the is_digit() code, but that has not been done yet. Godbolt comparison between the two implementations: https://godbolt.org/z/gDwfSF

bors · 2019-12-08T12:40:27Z

☀️ Try build successful - checks-azure
Build commit: 0d5360f (0d5360f705cd43953ae6a35613cdf140de944ad0)

rust-timer · 2019-12-08T12:40:29Z

Queued 0d5360f with parent e862c01, future comparison URL.

rust-timer · 2019-12-09T21:28:29Z

Finished benchmarking try commit 0d5360f, comparison URL.

sfackler · 2019-12-10T00:56:52Z

src/libcore/char/methods.rs

@@ -1245,7 +1245,14 @@ impl char {
    #[stable(feature = "ascii_ctype_on_intrinsics", since = "1.24.0")]
    #[inline]
    pub fn is_ascii_hexdigit(&self) -> bool {
-        self.is_ascii() && (*self as u8).is_ascii_hexdigit()
+        if !self.is_ascii() {


Why not change this like is_ascii_digit to self.is_digit(16)? Does that end up with worse codegen?

yes, currently is_digit(x) checks two separate alphabetic ranges instead of one bit-manipulated one, but i could change that if desired

Yeah, seems like it'd be best to fix is_digit.

JohnTitor · 2020-01-12T04:04:58Z

Ping from triage: @DarkKirb could you add the change mentioned on the review?

JohnCSimon · 2020-01-18T22:05:47Z

Pinging again from triage:
@DarkKirb could you please address the change mentioned on the review?

JohnCSimon · 2020-01-25T06:43:50Z

Pinging again from triage -
@DarkKirb - Unfortunately I'm closing this out as inactive, when you have time to address the comments, please feel free to reopen this PR.
Note: please don't push to the PR while it is closed as that prevents it from being reopened.

rust-highfive assigned sfackler Dec 8, 2019

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

fix: fixed typo in is_ascii_hexdigit()

c2434c0

I forgot to invert the constant to 0x20 after putting an inversion operator in front of it.

fix: fix compilation error in is_ascii_hexdigit()

34f89fd

sfackler reviewed Dec 10, 2019

View reviewed changes

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

ThomasdenH mentioned this pull request Jan 22, 2020

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

Closed

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 25, 2020

JohnCSimon closed this Jan 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Optimize is_ascii_digit() and is_ascii_hexdigit() #67143

Optimize is_ascii_digit() and is_ascii_hexdigit() #67143

Uh oh!

DarkKirb commented Dec 8, 2019

Uh oh!

rust-highfive commented Dec 8, 2019

Uh oh!

rust-highfive commented Dec 8, 2019

Uh oh!

Centril commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 8, 2019

Uh oh!

bors commented Dec 8, 2019

Uh oh!

bors commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 9, 2019

Uh oh!

sfackler Dec 10, 2019

Uh oh!

DarkKirb Dec 13, 2019

Uh oh!

sfackler Dec 13, 2019

Uh oh!

JohnTitor commented Jan 12, 2020

Uh oh!

JohnCSimon commented Jan 18, 2020

Uh oh!

JohnCSimon commented Jan 25, 2020

Uh oh!

Uh oh!

Optimize is_ascii_digit() and is_ascii_hexdigit() #67143

Optimize is_ascii_digit() and is_ascii_hexdigit() #67143

Uh oh!

Conversation

DarkKirb commented Dec 8, 2019

Uh oh!

rust-highfive commented Dec 8, 2019

Uh oh!

rust-highfive commented Dec 8, 2019

Uh oh!

Centril commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 8, 2019

Uh oh!

bors commented Dec 8, 2019

Uh oh!

bors commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 8, 2019

Uh oh!

rust-timer commented Dec 9, 2019

Uh oh!

sfackler Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

DarkKirb Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

sfackler Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

JohnTitor commented Jan 12, 2020

Uh oh!

JohnCSimon commented Jan 18, 2020

Uh oh!

JohnCSimon commented Jan 25, 2020

Uh oh!

Uh oh!