-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
perf: Improve @babel/types
builders
#15843
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
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57981 |
3c03dea
to
e112104
Compare
There was a problem hiding this 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]); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
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. current
|
Lets do this without the suggestion I had in #15843 (review) :) |
We seem to have done that in a subsequent commit. :) |
7f71f8c
to
19c0ba7
Compare
Since |
I'll try to land it in Babel 7, since #13868 is no longer blocking. |
19c0ba7
to
2d2dd23
Compare
30d99c6
to
7add9e7
Compare
7add9e7
to
47819b4
Compare
}; | ||
validate(_data[1][0], node, "operator", operator); | ||
validate(_data[1][1], node, "left", left, true); | ||
validate(_data[1][2], node, "right", right, true); |
There was a problem hiding this comment.
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]
?
There was a problem hiding this comment.
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;
};
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"]
.
There was a problem hiding this comment.
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"]
.
There was a problem hiding this comment.
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.
for (const [type, fields] of Object.entries(fieldsBabel7).concat( | ||
Object.entries(fieldsBabel8) | ||
)) { |
There was a problem hiding this comment.
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!
Superseded by #16842. |
Fixes #1, Fixes #2
Some discussion is needed.
Because it's a breaking change.
Ref: #13868
This PR makes
fields
andbuilderKeys
only lookup once on initialization.