Rework children diffing to run in multiple phases #4162
Closed
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
Separate matching and inserting logic since skew logic needs to run in between those
Had to move component vnode children equality check out of constructNewChildrenArray since at this point we haven't diffed the vnode and so don't know if it's gonna shortcut and reuse the existing children array, requiring us to call reorderChildren to calculate `_nextDom`
andrewiggins
requested review from
developit,
marvinhagemeister and
JoviDeCroock
📊 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: +801 B (1%) Total Size: 58 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Oct 21, 2023
jsx-runtime/src/index.js
Outdated
Comment on lines
56
to
59
| _prevVNode: undefined, | ||
| _insert: false, | ||
| _matched: false, | ||
| _index: -1, |
There was a problem hiding this comment.
There's definitely opportunity to consolidate these, and possibly remove obsolete fields once we further take advantage of this new approach. Will explore that more once I get Suspense tests working.
Comment on lines
+2
to
+4
| export const EMPTY_VNODE = /** @type {import('./internal').VNode} */ ( | ||
| EMPTY_OBJ | ||
| ); |
There was a problem hiding this comment.
Probably unnecessary - will undo to reduce size later. Did this temporarily to make TypeScript happy in my editor.
src/diff/children.js
Outdated
Comment on lines
661
to
662
| let shouldSearch = | ||
| remainingOldChildren > (oldVNode != null && !oldVNode._matched ? 1 : 0); |
There was a problem hiding this comment.
TODO: verify terser inlines this nicely and removes the unnecessary variable for size purposes.
WIP: suspense hydration & suspense list
Comparing children arrays in diffChildren needs to use tripe-equals strict equality to avoid having `null` and `undefined` appear equal. Children is `undefined` when mounting and oldVNode is EMPTY_OBJ. Children can be null when a component renders null.
andrewiggins
force-pushed
the
two-phase-diffChildren
branch
from
8820aaa to
6e9ba3f
Compare
This was referenced Oct 24, 2023
andrewiggins
added a commit
that referenced
this pull request
Oct 26, 2023
…g components (#4170) 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: 1) The `_parent` pointer of children of the `oldVNode` point to the original VNode, not the copied one. But in this situation, they should point to the `oldVNode` since they belong to the old tree and not the new tree we are about to rerender/reconstruct. 2) the copied VNode (`oldVNode`) doesn't exist in its parent's children array, so in `getDomSibling`, the `indexOf` search we do on a VNode's parent won't work. So calls to `getDomSibling(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: 1) Before calling getDomSibling, we set the child's parent to `oldVNode` (called `oldParentVNode` at this point in `diffChildren`). Before that fix, parent pointer of the child VNode would point to newParentVNode, which is incorrect. In diffChildren, we set `newParentVNode._children = []` and fill it as we loop through new children, meaning child VNode would never occur in newParentVNode._children. 2) We return `_nextDom` instead of `_dom` in `getDomSibling` if 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, `getDomSibling` would 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 `_nextDom` would 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. 1) To address the first issue, after copying the componet's VNode to `oldVNode`, we loop through the children and set their _parent pointer to `oldVNode`. 2) Instead of using `indexOf` in `getDomSibling` we will track a VNode's index in it's parent's children array as a property on the VNode, removing the need for `indexOf` in `getDomSibling`. Note: this doesn't yet fully fix the second issue ("`oldVNode` doesn'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 for `getDomSibling`. A future PR will attempt to address this problem more directly. So in summary, tl;dr: 1) After copying a component's VNode to rerender it, update the children to point to the copied VNode. 2) Manually track a child's index in its parent array to remove the dependence on `indexOf` in `getDomSibling`. Also note: #4162 also adds an `_index` property to VNodes so adding that property has multiple benefits, besides this one PR
…Index... So diffChildren can get the oldVNode out of oldChildren using _index before setting it to the VNode final position/index
|
Superseded by #4180 |
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.
Rework children diffing to run in multiple phases:
TODO: