Repository navigation
perf: Make VISITOR_KEYS etc. faster to access - #16918
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58153 |
|
I made a minimal reproduction. We can find that const object1 = {
a1: null,
a2: [],
a3: 0,
a4: "script",
a5: undefined,
a6: true,
a7: true,
a8: true,
a9: true,
a10: true,
a11: true,
a12: true,
a13: true,
a14: true,
a15: true,
a16: true,
a17: true,
a18: true,
a19: true,
a20: true,
};
let x;
function test(opts, name) {
console.time(name);
for (let i = 0; i < 1e10; i++) {
x = opts.a1;
}
console.timeEnd(name);
}
const name = process.argv[2];
switch (name) {
case "fast":
test(object1, name);
break;
case "slow":
const object2 = {};
for (const key of Object.keys(object1)) {
object2[key] = object1[key];
}
test(object2, name);
break;
case "toFast":
const object3 = {};
for (const key of Object.keys(object1)) {
object3[key] = object1[key];
}
%ToFastProperties(object3);
test(object3, name);
break;
case "cloneFast":
const object4 = { ...object1 };
test(object4, name);
break;
} |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
It's possible that ToFastProperties made sense in V8 versions from 5-6 years ago, but things changed since then.
Thanks for researching this!
| for (const key of Object.keys(defaultOptions) as (keyof Options)[]) { | ||
| options[key] = opts[key] ?? defaultOptions[key]; | ||
| } | ||
| return options; |
There was a problem hiding this comment.
Here we start from {} and then change the shape for ~20 times. Can we start the new options from { ...defaultOptions } and assign the values if opts[key] is not nullish? Doing so should also avoid the shape migration during initialization.
| DEPRECATED_ALIASES, | ||
| DEPRECATED_KEYS, | ||
| _DEPRECATED_KEYS as DEPRECATED_KEYS, | ||
| NODE_PARENT_VALIDATIONS, |
There was a problem hiding this comment.
Can you also do the shallow copy for NODE_PARENT_VALIDATIONS?
| // https://github.com/babel/babel/pull/16918 | ||
| // `options` is accessed frequently, please make sure it is a fast object. | ||
| // `%ToFastProperties` can make it a fast object, but the performance is the same as the slow object. | ||
| const options: any = { ...defaultOptions }; |
There was a problem hiding this comment.
Nit: If we move the assignment to the start, the opts == null branch can just reuse the options.
|
Oops, my description in the PR was wrong. |
Fixes #1, Fixes #2This fixes potential #16873 (comment)
Also improved access speed for some properties.
before
after
This is not due to the
to-fast-propertiespackage, as%HasFastPropertiesshows that it correctly makes the object fast.I suspect the original optimization here is being lost due to some changes in v8, even if I use the
%ToFastPropertiesprovided by v8, it still performs the same as without special optimizations.Take the parser as an example
When we go from 19 to 20 options, the return value of
%HasFastPropertieschanges from true to false, and the overall performance drops by 10%.After we call
%ToFastProperties, the return value of%HasFastPropertieschanges from false to true, but the performance does not return to the previous state.Until we create a new fast object using
{...obj}, performance will return to previous state.