-
-
Notifications
You must be signed in to change notification settings - Fork 722
perf(lexer): skip single space in read_next_token
#15513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(lexer): skip single space in read_next_token
#15513
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #15513 will improve performances by 6.57%Comparing Summary
Benchmarks breakdown
|
020e6c0 to
1bb8b9f
Compare
bef04c1 to
2f0518d
Compare
1bb8b9f to
e54a659
Compare
e54a659 to
10fbede
Compare
017c2da to
482bd16
Compare
482bd16 to
6dba827
Compare
|
First version of this PR showed very weird benchmark results - massive regression on the lexer benchmarks, but improvement on all parser benchmarks (between 2% - 7%). Turns out that this was an oddity of the benchmark code. The increase in size of #15519 and #15520 fix that, so now we can see the effects of this change in isolation. And they're quite good! |
Boshen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude study assembly code and discovered the jump table has a huge branch mis-prediction problem, but I couldn't figure out where the branch is.
Question: can we use simd to find the next position that is not a whitespace?
Thought: We should study v8 at some point.
Merge activity
|
It's very common for tokens to be separated by a single space. e.g. `const x = 1`, `x === y`. Previously a single space resulted in calling the `SPS` byte handler, which consumes the space, and then going round the loop again in `Lexer::read_next_token`. Instead, branchlessly consume a single space (if there is one) before calling the byte handler. Gives between 2% and 7% perf improvement on parser benchmarks. --- This also enables a further optimization (not yet implemented). Now the handler for whitespace (`SPS`) no longer has a hot path for single spaces - it's now only called for a tab, or a 2nd space in a row. In both those cases, it's quite likely there'll be more whitespace following it, so it can now be optimized for that case, and continue consuming bytes until it finds one that *isn't* whitespace. If handlers for whitespace, line breaks, and comments all continue consuming bytes until they find a "real" token, then we can get rid of `Kind::Skip`, and remove the loop from `read_next_token`. This would remove another unpredictable branch.
…mark (#15519) Preparatory step for #15513. That PR was showing a massive slowdown on lexer benchmarks, but it was only due to the change in that PR resulting in `next_token` not being inlined into the lexer benchmark. Add a separate function `next_token_for_benchmarks` which has identical context as `next_token`, but is marked `#[inline(always)]`, and use it in lexer benchmark instead. This fixes the problem with the benchmark in #15513.
Preparatory step for #15513. That PR adds a 2nd callsite for `handle_byte`. Mark it as `#[inline(always)]` to make sure it gets inlined in both places. This was originally part of #15513, but have split it out into a separate PR so that Codspeed's results on #15513 measure the actual substantive change in isolation - to check that change is having the effect I think it is, and that the gain wasn't actually coming from adding `#[inline(always)]` here. This PR has no effect on performance, so the gain *is* in #15513.
ff4461f to
b310c28
Compare
6dba827 to
f1efc63
Compare
Interesting! Can you post what he said? I don't think it's surprising though. The branch is the jump table. CPU cannot predict which way it'll jump. I've opened an issue in backlog about this: oxc-project/backlog#192
Yes, and Rust stabilized
Yes probably! |

It's very common for tokens to be separated by a single space. e.g.
const x = 1,x === y.Previously a single space resulted in calling the
SPSbyte handler, which consumes the space, and then going round the loop again inLexer::read_next_token.Instead, branchlessly consume a single space (if there is one) before calling the byte handler.
Gives between 2% and 7% perf improvement on parser benchmarks.
This also enables a further optimization (not yet implemented).
Now the handler for whitespace (
SPS) no longer has a hot path for single spaces - it's now only called for a tab, or a 2nd space in a row. In both those cases, it's quite likely there'll be more whitespace following it, so it can now be optimized for that case, and continue consuming bytes until it finds one that isn't whitespace.If handlers for whitespace, line breaks, and comments all continue consuming bytes until they find a "real" token, then we can get rid of
Kind::Skip, and remove the loop fromread_next_token. This would remove another unpredictable branch.