Many is_ascii_ methods are slower than they have to be #65127

Activity

1. Dylan-DPC-zz

   transferred this issue fromrust-lang/rfcson Oct 5, 2019
2. SimonSapin commented on Oct 5, 2019

   

   SimonSapin

   on Oct 5, 2019

   Contributor

   More actions

   (I can’t seem to find the relevant part in the middle of this discord stream…)

   I’m not sure what you mean by “corrupt”. some_char as u8 is well defined: it takes the numerical value of the code point (same as some_char as u32), then truncates to keep the lower 8 bits. 'A' as u8 equals 0x41, but so does '🁁' as u8. We want to reject the latter since U+1F041 🁁 is not an ASCII upper-case letter. If char::is_ascii_uppercase is implemented in terms of u8::is_ascii_uppercase, the ASCII range check is necessary. (It could be a u8 range check to, for example with u8::try_from(u32).)

   But maybe char::is_ascii_uppercase doesn’t need to be implemented in terms of u8::is_ascii_uppercase. Maybe it could duplicate the logic instead.

   Once upon a time u8::is_ascii_uppercase was implemented with a lookup table of 256 entries. A flat lookup table for all of Unicode would be way too big. But these days it’s based on matching a u8 range pattern. Doing the same with a char range pattern could also work.

   That said, would it actually be faster? Maybe the optimizer already does its job well. If you want to work on this, consider adding some benchmark to show that code duplication is worth it.
4. BurntSushi commented on Oct 5, 2019

   

   BurntSushi

   on Oct 5, 2019

   Member

   More actions

   I think the minimal standard here should be a benchmark before we change the implementation. Can you provide that?
6. anirudhb commented on Oct 6, 2019

   

   anirudhb

   on Oct 6, 2019

   Last edited by anirudhb

   Contributor

   More actions

   Looking at the assembly, it seems that the problem is the is_ascii check.

   These three functions produce identical assembly:

   pub fn foo(c: char) -> bool {
       match c as u8 {
           65..=90 => true,
           _ => false
       }
   }
   
   pub fn bar(c: char) -> bool {
       match c {
           'A'..='Z' => true,
           _ => false,
       }
   }
   
   pub fn baz(c: char) -> bool {
       (c as u8).is_ascii_uppercase()
   }

   So, it seems that the is_ascii check is unnecessary.

   I could make a PR for this.

   ❤️React with ❤️1Reacted by Marcel Hellwig
7. anirudhb commented on Oct 6, 2019

   

   anirudhb

   on Oct 6, 2019

   Contributor

   More actions

   Here are some benchmarks:

   #![feature(test)]
   
   extern crate test;
   use test::Bencher;
   
   #[inline(always)]
   fn custom_ascii_uppercase_without_ascii_check(c: char) -> bool {
       match c {
           'A'..='Z' => true,
           _ => false,
       }
   }
   
   #[inline(always)]
   fn custom_ascii_uppercase_with_ascii_check_builtin_is_ascii(c: char) -> bool {
       c.is_ascii() && match c {
           'A'..='Z' => true,
           _ => false,
       }
   }
   
   #[inline(always)]
   fn custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u8(c: char) -> bool {
       c as u8 <= 0x7fu8 && match c {
           'A'..='Z' => true,
           _ => false,
       }
   }
   
   #[inline(always)]
   fn custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u32(c: char) -> bool {
       c as u32 <= 0x7fu32 && match c {
           'A'..='Z' => true,
           _ => false,
       }
   }
   
   #[bench]
   fn bench_custom_ascii_uppercase_without_ascii_check(b: &mut Bencher) {
       let x = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
       b.iter(|| {
           for c in x.chars() {
               test::black_box(custom_ascii_uppercase_without_ascii_check(c));
           }
       });
   }
   
   #[bench]
   fn bench_custom_ascii_uppercase_with_ascii_check_builtin_is_ascii(b: &mut Bencher) {
       let x = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
       b.iter(|| {
           for c in x.chars() {
               test::black_box(custom_ascii_uppercase_with_ascii_check_builtin_is_ascii(c));
           }
       });
   }
   
   #[bench]
   fn bench_custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u8(b: &mut Bencher) {
       let x = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
       b.iter(|| {
           for c in x.chars() {
               test::black_box(custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u8(c));
           }
       });
   }
   
   #[bench]
   fn bench_custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u32(b: &mut Bencher) {
       let x = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
       b.iter(|| {
           for c in x.chars() {
               test::black_box(custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u32(c));
           }
       });
   }
   
   #[bench]
   fn bench_builtin_ascii_uppercase(b: &mut Bencher) {
       let x = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
       b.iter(|| {
           for c in x.chars() {
               test::black_box(c.is_ascii_uppercase());
           }
       });
   }

   $ cargo bench
       Finished release [optimized] target(s) in 0.00s
        Running target/release/deps/_14178-f02baafefc742e50
   
   running 5 tests
   test bench_builtin_ascii_uppercase                                                ... bench:          79 ns/iter (+/- 11)
   test bench_custom_ascii_uppercase_with_ascii_check_builtin_is_ascii               ... bench:          76 ns/iter (+/- 13)
   test bench_custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u32 ... bench:          78 ns/iter (+/- 8)
   test bench_custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u8  ... bench:          80 ns/iter (+/- 24)
   test bench_custom_ascii_uppercase_without_ascii_check                             ... bench:          58 ns/iter (+/- 9)
   
   test result: ok. 0 passed; 0 failed; 0 ignored; 5 measured; 0 filtered out
   

   So it seems that removing the is_ascii check has a measurable performance difference.
8. SimonSapin commented on Oct 7, 2019

   

   SimonSapin

   on Oct 7, 2019

   Contributor

   More actions

   custom_ascii_uppercase_with_ascii_check_custom_is_ascii_convert_to_u8 is incorrect. The conversion should happen not for the ASCII check but for the match, and the pattern use byte literals like b'A'. (Though I expect this won’t affect results much.)
9. Alexendoo

   added

   C-enhancementCategory: An issue proposing an enhancement or a PR with one.Category: An issue proposing an enhancement or a PR with one.

   I-slowIssue: Problems and improvements with respect to performance of generated code.Issue: Problems and improvements with respect to performance of generated code.

   T-libs-api[DEPRECATED; DO NOT USE][DEPRECATED; DO NOT USE]

   on Oct 9, 2019
10. Dylan-DPC-zz

    added a commit that references this issue on Feb 12, 2020

    Rollup merge of rust-lang#67585 - ranma42:fix/char-is-ascii-codegen, …

    ...

    79ebf53
11. bors

    closed this as completedin 79ebf53on Feb 12, 2020