perf(parser): optimize conditional advance on ASCII values

[lucab]

Part of #3291.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[lucab]

• perf(parser): support peeking over bytes #4304
• perf(parser): optimize conditional advance on ASCII values #4298 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @lucab and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #4298 will not alter performance

_{Comparing ups/parser-advance-ascii-eq (868fc87) with main (e2735ca)}

Summary

✅ 32 untouched benchmarks

[overlookmotel]

Surprising and disappointing that this has no effect on benchmarks. I really thought it would, and if I remember right when I played with this back in Dec/Jan it did produce a speed-up.

I'm not sure what's going on! The problem previously was that Chars iterator was really slow as compiler didn't recognise that self.remaining().chars().next() == Some('.') only requires checking 1 byte, and generated a bunch of code to read a potentially multi-byte char before checking if it's '.'. Maybe between then and now Chars iterator has become more optimized?

[overlookmotel]

Nope. Chars iterator still generates a ton more code on Rust 1.79.0 than checking by byte.

https://godbolt.org/z/dsv93Pnce

I'll take another look at this tomorrow with fresh eyes. I really hope I haven't wasted your time.

[overlookmotel]

I am really struggling to understand why this isn't doing more on benchmarks.

Here is a more complete version of before vs after this PR on Godbolt: https://godbolt.org/z/fjc96GfeE

• Old version next_eq_dot_old is 55 instructions.
• New version next_eq_dot_new is 11.

So the old code is way more verbose. It also requires 2 branches before it gets to the cmp esi, 46 instruction, which is where it does the actual check for "is this byte .?". Granted, both those branches are almost never taken - the 1st branch is only taken when have reached EOF, and the 2nd only taken if the byte read is not ASCII (never happens in our benchmark fixtures) - so branch predictor will speculatively continue down the common path without delay. But still, I'd have expected that:

1. Trimming even a few extra instructions in a path as hot as this would make a real difference, at least to the lexer benchmarks.
2. The excessive amount of code generated by the old version might cause more instruction cache to be consumed and therefore cause more instruction cache misses.

But no!

I've also tried adding #[inline(always)] to both the relevant functions to make sure inlining is definitely happening. But I didn't expect that to make any difference, and sure enough it doesn't.

The Godbolt link above also includes an alternative version advance_if_ascii_eq2 which converts the branch which is unpredictable to straight line code. But I doubt that'd help in practice - in actual code, we're always branching on the result of next_ascii_char_eq too, so that'd re-introduce the branch again.

@lucab I don't know how used you are to looking at assembly like this, but any ideas?

@strager I don't know if you're out there (and I know you deny being an expert in such matters!), but I'm a bit lost here and would really appreciate your thoughts if you have time.

[DonIsaac]

Any updates on this PR?

[Boshen]

I see no objections, merging.

We can also use /usr/bin/time to measure CPU instruction counts.

[graphite-app]

Merge activity

• Jul 26, 9:17 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jul 26, 9:17 PM EDT: Boshen added this pull request to the Graphite merge queue.
• Jul 26, 9:20 PM EDT: Boshen merged this pull request with the Graphite merge queue.