Skip to content
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

Simplify pipeline parsing with using declarations #16216

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 15, 2024

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is mostly an experiment to see how using declarations can simplify our state management in the parser (currently we have a lot of intermediate functions to hide try/finally statements).

I also have other drafts to see how migrating to using looks like: https://github.com/nicolo-ribaudo/babel/pull/10/files?w=1, https://github.com/nicolo-ribaudo/babel/pull/11/files?w=1

I'll also open a PR to remove the F# and smart proposals in babel 8, since they are unfortunately not being pursued anymore.

@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: parser Spec: Pipeline Operator labels Jan 15, 2024
@babel-bot
Copy link
Collaborator

babel-bot commented Jan 15, 2024

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56142/

@liuxingbaoyu
Copy link
Member

I personally tend to use using outside of parser/generator and other places where performance is more important. I am worried that this will affect a little performance.

@nicolo-ribaudo
Copy link
Member Author

I will try to do some benchmarks

@nicolo-ribaudo
Copy link
Member Author

This PR introduces a performance regression indeed, we should not merge it until I figure out how to solve it.

However, it also looks like we currently have a big perf regression on main 🤔

This PR:

 node ./benchmark/babel-parser/real-case/bench.mjs
current 4 jquery 3.6: 13.21 ops/sec ±1.75% (76ms)
baseline 4 jquery 3.6: 22.71 ops/sec ±2.19% (44ms)
current 4 jquery 3.6: 13.32 ops/sec ±1.58% (75ms)
baseline 4 jquery 3.6: 22.31 ops/sec ±2.49% (45ms)

main:

➜ node ./benchmark/babel-parser/real-case/bench.mjs
current 4 jquery 3.6: 15.07 ops/sec ±1.91% (66ms)
baseline 4 jquery 3.6: 23.62 ops/sec ±2.25% (42ms)
current 4 jquery 3.6: 14.88 ops/sec ±2.2% (67ms)
baseline 4 jquery 3.6: 24.06 ops/sec ±2.07% (42ms)

where baseline is @babel/parser@7.23.6, and I'm compiling as CJS with make build.

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Jan 16, 2024

Please use make prepublish-build. :)

To be honest, I'm surprised that this is reflected in real-case, or even that this syntax is not included in the example.

@nicolo-ribaudo
Copy link
Member Author

To be honest, I'm surprised that this is reflected in real-case, or even that this syntax is not included in the example.

It's because parsing for the smart pipeline needs to reset context every time we enter a block, so it's all over the place.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft January 16, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: parser PR: Internal 🏠 A type of pull request used for our changelog categories Spec: Pipeline Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants