Manually track children's index & fix parent pointers when rerendering components #4170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
03_update10th1k_x16
duration
usedJSHeapSize
07_create10k
duration
usedJSHeapSize
filter_list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many_updates
duration
usedJSHeapSize
text_update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
|
|
Size Change: -8 B (0%) Total Size: 57.2 kB
ℹ️ View Unchanged
|
In `getDomSibling` we do a `indexOf` search in the parent VNode to find the location of the vnode in the parent's children array. However, when rerendering a component we copy the VNode to make the oldVNode. This copy if the VNode means the `indexOf` search won't work cuz that copy doesn't exist the parent's children array. So calls to `getDomSibling(oldVNode)` would not return the correct result. This PR fixes this by manually tracking the index the childVNode is in the parent's children array so we no longer need to do the indexOf search.
andrewiggins
force-pushed
the
index-vnode-field
branch
from
c93971d
to
97f0465
Compare
JoviDeCroock
approved these changes
Oct 25, 2023
marvinhagemeister
approved these changes
Oct 26, 2023
|
Fun story for posterity, the initial push of this PR showed an unexpected regression in 02_replace1k benchmark and greater memory usage than expected (some rise is expected, but not what it initially showed). Upon looking into why, I realized I forgot to initialize the Just wanted to share a story where having these benchmarks led to finding and fixing issues :) |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When rerendering a component we copy the component's VNode to make the oldVNode. However, copying the VNode breaks two connections in the virtual node tree:
The
_parentpointer of children of theoldVNodepoint to the original VNode, not the copied one. But in this situation, they should point to theoldVNodesince they belong to the old tree and not the new tree we are about to rerender/reconstruct.the copied VNode (
oldVNode) doesn't exist in its parent's children array, so ingetDomSibling, theindexOfsearch we do on a VNode's parent won't work. So calls togetDomSibling(oldVNode)would not return the correct result.Previously we discovered this issue when using null placeholders in components that setState. So the current fixes address that particular scenario by:
Before calling getDomSibling, we set the child's parent to
oldVNode(calledoldParentVNodeat this point indiffChildren). Before that fix, parent pointer of the child VNode would point to newParentVNode, which is incorrect. In diffChildren, we setnewParentVNode._children = []and fill it as we loop through new children, meaning child VNode would never occur in newParentVNode._children.We return
_nextDominstead of_domingetDomSiblingif it is set. This fix coincidentally worked for the test cases we had because there was only one component with no sibling components. In this situation,getDomSiblingwould return a DOM node that is about to be unmounted and so all DOM after it would be re-appended. In the situation where a component has a sibling component that also returns a Fragment, returning_nextDomwould prevent us from seeing this new sibling component and it's dom node and and so we would incorrectly append children after it's DOM. This situation is captured in a new test case.Now, having a deeper understanding of the issue, this PR proposes two fixes to address this problems more holistically.
To address the first issue, after copying the componet's VNode to
oldVNode, we loop through the children and set their _parent pointer tooldVNode.Instead of using
indexOfingetDomSiblingwe will track a VNode's index in it's parent's children array as a property on the VNode, removing the need forindexOfingetDomSibling. Note: this doesn't yet fully fix the second issue ("oldVNodedoesn't exist in the parent's children array"), but it works around that issue by relying on the fact that in this situation the VNode it was copied from will be at the same index in the parent's children array and so works forgetDomSibling. A future PR will attempt to address this problem more directly.So in summary, tl;dr:
indexOfingetDomSibling.Also note: #4162 also adds an
_indexproperty to VNodes so adding that property has multiple benefits, besides this one PR