Move renderCallbacks to VNode and queue refs as renderCallbacks#2098
Closed
andrewiggins wants to merge 11 commits into
Closed
Move renderCallbacks to VNode and queue refs as renderCallbacks#2098andrewiggins wants to merge 11 commits into
andrewiggins wants to merge 11 commits into
Conversation
andrewiggins
commented
Nov 7, 2019
| _dom: null, | ||
| _lastDomChild: null, | ||
| _component: null, | ||
| _renderCallbacks: null, |
Member
Author
There was a problem hiding this comment.
FYI, initializing this to an array saves us about 11 B 9B. However it does add an extra array allocation for every VNode :/ Unsure how that affects performance. Perhaps someone can investigate this and open a new PR if its worth it?
andrewiggins
commented
Nov 7, 2019
| ); | ||
|
|
||
| if ((j = childVNode.ref) && oldVNode.ref != j) { | ||
| if (!refs) refs = []; |
Member
Author
There was a problem hiding this comment.
Yay! This array allocation is gone! Now if you attach a ref to a component, we re-use that component's _renderCallbacks array. However, if you attach a ref to a DOM VNode, we allocate a new array (that VNode's _renderCallbacks array) for that ref. But I think the net is still fewer array allocations.
Member
|
Hey, Could we add a test here (can be skipped) to see if the refs are bound in ComponentDidMount? Specific example: The Child for example has a span with ref={ref} and in |
Member
Author
|
Oh good call! Will do |
andrewiggins
commented
Nov 13, 2019
| // increment is never cleared once the component suspends. So when it | ||
| // resumes and the component is rerendered, we queue up another cDU so | ||
| // cDU is called an extra time. | ||
| expect(componentDidUpdate).to.have.been.calledThrice; |
Member
Author
There was a problem hiding this comment.
Since _renderCallbacks is stored on VNode now, they don't persist across renders. If a VNode suspends, it'll never get added to the commitQueue array so its _renderCallbacks won't get called. And since VNodes are recreated on each render it won't get invoked on the next render since that'll be with a newVNode, and not the one from the previous render which had the _renderCallback.
bd2635f to
acf8195
Compare
|
Size Change: -37 B (0%) Total Size: 42.3 kB
ℹ️ View Unchanged
|
… (-20 B) Some test still fail cause useLayoutEffects are scheduled before the refs are :(
Since _renderCallbacks is stored on VNode now, if a VNode suspends, it'll never get added to the commitQueue array so its _renderCallbacks won't get called. And since VNodes are recreated on each render it won't get invoked on the next render since that'll be with a newVNode
acf8195 to
dd44824
Compare
📊 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
|
Member
Author
|
Superseded by #3860 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In the spirit of #2011, queue the
applyRefcalls as_renderCallbacksso they are invoked after the entire DOM is updated. This change effectively undos 40212eb from #1658.I always didn't like how I had to move ref handling to
diffChildren. That had to be done in #1658 because refs needed to be unmounted first, then the new value applied, for example, if the VNode changed tag type fromdivtospan. We would need to call therefwithnullwhen thedivis unmounted, then call it again with thespanin that order. This PR accomplishes the same behavior by queuing the setting of the new ref value in_renderCallbacksafter all unmounts and DOM manipulations are done.