perf: hoist static objects/arrays in templates #2618
Conversation
nolanlawson
force-pushed
the
nolan/hoist-static-redux
branch
from
d92b793
to
7c62204
Compare
|
Rebased on master ^ |
nolanlawson
force-pushed
the
nolan/hoist-static-redux
branch
from
7c62204
to
51fdf8e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/nucleus ignore --reason "ui-interaction-explorer-components failing due to object spread" |
|
I am extremely excited by this PR. 🎉 |
Co-authored-by: Pierre-Marie Dartus <[email protected]>
nolanlawson
force-pushed
the
nolan/hoist-static-redux
branch
from
ce15736
to
3166849
Compare
|
Removing |
| @@ -588,7 +588,12 @@ function updateDynamicChildren(parentElm: Node, oldCh: VNodes, newCh: VNodes) { | |||
| newStartVnode.hook.insert(newStartVnode, parentElm, oldStartVnode.elm!); | |||
| } else { | |||
| patchVnode(elmToMove, newStartVnode); | |||
| oldCh[idxInOld] = undefined as any; | |||
| // Delete the old child, but copy the array since it is read-only. | |||
There was a problem hiding this comment.
my intent is to remove (or minimize) this cloning from the codepath, I have some questions:
- is it that we declared VNodes readonly for convenience, or that it must be readonly?
- if it is for convenience, can we redeclare the type in
updateDynamicChildrento receive a Writable? - if it is a must (or we want to be strict with the type), would it help just to clone the array once, instead of multiple times?
My thought process is that we may need to move multiple nodes via this codepath (ex: sort rows in a table), and all this cloning may happen multiple times, when it really needs to happen only once (or not at all).
There was a problem hiding this comment.
1 - Yes it is required. Vnodes is the children array, and it may be shared (due to empty arrays currently, but in the future I'd like static children to be completely shared too).
2 - Hm, the thing is that this code path is very uncommon (it wasn't even covered in any of our Karma tests until I added a test), so we don't want to clone the array every time the function is called. We can certainly clone once (when the codepath is hit), but I don't know if it would actually improve perf in many scenarios, since this scenario is already so rare.
I'll open a separate issue to explore the "only clone once," since I think we need to explore whether it actually improves perf and by how much.
Details
Un-revert of #2589. Reverts commit 66c0762 (#2617).
This can't be merged until we're ready for the compiler changes.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Template compiler has changed.