refactor(parser): split lexer into multiple files#2228
Conversation
CodSpeed Performance ReportMerging #2228 will not alter performanceFalling back to comparing Summary
|
|
PS I also tried out moving the byte handlers into multiple files (e.g. move I propose instead as a follow-up to refactor the non-trivial byte handlers so they call I think that's a better solution than scattering the byte handlers across multiple files, while keeping |
|
By the way, just to add some context, this is what all my recent PRs are building up to: https://codspeed.io/overlookmotel/oxc/branches/lexer-string4 |
Boshen
left a comment
Boshen
left a comment
There was a problem hiding this comment.
1 file style or multifile styles are both fine as long as it's easy to navigate with lsp enabled IDE.
https://github.com/oven-sh/bun/blob/main/src/js_parser.zig is 23077 loc lmao.
|
Btw to save some precious time for you, you may just leave the PR description as "moving code around for easier navigation", I understand most the things you are doing, I'll ask for comment when it's confusing 😁 |
WTF??????????????????????????? |
|
Thanks for merging this, and for the note on PR descriptions.
LSP does help, but personally I find it easier to grok if I can look at all the code for e.g. identifier lexing on one page, without having to jump all over the place.
That's just the start! That gain is all down to speeding up finding the end of identifiers. Should be able to use same technique for finding end of strings and comments, for further speed-ups (though they won't be as dramatic - identifiers are particularly tricky/slow due to handling unicode characters). |
|
On speaking of unicode and identifiers ... https://github.com/rusticstuff/simdutf8 Oh man we are just going deeper and deeper! |
I own the crate, https://crates.io/crates/unicode-id-start, if you haven't noticed, which means we can hack it even more! |
This PR has a large diff, but it contains no substantive changes whatsoever. It purely breaks up the lexer into multiple smaller files. I've been working quite intensively on the lexer over past few weeks, but still have been finding it hard to make sense of, due to most of the logic currently being contained in [a single 1800-line file](https://github.com/oxc-project/oxc/blob/018675ceb1a7e3442d1aeaa69e0256964675d207/crates/oxc_parser/src/lexer/mod.rs). I feel that breaking it up into multiple files makes it much easier to navigate and understand. An additional benefit is that many functions can have their visibility reduced to module scope, so sub-systems for e.g. lexing numbers have fewer exposed functions. This makes it clearer what the entry points are, and makes it harder to make mistakes when working on the lexer. I intend to later make changes to the lexer for performance which will introduce unsafe code. Keeping that unsafe code encapsulated in modules will make it more viable to validate the workings of that code, and avoid accidental UB. There is one downside to this change. Previously [`lexer/mod.rs`](https://github.com/oxc-project/oxc/blob/018675ceb1a7e3442d1aeaa69e0256964675d207/crates/oxc_parser/src/lexer/mod.rs) was laid out in same order as the JS spec. If you were trying to validate the lexer against the spec, this would make it easier. However, as OXC's parser is fairly mature at this point, and I imagine most spec-compliance issues have been flushed out by now, in my opinion this advantage is less compelling than it probably used to be. So in my view it's outweighed by the benefit of more readable code. Reviewing this could be a bit of a battle due to the size of the diff. I do have further changes I'd like to make, but I've intentionally kept this PR as 100% just: 1. Moving code around. 2. Reducing visibility of functions to module/super scope where that's possible to do without changing anything else. Aside from that, not even a single comment has changed. If you're willing to trust me on that promise, I think it can be merged without poring through it line by line.
2 participants
This PR has a large diff, but it contains no substantive changes whatsoever. It purely breaks up the lexer into multiple smaller files.
I've been working quite intensively on the lexer over past few weeks, but still have been finding it hard to make sense of, due to most of the logic currently being contained in a single 1800-line file.
I feel that breaking it up into multiple files makes it much easier to navigate and understand.
An additional benefit is that many functions can have their visibility reduced to module scope, so sub-systems for e.g. lexing numbers have fewer exposed functions. This makes it clearer what the entry points are, and makes it harder to make mistakes when working on the lexer.
I intend to later make changes to the lexer for performance which will introduce unsafe code. Keeping that unsafe code encapsulated in modules will make it more viable to validate the workings of that code, and avoid accidental UB.
There is one downside to this change. Previously
lexer/mod.rswas laid out in same order as the JS spec. If you were trying to validate the lexer against the spec, this would make it easier. However, as OXC's parser is fairly mature at this point, and I imagine most spec-compliance issues have been flushed out by now, in my opinion this advantage is less compelling than it probably used to be. So in my view it's outweighed by the benefit of more readable code.Reviewing this could be a bit of a battle due to the size of the diff. I do have further changes I'd like to make, but I've intentionally kept this PR as 100% just:
Aside from that, not even a single comment has changed.
If you're willing to trust me on that promise, I think it can be merged without poring through it line by line.