Skip to content

Conversation

liuxingbaoyu
Copy link
Member

@liuxingbaoyu liuxingbaoyu commented Aug 7, 2023

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

Some discussion is needed.
Because it's a breaking change.
Ref: #13868

This PR makes fields and builderKeys only lookup once on initialization.

PS F:\babel\benchmark\babel-types\builders> node .\memberExpression.mjs
baseline memberExpression builder: 804_478 ops/sec ±0.52% (0.001ms)
current memberExpression builder: 7_391_582 ops/sec ±0.26% (0ms)
PS F:\babel\benchmark\babel-types\builders> node .\stringLiteral.mjs
baseline stringLiteral builder: 32_462_576 ops/sec ±0.42% (0ms)
current stringLiteral builder: 81_001_155 ops/sec ±0.6% (0ms)

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 7, 2023

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

@liuxingbaoyu liuxingbaoyu force-pushed the improve-builder branch 3 times, most recently from 3c03dea to e112104 Compare August 7, 2023 11:30
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Awesome performance numbers! I am in favor of landing this for Babel 8.

I'm wondering if maybe we can get even better numbers by inlining the validateInternal calls directly in the builder functions, at least for "pupular" nodes (Identifier, BinaryExpression, UnaryExpression, LogicalExpresssion, VariableDeclarator, VariableDeclaration, ...)


for (const key of builderKeys) {
const field = fields[key];
if (field != null) validateInternal(field, node, key, node[key]);
Copy link
Member

Choose a reason for hiding this comment

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

Does the validation for required field still work with this field != null check?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will work, I just followed here to check ahead.
https://github.com/babel/babel/pull/15843/files#diff-04a0d87b83569ab9a9d94ee801346cf5443ecf057bf35f4836d4a259e882c8fcR39
But thanks to you, I found another problem, validateChild was accidentally skipped. :)

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Aug 8, 2023

Yes, performance gets better when we inline, but I'm not sure if that's the way to go since the size would increase a lot.
Maybe we can dynamically generate fast versions of them when new Function()/eval is available?
But I'm not sure if doing so under ESM would be difficult.

current

PS F:\babel\benchmark\babel-types\builders> node .\memberExpression.mjs
baseline memberExpression builder: 788_321 ops/sec ±0.65% (0.001ms)
current memberExpression builder: 7_414_300 ops/sec ±0.47% (0ms)

image

PS F:\babel\benchmark\babel-types\builders> node .\memberExpression.mjs
baseline memberExpression builder: 807_823 ops/sec ±0.46% (0.001ms)
current memberExpression builder: 12_744_969 ops/sec ±0.31% (0ms)

image

PS F:\babel\benchmark\babel-types\builders> node .\memberExpression.mjs
baseline memberExpression builder: 813_116 ops/sec ±0.43% (0.001ms)
current memberExpression builder: 10_822_404 ops/sec ±0.35% (0ms)

@liuxingbaoyu liuxingbaoyu added PR: Performance (next major) 🏃‍♀️ A type of pull request used for our changelog categories for next major release and removed i: discussion labels Aug 22, 2023
@liuxingbaoyu liuxingbaoyu added this to the v8.0.0-beta milestone Sep 2, 2024
@nicolo-ribaudo
Copy link
Member

Lets do this without the suggestion I had in #15843 (review) :)

@liuxingbaoyu
Copy link
Member Author

Lets do this without the suggestion I had in #15843 (review) :)

We seem to have done that in a subsequent commit. :)

@liuxingbaoyu
Copy link
Member Author

Since BABEL_8_BREAKING is dynamic, this PR is a bit harder to merge.
I will open a PR to complete #13868, which will allow us to merge this PR in the Babel 8 patch release.

@liuxingbaoyu
Copy link
Member Author

I'll try to land it in Babel 7, since #13868 is no longer blocking.
At the same time I realize that this will be annoying as long as we use process.env.BABEL_8_BREAKING or even process.env.BABEL_9_BREAKING.
Hopefully we can fix it once and for all.

@liuxingbaoyu liuxingbaoyu modified the milestone: v8.0.0-beta Sep 17, 2024
@liuxingbaoyu liuxingbaoyu added PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories and removed PR: Performance (next major) 🏃‍♀️ A type of pull request used for our changelog categories for next major release labels Sep 17, 2024
@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review September 17, 2024 21:56
};
validate(_data[1][0], node, "operator", operator);
validate(_data[1][1], node, "left", left, true);
validate(_data[1][2], node, "right", right, true);
Copy link
Member

Choose a reason for hiding this comment

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

Can we hard-code numbers here, instead of using _data[1][2]?

Copy link
Member Author

Choose a reason for hiding this comment

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

The data in the array is like this.

export type FieldOptions = {
  default?: string | number | boolean | [];
  optional?: boolean;
  deprecated?: boolean;
  validate?: Validator;
};

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. And the reason we don't do validate(NODE_FIELDS[node.type].operator, node, "operator", operator) is for code size?

Copy link
Member

Choose a reason for hiding this comment

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

Or even just validate(node.type, node, "operator", operator), and then validate internally can get NODE_FIELDS[node.type]["operator"].

Copy link
Member Author

Choose a reason for hiding this comment

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

This avoids a lookup in NODE_FIELDS.
Also array access may be faster than .["operator"].

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that the performance data has little impact, I will open a new PR to try.

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Sep 18, 2024

I think the CI error is a build error in Babel 8 where we accidentally bundled @babel/types everywhere. 🤦‍♂️
I will investigate.
image

Comment on lines +37 to +39
for (const [type, fields] of Object.entries(fieldsBabel7).concat(
Object.entries(fieldsBabel8)
)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The formatting here I'm not sure if it can be improved.
cc @fisker
If you're interested!

@liuxingbaoyu
Copy link
Member Author

Superseded by #16842.

@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 Dec 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants