Skip to content

improve builder performance in @babel/types#13369

Closed
zxbodya wants to merge 3 commits intobabel:mainfrom
zxbodya:babel-types-experimental-improvements
Closed

improve builder performance in @babel/types#13369
zxbodya wants to merge 3 commits intobabel:mainfrom
zxbodya:babel-types-experimental-improvements

Conversation

@zxbodya
Copy link
Copy Markdown
Contributor

@zxbodya zxbodya commented May 25, 2021 •
edited
Loading

Q                       A
License MIT

This is experimental try to improve node builders in @babel/types. Feedback and suggestions are welcome.

Goals:

  • try generating complete builders instead of dynamically building nodes - having juts one object literal might be faster
  • experiment about removing node validation from builders -- idea is that this is probably useful only during development, but if typescript is used - this should be already covered for most cases… so - it sounds like a good candidate to remove/disable in production

Todo:

  • come-up with realistic approach to measure how faster it can be if generated and how much overhead there is because of validation -- suggestions and help is welcome

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented May 25, 2021 •
edited
Loading

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

zxbodya
zxbodya commented May 25, 2021
View reviewed changes
packages/babel-types/scripts/generators/builders.js
.map(([k, v]) => ` ${k}: ${v},`)
.join(
"\n"
)} } as t.${type};\n validateNode(node);\n return node;\n}\n`;
Copy link
Copy Markdown
Contributor Author

@zxbodya zxbodya May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking if this validateNode(…) call can be removed…

Copy link
Copy Markdown
Contributor

@JLHwung JLHwung May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have customized validation that can only be invoked via validateNode.

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented May 25, 2021 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ea26c67:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@zxbodya zxbodya mentioned this pull request May 25, 2021
Closed
JLHwung
JLHwung reviewed May 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the performance benchmark, we have micro benchmark cases in babel-parser: packages/babel-parser/benchmark

packages/babel-types/scripts/generators/builders.js
.map(([k, v]) => ` ${k}: ${v},`)
.join(
"\n"
)} } as t.${type};\n validateNode(node);\n return node;\n}\n`;
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have customized validation that can only be invoked via validateNode.

packages/babel-types/src/builders/generated/index.ts Outdated
) {
const node = {
type: "CatchClause",
param: param === undefined ? null : param,
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace # === undefined ? a : b by default parameters, if it is one of the builder keys.

Copy link
Copy Markdown
Contributor Author

@zxbodya zxbodya May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, actually just tried that - let me push it

@zxbodya
Copy link
Copy Markdown
Contributor Author

zxbodya commented May 25, 2021 •
edited
Loading

On the performance benchmark, we have micro benchmark cases in babel-parser: packages/babel-parser/benchmark

yeah, seen it… - however that would be useful only after already using babel/types in parser

curios also what would be impact of the change on existing plugins already using it -- it there already some benchmark on something like typical "es6" to "es5" codebase?

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented May 25, 2021

The benchmark is for general purpose. For example I have drafted a benchmark on the string literal builder: JLHwung@44e6253

node --predictable ./builder-string-literal/bench.mjs
baseline stringLiteral builder: 3764552 ops/sec ±1.06% (0ms)
current stringLiteral builder: 5278258 ops/sec ±0.77% (0ms)

The new implementation is 40% faster.

@JLHwung JLHwung mentioned this pull request Oct 13, 2021
Merged
@JLHwung JLHwung mentioned this pull request May 3, 2022
Merged
@zxbodya zxbodya deleted the babel-types-experimental-improvements branch May 9, 2022 22:26
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JLHwung JLHwung JLHwung left review comments

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zxbodya @babel-bot @JLHwung