Optimize is_ascii for str and [u8].

[thomcc]

This optimizes the is_ascii function for [u8] and str. I've been surprised this wasn't done for a while, so I just did it.

Benchmarks comparing before/after look like:

test ascii::long_readonly::is_ascii_slice_iter_all              ... bench:         174 ns/iter (+/- 79) = 40172 MB/s
test ascii::long_readonly::is_ascii_slice_libcore               ... bench:          16 ns/iter (+/- 5) = 436875 MB/s
test ascii::medium_readonly::is_ascii_slice_iter_all            ... bench:          12 ns/iter (+/- 3) = 2666 MB/s
test ascii::medium_readonly::is_ascii_slice_libcore             ... bench:           2 ns/iter (+/- 0) = 16000 MB/s
test ascii::short_readonly::is_ascii_slice_iter_all             ... bench:           3 ns/iter (+/- 0) = 2333 MB/s
test ascii::short_readonly::is_ascii_slice_libcore              ... bench:           4 ns/iter (+/- 0) = 1750 MB/s

(Taken on a x86_64 macbook 2.9 GHz Intel Core i9 with 6 cores)

Where is_ascii_slice_iter_all is the old version, and is_ascii_slice_libcore is the new.

I tried to document the code well, so hopefully it's understandable. It has fairly exhaustive tests ensuring size/align doesn't get violated -- because miri doesn't really help a lot for this sort of code right now, I tried to debug_assert all the safety invariants I'm depending on. (Of course, none of them are required for correctness or soundness -- just allows us to test that this sort of pointer manipulation is sound and such).

Anyway, thanks. Let me know if you have questions/desired changes.

[rust-highfive]

r? @shepmaster

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

[jonas-schievink]

is_ascii_slice_libcore ... bench: 16 ns/iter (+/- 5) = 436875 MB/s

That's over 400 GB/s, are you sure that's measuring the correct thing?

[thomcc]

Good point!

[ecstatic-morse]

You should have a look at slice::align_to.

[thomcc]

@ecstatic-morse: The algorithm I'm using is different (and more efficient on most machines) than what I'd get with slice::align_to.

@jonas-schievink: Not sure I know how to debug this really -- all the timings from that module seem dodgy. Well over 30GB/s for most of them, and those also contain to_vec() of a ~6000 byte slice.

test ascii::long::is_ascii_alphabetic                           ... bench:         186 ns/iter (+/- 17) = 37580 MB/s
test ascii::long::is_ascii_alphanumeric                         ... bench:         208 ns/iter (+/- 62) = 33605 MB/s
test ascii::long::is_ascii_control                              ... bench:         238 ns/iter (+/- 89) = 29369 MB/s
test ascii::long::is_ascii_digit                                ... bench:         301 ns/iter (+/- 199) = 23222 MB/s
test ascii::long::is_ascii_graphic                              ... bench:         252 ns/iter (+/- 242) = 27738 MB/s
test ascii::long::is_ascii_hexdigit                             ... bench:         213 ns/iter (+/- 87) = 32816 MB/s
test ascii::long::is_ascii_lowercase                            ... bench:         239 ns/iter (+/- 114) = 29246 MB/s
test ascii::long::is_ascii_punctuation                          ... bench:         197 ns/iter (+/- 103) = 35482 MB/s
test ascii::long::is_ascii_uppercase                            ... bench:         188 ns/iter (+/- 34) = 37180 MB/s
test ascii::long::is_ascii_whitespace                           ... bench:         207 ns/iter (+/- 195) = 33768 MB/s

[ecstatic-morse]

Simple code is good code, and people running ARM may also want to check whether their strings are ASCII-only. If your more complex version is meaningfully more efficient, then fine, but I suspect it's not.

[thomcc]

test ascii::long_readonly::is_ascii_slice_align_to              ... bench:          23 ns/iter (+/- 5) = 303913 MB/s
test ascii::long_readonly::is_ascii_slice_iter_all              ... bench:         161 ns/iter (+/- 24) = 43416 MB/s
test ascii::long_readonly::is_ascii_slice_libcore               ... bench:          16 ns/iter (+/- 4) = 436875 MB/s
test ascii::medium_readonly::is_ascii_slice_align_to            ... bench:           6 ns/iter (+/- 1) = 5333 MB/s
test ascii::medium_readonly::is_ascii_slice_iter_all            ... bench:          14 ns/iter (+/- 6) = 2285 MB/s
test ascii::medium_readonly::is_ascii_slice_libcore             ... bench:           3 ns/iter (+/- 3) = 10666 MB/s
test ascii::short_readonly::is_ascii_slice_align_to             ... bench:           8 ns/iter (+/- 2) = 875 MB/s
test ascii::short_readonly::is_ascii_slice_iter_all             ... bench:           3 ns/iter (+/- 3) = 2333 MB/s
test ascii::short_readonly::is_ascii_slice_libcore              ... bench:           4 ns/iter (+/- 5) = 1750 MB/s

for

    fn is_ascii_slice_align_to(bytes: &[u8]) {
        fn contains_nonascii(v: usize) -> bool {
            const NONASCII_MASK: usize = 0x80808080_80808080u64 as usize;
            (NONASCII_MASK & v) != 0
        }
        let (head, body, tail) = unsafe { bytes.align_to::<usize>() };
        head.iter().all(|b| b.is_ascii()) &&
        body.iter().all(|w| !contains_nonascii(*w)) &&
        tail.iter().all(|b| b.is_ascii())
    }

Of course with the standard caveat of "I spent far longer on my version than this version".

Also: On modern ARM (armv7+) read_unaligned is only slow if the address is actually unaligned, and even then it's really not that bad in my experience. Like, I don't have numbers for this but it seems around the same as just reading the bytes.

There are platforms where this is worse though, but I'd expect those to compile read_unaligned down to load each byte. Which is what the align_to version has to do (I'll admit, I find align_to a little annoying for this reason).

Anyway, I actually noticed is_ascii was bad because I was optimizing firefox for android's (the "preview") autocomplete search implementation, so I do care very much about ARM performance for this code.

[ecstatic-morse]

I would have added a fast path for when slice.len() < mem::size_of::<usize>() to the align_to version since your version has a similar check that makes it fast on the small benchmark. I will note that the margin of error for both versions overlaps, but I would call your version meaningfully more efficient on x86 at least. Thanks for doing the work.

[shepmaster]

Beyond my knowledge, let's try...

r? @sfackler

/cc @BurntSushi as I know your interest in these matters.

[nagisa]

Consider slice::align_to instead of align_offset? It also takes care of various other asserts you have in the rest of this function as well as removes the necessity for the "thoroughly" test case and significantly reduce the amount of unsafe in the code...

EDIT: wrote this before I saw the other comments...

The algorithm I'm using is different (and more efficient on most machines) than what I'd get with slice::align_to.

That likely means slice::align_to can be improved instead. I’ll experiment with it.

[thomcc]

Consider slice::align_to instead of align_offset? It also takes care of various other asserts you have in the rest of this function as well as removes the necessity for the "thoroughly" test case and significantly reduce the amount of unsafe in the code...

This is discussed in other comments above, it's measurably less performant (on my machine, anyway).

I suspect in practice it will do a little worse than it does on the benchmark I linked in #74066 (comment) (at least for the case that caused me to file this PR -- mostly medium-length strings). That is, the benchmarks don't really hit the cases where align_to is bad -- And I specifically wrote my function the way I did to avoid the cases where align_to is bad (concretely: to avoid a per-byte loop on the unaligned case).

To demonstrate this, if I modify the code* so that first and last bytes of the test string are skipped (to force unaligned on both ends), you see the following for medium (small is smaller than a usize, and long is too large for something like this to matter):

test ascii::medium_readonly::is_ascii_slice_align_to            ... bench:          12 ns/iter (+/- 4) = 2500 MB/s
test ascii::medium_readonly::is_ascii_slice_iter_all            ... bench:          10 ns/iter (+/- 2) = 3000 MB/s
test ascii::medium_readonly::is_ascii_slice_libcore             ... bench:           2 ns/iter (+/- 1) = 15000 MB/s

Note that previously medium would have been aligned to usize (came right out of an allocator), and was 32 bytes -- e.g. it would totally avoid the problem cases for align_to.

In practice a case like bit me before (it was with SIMD vectors, which exacerbates the issue, since they have longer tail/head sequences, but...). Which is... admittedly why hold a little bit of a grudge against align_to >_>

That said, I do feel like I'm getting a signal of "This is not worth the complexity", and I'd rather have an is_ascii that uses align_to than the current one, and I'm totally willing to change it if desired... I also have a few ideas on how to make it perform closer to my implementation, although IDK if there's any way to avoid bad performance in it's problem cases.

That said, IMO the stdlib is a better-than-average place to make a complexity/performance trade-off like this, so I'm going to probably wait for the reviewer to tell me to switch to align_to if desired.

---

* Uh, I'll have the is_ascii_slice_align_to bench up in a few minutes after sending this comment, but it's basically what's #74066 (comment) + the suggestion from #74066 (comment).

I probably won't add the hacky code I used to force misalignment (replacing vec[..] with vec[1..vec.len()-1]...) and generate the benchmark above, but I'll try to add a non-hacky benchmark that measures the misaligned case soon. I've added this in not-entirely-hacky way.

EDIT: (My comment was posted before I saw your response too)

That likely means slice::align_to can be improved instead. I’ll experiment with it.

@nagisa IMO it's kind of a problem with the API rather than something like a missed optimization (although, to be clear, align_to compiles weird -- LLVM aggressively often unrolls loops on the head/tail, e.g: https://godbolt.org/z/N7t96u).

In particular a big issue for me is that coming from &[u8] is common, but you often really don't want to be doing operations on each u8 in the lead/trail. In practice it's a little annoying since it feels like 80% of what I want, but the remaining 20%... If I'm reaching for it at all.

That said, I can't deny it's much more readable and maintainable than what I came up with.

[nagisa]

I extracted the implementations into a local crate and wrote a criterion-based benchmark… and tested the three implementations on both x86_64 and an aarch64. From what I can see, the proposed implementation is (noticeably) better than the align_to implementation at input sizes ~8 bytes to ~1kB, after which point the align_to begins overtaking the proposed implementation.

This makes sense to me given that align_to computes the slices ahead of time and the align_offset version checks its ability to stay in the fast path on every iteration.

So ultimately its probably a question of what kind of input we want to optimize for (and I think the medium length case is the most interesting one, favouring the currently proposed implementation).

That said, I also see extreme (>100%) run-to-run variance in some of the benchmark results, even though I did make an effort to avoid most of the well known pitfalls in benchmarking, so whatever data my benchmark is generating is gibberish anyway… This also makes it super difficult to just experiment with the minor changes to the code...

[thomcc]

One idea I had was something like:

fn is_ascii_align_to_unrolled(bytes: &[u8]) -> bool {
    // Not really clear if this should be testing size_of [usize; 2] or size_of usize still...
    if bytes.len() < core::mem::size_of::<[usize; 2]>() {
        return bytes.iter().all(|b| b.is_ascii());
    }
    // SAFETY: transmuting a sequence of `u8` to `[usize; 2]` is always fine
    let (head, body, tail) = unsafe { bytes.align_to::<[usize; 2]>() };
    head.iter().all(|b| b.is_ascii())
        && body.iter().all(|w| !contains_nonascii(w[0] | w[1]))
        && tail.iter().all(|b| b.is_ascii())
}

And this actually is as fast or almost as fast as the main impl in the PR... So long as your string's length is a multiple of size_of::<usize>() (Actually, I guess it would be size_of::<[usize; 2]>(), since align_to's tail is based on the size of the type, not it's alignment...).

Which seems like a pretty common case.

Anyway my most recent push adds a bench for that, separates out benches for different alignment issues that align_to can have problem with (head vs tail vs both), and moves the benchmarks to a different file, which cleaned a lot of things up. I probably should have been a bit better about making it clear which cases I was trying to optimize (or avoid pessimizing) in my initial PR)

A run of the benchmark results is here.

This makes sense to me given that align_to computes the slices ahead of time and the align_offset version checks its ability to stay in the fast path on every iteration.

Hm. I'm going to think about this since I feel like there's no reason we shouldn't be doing the same. I'm not fully sure I follow though...

That said really there are a few things I probably should do to improve the implementation of is_ascii in the PR (it probably should unroll the inner loop once), but also it might be complex enough as-is given that it's complexity already caused minor controversy.

[nagisa]

I’m going to approve this. I think optimizing for performance with smaller strings here makes sense and the algorithm as implemented here will do better with said smaller strings specifically because it just does less work.

I also don't see a reason to block this on benchmarks that are super fuzzy anyway...

@bors r+

[bors]

📌 Commit a150dcc has been approved by nagisa

[pickfire]

Is this necessary? What happens if we always check last_word? As in making it branchless.

[thomcc]

It's an optimization to avoid an extra (redundant) load. It's not necessary for correctness.

[pickfire]

It's an optimization to avoid an extra (redundant) load. It's not necessary for correctness.

Avoid extra load? But what happens if you remove it? Along with the debug assert.

[thomcc]

Then we perform an extra load of the last word, which we've already checked. What's your point?

[pickfire]

Yes, that's what I mean, perform the extra load of the last word to remove the branch. Or we could rework the logic a bit and remove this branch, I wonder would it be faster.

[thomcc]

Yes, that doesn't seem worth it to me at all. If you can show it's faster in benchmarks (which there are many), I suggest you submit a PR.

Keep in mind on some platforms a read_unaligned is comparatively expensive, so it's worth avoiding if possible.

[pickfire]

Do you have a minimal testing crate to test this easily without having to build rust?

[thomcc]

No, sorry.