Performance Improvements for Vnode.normalizeChildren()
#3051
Conversation
…nodes are not component vnodes Since commit 1579fe8, all `vnode.children` arrays have been recreated during normalization. However, for non-component vnodes, creating new arrays is unnecessary. This change normalizes them in place to improve performance. Passes all existing tests as is.
… for better performance This change merges multiple loops into a single one and reduces the number of comparisons needed for consistency checking, resulting in improved performance. Additionally, since key checks are now performed on normalized vnodes, the consistency checks become more accurate. All existing tests pass. The comments have also been clarified with reference to the original commit 6c562d2.
|
Unfortunately, removing the array cloning is a breaking change. People actually depend on that behavior, depending on Mithril to not change nodes out from under them before they're returned to the renderer. |
|
@dead-claudia Thank you very much for your review. It seems that array cloning is the main bottleneck, so optimizing only loops and integrity checks doesn’t provide much performance improvement. Thanks again for taking the time to review it! |
@kfule In an ideal world, I would agree with you, but unfortunately, it's just too risky. People have long been directly mutating the vnode arrays they return. I've seen it multiple times in issues and discussions through the years, complete with people wondering why they can't |
|
@dead-claudia You're right - there are all kinds of users out there, so this change could indeed be too risky😅 |
|
@dead-claudia I've made a new finding and created a separate PR instead of reopening this PR. The new PR is more modest than this PR, but I believe it is a non-breaking improvement. |
Improves
Vnode.normalizeChildren()performance by avoiding array re-creation, merging loops, reducing comparisons, and performing key consistency checks after normalization without breaking existing behavior.Description
This change improves the performance of
Vnode.normalizeChildren()by:result of
npm run perfnpm run perf(on my laptop, node v22)Based on the results of #3041, this change improves performance by about 5% for "rerender same tree".
Motivation and Context
Earlier, I found that performance could be improved by avoiding array re-creation and removing consistency checks, as in v1.
However, that approach clearly broke the current behavior, so I did not include it in #3041.
After further investigation, it turned out that similar performance improvements could be achieved without breaking the current behavior.
Although I’m slightly disappointed by the small increase in bundle size introduced in this PR after #3050, I’m pleased with the noticeable performance improvements.
How Has This Been Tested?
npm run testTypes of changes
Checklist