perf(parser): reduce Token size to 8 bytes from 16#8153
perf(parser): reduce Token size to 8 bytes from 16#8153branchseer wants to merge 5 commits intooxc-project:mainfrom
Token size to 8 bytes from 16#8153Conversation
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. |
Token size to 8 bytes from 12Token size to 8 bytes from 16
CodSpeed Performance ReportMerging #8153 will degrade performances by 13.22%Comparing Summary
Benchmarks breakdown
|
|
My local bench run shows the same regression on lexer, but also shows noticeable improvements on parser. I guess the lexer regression makes sense since the lexer now does more calculation but barely copies tokens on it own. Here's my local bench result of parser: I suspected it was a cpu arch thing so I ran the parser bench under rosetta 2, only to see even more improvements: DetailsNow I'm lost. @overlookmotel any insight on this? |
|
Thanks for investigating further. I did spend a couple of hours looking at this yesterday and scratching my head. I had to stop because I could feel a rabbit hole coming on and I had other tasks I needed to get on with! Please give me a few days to mull it over and I'll come back to you with some ideas. Also, #8298 may have an effect, as lots of work on One question in meantime:
What effect did you expect Rosetta 2 to have? Rosetta is an x86_64 emulator, right? (just checking I do know what I think I know!) |
|
Yeah I ran Rosetta 2 to check the bench result under x86_64. It was my wishful thinking that if Rosetta 2 gave the same result as codespeed, that would prove the improvements occur only on specific cpu archs (apple arm64). |
|
We won't be able to merge this PR due to conflicting results from the benchmark, but I can merge the token API change so that we can focus on changing the token shape next time. |
|
Personally, I don't think we should merge until we have confirmation that the perf regression that's showing on CodSpeed does not also affect "real" x86_64 (Rosetta is not x86, but an ARM chip pretending to be x86, so I'm not sure if it's representative). Also I would like to finish up #8298 and see what this looks like on top of that. I do have this on my radar, please give me a couple of days. |
|
Those 2 PRs are now merged. @branchseer Could you please rebase this on latest main so we can see if benchmarks shift at all? By the way, I tried a few optimizations to |
# Conflicts: # crates/oxc_parser/src/lexer/token.rs
# Conflicts: # crates/oxc_parser/src/modifiers.rs
daa2a58 to
0024247
Compare
|
Close as stale. |
end: u32withlen: u16. Ends of long tokens (which are rare) are stored inlexer.long_token_ends;endis calculated fromstart + len,startmust be properly set. In some places they were not. This PR fixes them and adds adebug-assertioncheck in the lexer.