Skip to content

Move renderCallbacks to VNode and queue refs as renderCallbacks#3860

Closed
andrewiggins wants to merge 10 commits into
mainfrom
feat/vnode-render-callbacks-3
Closed

Move renderCallbacks to VNode and queue refs as renderCallbacks#3860
andrewiggins wants to merge 10 commits into
mainfrom
feat/vnode-render-callbacks-3

Conversation

@andrewiggins

@andrewiggins andrewiggins commented Jan 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Ref callbacks can be used for side-effects involving DOM nodes such as setting up IntersectionObservers or measuring width/height of DOM elements for manual positioning. However in Preact today, refs are applied while we are rendering in diffChildren. For updates, this is acceptable because the DOM nodes are already attached to the document. When mounting, we apply refs after appending a DOM child to its parent. However, if that DOM node's parent hasn't been mounted yet, then the ref callback is invoked before the DOM node is attached to the document. This PR changes our ref application logic to always apply refs as render/commit callbacks to ensure they are applied after DOM elements have been fully attached to the document.

Resurrect #2098 to fix issue with ref and effect ordering.

@github-actions

github-actions Bot commented Jan 12, 2023 •
edited
Loading

Copy link
Copy Markdown

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -1% - +4% (-0.89ms - +6.31ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -4% - +4% (-2.39ms - +2.62ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -1% - +1% (-11.50ms - +15.76ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -1% - +2% (-0.30ms - +0.38ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -4% - +3% (-8.66ms - +5.47ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -1% - +6% (-0.41ms - +1.91ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 -4% - -0% (-0.11ms - +0.00ms)
    preact-local vs preact-master
  • todo: faster ✔ 0% - 2% (0.00ms - 0.80ms)
    preact-local vs preact-master

usedJSHeapSize

  • 02_replace1k: slower ❌ 1% - 2% (0.02ms - 0.06ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: slower ❌ 1% - 2% (0.04ms - 0.05ms)
    preact-local vs preact-master
  • 07_create10k: slower ❌ 1% - 2% (0.35ms - 0.41ms)
    preact-local vs preact-master
  • filter_list: slower ❌ 1% - 2% (0.01ms - 0.03ms)
    preact-local vs preact-master
  • hydrate1k: slower ❌ 1% - 3% (0.09ms - 0.17ms)
    preact-local vs preact-master
  • many_updates: slower ❌ 1% - 2% (0.05ms - 0.09ms)
    preact-local vs preact-master
  • text_update: faster ✔ 6% - 8% (0.04ms - 0.06ms)
    preact-local vs preact-master
  • todo: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-master

Results

02_replace1k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master161.87ms - 166.75ms-unsure 🔍
-4% - +1%
-6.31ms - +0.89ms		unsure 🔍
-4% - +0%
-6.53ms - +0.31ms
preact-local164.38ms - 169.66msunsure 🔍
-1% - +4%
-0.89ms - +6.31ms		-unsure 🔍
-2% - +2%
-3.96ms - +3.17ms
preact-hooks165.03ms - 169.81msunsure 🔍
-0% - +4%
-0.31ms - +6.53ms		unsure 🔍
-2% - +2%
-3.17ms - +3.96ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.37ms - 3.40ms-faster ✔
1% - 2%
0.02ms - 0.06ms		faster ✔
1% - 2%
0.04ms - 0.07ms
preact-local3.41ms - 3.44msslower ❌
1% - 2%
0.02ms - 0.06ms		-unsure 🔍
-1% - +0%
-0.03ms - +0.00ms
preact-hooks3.43ms - 3.45msslower ❌
1% - 2%
0.04ms - 0.07ms		unsure 🔍
-0% - +1%
-0.00ms - +0.03ms		-

run-warmup-0

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master49.79ms - 51.86ms-unsure 🔍
-3% - +3%
-1.60ms - +1.29ms		faster ✔
0% - 5%
0.01ms - 2.80ms
preact-local49.97ms - 51.99msunsure 🔍
-3% - +3%
-1.29ms - +1.60ms		-unsure 🔍
-5% - +0%
-2.63ms - +0.13ms
preact-hooks51.29ms - 53.17msunsure 🔍
-0% - +6%
+0.01ms - +2.80ms		unsure 🔍
-0% - +5%
-0.13ms - +2.63ms		-

run-warmup-1

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master74.58ms - 77.57ms-unsure 🔍
-3% - +3%
-2.24ms - +2.64ms		unsure 🔍
-4% - +1%
-3.47ms - +0.91ms
preact-local73.94ms - 77.80msunsure 🔍
-3% - +3%
-2.64ms - +2.24ms		-unsure 🔍
-5% - +1%
-3.99ms - +1.03ms
preact-hooks75.75ms - 78.96msunsure 🔍
-1% - +5%
-0.91ms - +3.47ms		unsure 🔍
-1% - +5%
-1.03ms - +3.99ms		-

run-warmup-2

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master55.52ms - 59.26ms-unsure 🔍
-3% - +5%
-1.71ms - +2.91ms		unsure 🔍
-8% - +0%
-4.88ms - +0.18ms
preact-local55.43ms - 58.15msunsure 🔍
-5% - +3%
-2.91ms - +1.71ms		-faster ✔
1% - 8%
0.77ms - 5.13ms
preact-hooks58.03ms - 61.44msunsure 🔍
-0% - +9%
-0.18ms - +4.88ms		slower ❌
1% - 9%
0.77ms - 5.13ms		-

run-warmup-3

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master64.17ms - 68.25ms-unsure 🔍
-7% - +2%
-4.86ms - +1.35ms		unsure 🔍
-7% - +2%
-4.56ms - +1.28ms
preact-local65.63ms - 70.31msunsure 🔍
-2% - +7%
-1.35ms - +4.86ms		-unsure 🔍
-4% - +5%
-3.02ms - +3.25ms
preact-hooks65.76ms - 69.94msunsure 🔍
-2% - +7%
-1.28ms - +4.56ms		unsure 🔍
-5% - +4%
-3.25ms - +3.02ms		-

run-warmup-4

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master70.06ms - 73.42ms-faster ✔
0% - 7%
0.20ms - 5.52ms		faster ✔
2% - 8%
1.38ms - 5.82ms
preact-local72.53ms - 76.67msslower ❌
0% - 8%
0.20ms - 5.52ms		-unsure 🔍
-4% - +2%
-3.27ms - +1.79ms
preact-hooks73.89ms - 76.79msslower ❌
2% - 8%
1.38ms - 5.82ms		unsure 🔍
-2% - +4%
-1.79ms - +3.27ms		-

run-final

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master48.74ms - 53.06ms-unsure 🔍
-10% - +0%
-5.70ms - +0.32ms		faster ✔
2% - 13%
0.95ms - 7.08ms
preact-local51.50ms - 55.68msunsure 🔍
-1% - +11%
-0.32ms - +5.70ms		-unsure 🔍
-8% - +3%
-4.34ms - +1.69ms
preact-hooks52.74ms - 57.08msslower ❌
2% - 14%
0.95ms - 7.08ms		unsure 🔍
-3% - +8%
-1.69ms - +4.34ms		-
03_update10th1k_x16

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master59.55ms - 63.24ms-unsure 🔍
-4% - +4%
-2.62ms - +2.39ms		unsure 🔍
-4% - +5%
-2.33ms - +2.95ms
preact-local59.82ms - 63.20msunsure 🔍
-4% - +4%
-2.39ms - +2.62ms		-unsure 🔍
-3% - +5%
-2.11ms - +2.96ms
preact-hooks59.19ms - 62.98msunsure 🔍
-5% - +4%
-2.95ms - +2.33ms		unsure 🔍
-5% - +3%
-2.96ms - +2.11ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.29ms - 3.30ms-faster ✔
1% - 2%
0.04ms - 0.05ms		faster ✔
2% - 2%
0.06ms - 0.07ms
preact-local3.34ms - 3.35msslower ❌
1% - 2%
0.04ms - 0.05ms		-faster ✔
0% - 1%
0.01ms - 0.02ms
preact-hooks3.36ms - 3.36msslower ❌
2% - 2%
0.06ms - 0.07ms		slower ❌
0% - 1%
0.01ms - 0.02ms		-
07_create10k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1487.50ms - 1508.12ms-unsure 🔍
-1% - +1%
-15.76ms - +11.50ms		unsure 🔍
-1% - +1%
-16.25ms - +10.27ms
preact-local1491.03ms - 1508.85msunsure 🔍
-1% - +1%
-11.50ms - +15.76ms		-unsure 🔍
-1% - +1%
-13.06ms - +11.35ms
preact-hooks1492.46ms - 1509.13msunsure 🔍
-1% - +1%
-10.27ms - +16.25ms		unsure 🔍
-1% - +1%
-11.35ms - +13.06ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master25.40ms - 25.46ms-faster ✔
1% - 2%
0.35ms - 0.41ms		faster ✔
1% - 2%
0.36ms - 0.42ms
preact-local25.80ms - 25.83msslower ❌
1% - 2%
0.35ms - 0.41ms		-unsure 🔍
-0% - +0%
-0.02ms - +0.01ms
preact-hooks25.82ms - 25.82msslower ❌
1% - 2%
0.36ms - 0.42ms		unsure 🔍
-0% - +0%
-0.01ms - +0.02ms		-
filter_list

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master22.64ms - 23.12ms-unsure 🔍
-2% - +1%
-0.38ms - +0.30ms		unsure 🔍
-3% - +0%
-0.71ms - +0.08ms
preact-local22.68ms - 23.15msunsure 🔍
-1% - +2%
-0.30ms - +0.38ms		-unsure 🔍
-3% - +0%
-0.66ms - +0.11ms
preact-hooks22.88ms - 23.50msunsure 🔍
-0% - +3%
-0.08ms - +0.71ms		unsure 🔍
-1% - +3%
-0.11ms - +0.66ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.43ms - 1.44ms-faster ✔
1% - 2%
0.01ms - 0.03ms		faster ✔
2% - 3%
0.04ms - 0.05ms
preact-local1.45ms - 1.47msslower ❌
1% - 2%
0.01ms - 0.03ms		-faster ✔
1% - 2%
0.01ms - 0.03ms
preact-hooks1.47ms - 1.49msslower ❌
2% - 3%
0.04ms - 0.05ms		slower ❌
1% - 2%
0.01ms - 0.03ms		-
hydrate1k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master187.02ms - 197.68ms-unsure 🔍
-3% - +5%
-5.47ms - +8.66ms		unsure 🔍
-3% - +4%
-6.32ms - +7.94ms
preact-local186.12ms - 195.39msunsure 🔍
-4% - +3%
-8.66ms - +5.47ms		-unsure 🔍
-4% - +3%
-7.41ms - +5.84ms
preact-hooks186.81ms - 196.27msunsure 🔍
-4% - +3%
-7.94ms - +6.32ms		unsure 🔍
-3% - +4%
-5.84ms - +7.41ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master5.99ms - 6.06ms-faster ✔
1% - 3%
0.09ms - 0.17ms		faster ✔
2% - 3%
0.12ms - 0.21ms
preact-local6.13ms - 6.18msslower ❌
1% - 3%
0.09ms - 0.17ms		-unsure 🔍
-1% - +0%
-0.08ms - +0.00ms
preact-hooks6.16ms - 6.22msslower ❌
2% - 3%
0.12ms - 0.21ms		unsure 🔍
-0% - +1%
-0.00ms - +0.08ms		-
many_updates

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master31.11ms - 32.61ms-unsure 🔍
-6% - +1%
-1.91ms - +0.41ms		unsure 🔍
-3% - +3%
-0.97ms - +0.91ms
preact-local31.72ms - 33.50msunsure 🔍
-1% - +6%
-0.41ms - +1.91ms		-unsure 🔍
-1% - +6%
-0.33ms - +1.77ms
preact-hooks31.33ms - 32.45msunsure 🔍
-3% - +3%
-0.91ms - +0.97ms		unsure 🔍
-5% - +1%
-1.77ms - +0.33ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master4.50ms - 4.53ms-faster ✔
1% - 2%
0.05ms - 0.09ms		faster ✔
2% - 3%
0.08ms - 0.12ms
preact-local4.57ms - 4.60msslower ❌
1% - 2%
0.05ms - 0.09ms		-faster ✔
0% - 1%
0.00ms - 0.04ms
preact-hooks4.60ms - 4.63msslower ❌
2% - 3%
0.08ms - 0.12ms		slower ❌
0% - 1%
0.00ms - 0.04ms		-
text_update

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master2.69ms - 2.77ms-unsure 🔍
-0% - +4%
-0.00ms - +0.11ms		faster ✔
3% - 6%
0.08ms - 0.19ms
preact-local2.64ms - 2.71msunsure 🔍
-4% - -0%
-0.11ms - +0.00ms		-faster ✔
5% - 8%
0.13ms - 0.24ms
preact-hooks2.83ms - 2.90msslower ❌
3% - 7%
0.08ms - 0.19ms		slower ❌
5% - 9%
0.13ms - 0.24ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.71ms - 0.72ms-slower ❌
6% - 8%
0.04ms - 0.06ms		slower ❌
1% - 4%
0.01ms - 0.03ms
preact-local0.66ms - 0.66msfaster ✔
6% - 8%
0.04ms - 0.06ms		-faster ✔
4% - 5%
0.03ms - 0.04ms
preact-hooks0.69ms - 0.70msfaster ✔
1% - 4%
0.01ms - 0.03ms		slower ❌
4% - 5%
0.03ms - 0.04ms		-
todo

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master51.39ms - 51.89ms-slower ❌
0% - 2%
0.00ms - 0.80ms		faster ✔
1% - 2%
0.28ms - 1.21ms
preact-local50.93ms - 51.55msfaster ✔
0% - 2%
0.00ms - 0.80ms		-faster ✔
1% - 3%
0.64ms - 1.64ms
preact-hooks51.99ms - 52.78msslower ❌
1% - 2%
0.28ms - 1.21ms		slower ❌
1% - 3%
0.64ms - 1.64ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.92ms - 0.92ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms		faster ✔
3% - 3%
0.03ms - 0.03ms
preact-local0.92ms - 0.92msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms		-faster ✔
3% - 3%
0.03ms - 0.03ms
preact-hooks0.95ms - 0.95msslower ❌
3% - 3%
0.03ms - 0.03ms		slower ❌
3% - 3%
0.03ms - 0.03ms		-

tachometer-reporter-action v2 for Benchmarks

@andrewiggins andrewiggins mentioned this pull request Jan 12, 2023
Closed
@coveralls

coveralls commented Jan 12, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown

Coverage Status

Coverage: 99.706% (+0.008%) from 99.699% when pulling 9e1ff4b on feat/vnode-render-callbacks-3 into e97da39 on master.

@github-actions

github-actions Bot commented Jan 12, 2023 •
edited
Loading

Copy link
Copy Markdown

Size Change: +2.16 kB (3%)

Total Size: 56.5 kB

Filename Size Change
compat/dist/compat.js 4.07 kB +152 B (3%)
compat/dist/compat.module.js 4 kB +157 B (3%)
compat/dist/compat.umd.js 4.13 kB +151 B (3%)
debug/dist/debug.js 3.53 kB +529 B (14%) ⚠️
debug/dist/debug.module.js 3.52 kB +515 B (14%) ⚠️
debug/dist/debug.umd.js 3.6 kB +516 B (14%) ⚠️
dist/preact.js 4.23 kB +18 B (0%)
dist/preact.min.js 4.26 kB +20 B (0%)
dist/preact.min.module.js 4.26 kB +19 B (0%)
dist/preact.min.umd.js 4.29 kB +22 B (0%)
dist/preact.module.js 4.25 kB +19 B (0%)
dist/preact.umd.js 4.29 kB +18 B (0%)
hooks/dist/hooks.js 1.53 kB +6 B (0%)
hooks/dist/hooks.module.js 1.57 kB +8 B (0%)
hooks/dist/hooks.umd.js 1.62 kB +6 B (0%)
jsx-runtime/dist/jsxRuntime.js 362 B +2 B (0%)
jsx-runtime/dist/jsxRuntime.module.js 327 B +1 B
jsx-runtime/dist/jsxRuntime.umd.js 442 B +1 B
ℹ️ View Unchanged
Filename Size Change
devtools/dist/devtools.js 232 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
test-utils/dist/testUtils.js 442 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 526 B 0 B

compressed-size-action

@andrewiggins
Move _renderCallbacks to VNode and put refs into them
fe88c58
@andrewiggins
Fix some tests
fe9873c
@andrewiggins
Fix suspense componentWillUnmount test
51b9c53
@andrewiggins
Add test asserting refs are bound before cDM is called
5854999
@andrewiggins
Bring back _renderCallbacks on component
f8b36c3 
Bring back _renderCallbacks on component to fix Suspense test. I believe this fixes the test because it allows queued render callbacks (namely componentDidMount) to persist across suspensions. So cDM is queued on initial render when the component suspends. When the component resumes, cDM is still in the queue and is flushed and called on resumption.
@andrewiggins andrewiggins force-pushed the feat/vnode-render-callbacks-3 branch from f103446 to f8b36c3 Compare March 31, 2023 23:24
@andrewiggins andrewiggins force-pushed the feat/vnode-render-callbacks-3 branch from e8718e9 to 08fbf3b Compare April 1, 2023 00:11
@andrewiggins andrewiggins force-pushed the feat/vnode-render-callbacks-3 branch from 7937956 to ebde2e4 Compare April 6, 2023 00:00
@andrewiggins

andrewiggins commented Apr 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Note: Memory will increase slightly because of this PR because I'm adding a new field to every VNode: _renderCallbacks which increases each VNode by ~4 bytes.

In v11 we can reclaim this back because we won't have _renderCallbacks on both Component and Internals. There will only be one on Internals.

@andrewiggins andrewiggins marked this pull request as ready for review April 6, 2023 21:34
@andrewiggins andrewiggins mentioned this pull request Jun 28, 2023
Merged
andrewiggins added a commit that referenced this pull request Jun 30, 2023
@andrewiggins
Add tests from #3860
1f4e464
@JoviDeCroock JoviDeCroock mentioned this pull request Sep 11, 2023
Merged
@JoviDeCroock

JoviDeCroock commented Sep 11, 2023

Copy link
Copy Markdown
Member

Closing as fixed by #4054 and #4130

@andrewiggins andrewiggins deleted the feat/vnode-render-callbacks-3 branch January 8, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewiggins @coveralls @JoviDeCroock