From ef57c62cdf860c9f3658cf5ec344bc24ff6f7eb8 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Mon, 18 Dec 2023 09:17:53 -0500 Subject: [PATCH 1/5] WIP: batch commit callbacks from all components in the render queue --- .../test/browser/useSyncExternalStore.test.js | 9 +++--- src/component.js | 28 ++++++++++++++----- src/diff/index.js | 2 +- src/render.js | 2 ++ 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/compat/test/browser/useSyncExternalStore.test.js b/compat/test/browser/useSyncExternalStore.test.js index 2f5ab16a03..f800ae4b89 100644 --- a/compat/test/browser/useSyncExternalStore.test.js +++ b/compat/test/browser/useSyncExternalStore.test.js @@ -658,10 +658,11 @@ describe('useSyncExternalStore', () => { await act(() => { store.set(1); }); - // Preact logs differ from React here cuz of how we do rerendering. We - // rerender subtrees and then commit effects so Child2 never sees the - // update to 1 cuz Child1 rerenders and runs its layout effects first. - assertLog([1, /*1,*/ 'Reset back to 0', 0, 0]); + // // Preact logs differ from React here cuz of how we do rerendering. We + // // rerender subtrees and then commit effects so Child2 never sees the + // // update to 1 cuz Child1 rerenders and runs its layout effects first. + // assertLog([1, /*1,*/ 'Reset back to 0', 0, 0]); + assertLog([1, 1, 'Reset back to 0', 0, 0]); expect(container.textContent).to.equal('00'); }); diff --git a/src/component.js b/src/component.js index 48520bb375..147d5cac11 100644 --- a/src/component.js +++ b/src/component.js @@ -2,7 +2,7 @@ import { assign } from './util'; import { diff, commitRoot } from './diff/index'; import options from './options'; import { Fragment } from './create-element'; -import { MODE_HYDRATE } from './constants'; +import { EMPTY_ARR, MODE_HYDRATE } from './constants'; /** * Base Component class. Provides `setState()` and `forceUpdate()`, which @@ -120,12 +120,10 @@ export function getDomSibling(vnode, childIndex) { * Trigger in-place re-rendering of a component. * @param {Component} component The component to rerender */ -function renderComponent(component) { +function renderComponent(component, commitQueue, refQueue) { let oldVNode = component._vnode, oldDom = oldVNode._dom, - parentDom = component._parentDom, - commitQueue = [], - refQueue = []; + parentDom = component._parentDom; if (parentDom) { const newVNode = assign({}, oldVNode); @@ -146,11 +144,16 @@ function renderComponent(component) { ); newVNode._parent._children[newVNode._index] = newVNode; - commitRoot(commitQueue, newVNode, refQueue); + + newVNode._nextDom = undefined; + // if (options._commit) options._commit(newVNode, EMPTY_ARR); + // commitRoot(EMPTY_ARR, newVNode, EMPTY_ARR); if (newVNode._dom != oldDom) { updateParentDomPointers(newVNode); } + + return newVNode; } } @@ -220,21 +223,32 @@ const depthSort = (a, b) => a._vnode._depth - b._vnode._depth; /** Flush the render queue by rerendering all queued components */ function process() { let c; + let commitQueue = []; + let refQueue = []; + let root; rerenderQueue.sort(depthSort); // Don't update `renderCount` yet. Keep its value non-zero to prevent unnecessary // process() calls from getting scheduled while `queue` is still being consumed. while ((c = rerenderQueue.shift())) { if (c._dirty) { let renderQueueLength = rerenderQueue.length; - renderComponent(c); + root = renderComponent(c, commitQueue, refQueue) || root; if (rerenderQueue.length > renderQueueLength) { + commitRoot(commitQueue, root, refQueue); + commitQueue.length = 0; + refQueue.length = 0; + root = undefined; // When i.e. rerendering a provider additional new items can be injected, we want to // keep the order from top to bottom with those new items so we can handle them in a // single pass rerenderQueue.sort(depthSort); + } else if (root) { + if (options._commit) options._commit(root, EMPTY_ARR); } } } + commitRoot(commitQueue, root, refQueue); + // if (root) commitRoot(commitQueue, root, refQueue); process._rerenderCount = 0; } diff --git a/src/diff/index.js b/src/diff/index.js index b5bbb49db6..4bbed9542f 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -311,7 +311,7 @@ export function diff( * @param {VNode} root */ export function commitRoot(commitQueue, root, refQueue) { - root._nextDom = undefined; + if (root) root._nextDom = undefined; for (let i = 0; i < refQueue.length; i++) { applyRef(refQueue[i], refQueue[++i], refQueue[++i]); diff --git a/src/render.js b/src/render.js index 1ee326bc92..06d01e96d7 100644 --- a/src/render.js +++ b/src/render.js @@ -60,6 +60,8 @@ export function render(vnode, parentDom, replaceNode) { ); // Flush all queued effects + // vnode._nextDom = undefined; + // if (options._commit) options._commit(vnode, EMPTY_ARR); commitRoot(commitQueue, vnode, refQueue); } From 926b8c864d300a8942d9bf49cb2edff3e073c016 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Mon, 18 Dec 2023 13:11:53 -0500 Subject: [PATCH 2/5] run commit callbacks within the render queue to ensure render->effect->render is collapsed to one flush --- src/component.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/component.js b/src/component.js index 147d5cac11..41bff76935 100644 --- a/src/component.js +++ b/src/component.js @@ -233,7 +233,10 @@ function process() { if (c._dirty) { let renderQueueLength = rerenderQueue.length; root = renderComponent(c, commitQueue, refQueue) || root; - if (rerenderQueue.length > renderQueueLength) { + // If this WAS the last component in the queue, run commit callbacks *before* we exit the tight loop. + // This is required in order for `componentDidMount(){this.setState()}` to be batched into one flush. + // Otherwise, also run commit callbacks if the render queue was mutated. + if (renderQueueLength === 0 || rerenderQueue.length > renderQueueLength) { commitRoot(commitQueue, root, refQueue); commitQueue.length = 0; refQueue.length = 0; @@ -247,7 +250,7 @@ function process() { } } } - commitRoot(commitQueue, root, refQueue); + if (root) commitRoot(commitQueue, root, refQueue); // if (root) commitRoot(commitQueue, root, refQueue); process._rerenderCount = 0; } From b39e0bc94b9aa3b42e2dfe289106762544b11dae Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Mon, 18 Dec 2023 17:18:55 -0500 Subject: [PATCH 3/5] size optimization --- src/component.js | 2 -- src/diff/index.js | 2 -- src/render.js | 1 + 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/component.js b/src/component.js index 41bff76935..66b12d02ab 100644 --- a/src/component.js +++ b/src/component.js @@ -146,8 +146,6 @@ function renderComponent(component, commitQueue, refQueue) { newVNode._parent._children[newVNode._index] = newVNode; newVNode._nextDom = undefined; - // if (options._commit) options._commit(newVNode, EMPTY_ARR); - // commitRoot(EMPTY_ARR, newVNode, EMPTY_ARR); if (newVNode._dom != oldDom) { updateParentDomPointers(newVNode); diff --git a/src/diff/index.js b/src/diff/index.js index 4bbed9542f..16b07395a2 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -311,8 +311,6 @@ export function diff( * @param {VNode} root */ export function commitRoot(commitQueue, root, refQueue) { - if (root) root._nextDom = undefined; - for (let i = 0; i < refQueue.length; i++) { applyRef(refQueue[i], refQueue[++i], refQueue[++i]); } diff --git a/src/render.js b/src/render.js index 06d01e96d7..997678b16c 100644 --- a/src/render.js +++ b/src/render.js @@ -62,6 +62,7 @@ export function render(vnode, parentDom, replaceNode) { // Flush all queued effects // vnode._nextDom = undefined; // if (options._commit) options._commit(vnode, EMPTY_ARR); + vnode._nextDom = undefined; commitRoot(commitQueue, vnode, refQueue); } From 5f6d63e2b4ff339fcca9d891588e7d4aa2a3dbfe Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Tue, 2 Jan 2024 11:10:00 -0500 Subject: [PATCH 4/5] Update src/component.js Co-authored-by: Jovi De Croock --- src/component.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/component.js b/src/component.js index 66b12d02ab..767c3e8fd3 100644 --- a/src/component.js +++ b/src/component.js @@ -236,8 +236,7 @@ function process() { // Otherwise, also run commit callbacks if the render queue was mutated. if (renderQueueLength === 0 || rerenderQueue.length > renderQueueLength) { commitRoot(commitQueue, root, refQueue); - commitQueue.length = 0; - refQueue.length = 0; + refQueue.length = commitQueue.length = 0; root = undefined; // When i.e. rerendering a provider additional new items can be injected, we want to // keep the order from top to bottom with those new items so we can handle them in a From b09f4c132c9aa0e4d77ad1c5ab90ccf9f01d5db4 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 3 Jan 2024 11:22:48 -0500 Subject: [PATCH 5/5] remove commented-out code --- compat/test/browser/useSyncExternalStore.test.js | 4 ---- src/component.js | 1 - src/render.js | 3 --- 3 files changed, 8 deletions(-) diff --git a/compat/test/browser/useSyncExternalStore.test.js b/compat/test/browser/useSyncExternalStore.test.js index f800ae4b89..c51760cd24 100644 --- a/compat/test/browser/useSyncExternalStore.test.js +++ b/compat/test/browser/useSyncExternalStore.test.js @@ -658,10 +658,6 @@ describe('useSyncExternalStore', () => { await act(() => { store.set(1); }); - // // Preact logs differ from React here cuz of how we do rerendering. We - // // rerender subtrees and then commit effects so Child2 never sees the - // // update to 1 cuz Child1 rerenders and runs its layout effects first. - // assertLog([1, /*1,*/ 'Reset back to 0', 0, 0]); assertLog([1, 1, 'Reset back to 0', 0, 0]); expect(container.textContent).to.equal('00'); }); diff --git a/src/component.js b/src/component.js index 767c3e8fd3..a5e7fd513f 100644 --- a/src/component.js +++ b/src/component.js @@ -248,7 +248,6 @@ function process() { } } if (root) commitRoot(commitQueue, root, refQueue); - // if (root) commitRoot(commitQueue, root, refQueue); process._rerenderCount = 0; } diff --git a/src/render.js b/src/render.js index 997678b16c..b550ef96af 100644 --- a/src/render.js +++ b/src/render.js @@ -59,9 +59,6 @@ export function render(vnode, parentDom, replaceNode) { refQueue ); - // Flush all queued effects - // vnode._nextDom = undefined; - // if (options._commit) options._commit(vnode, EMPTY_ARR); vnode._nextDom = undefined; commitRoot(commitQueue, vnode, refQueue); }