Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use copied VNode as newVNode instead of oldVNode when rerendering #4171

Merged
merged 1 commit into from
Oct 28, 2023
Merged

Use copied VNode as newVNode instead of oldVNode when rerendering #4171

merged 1 commit into from
Oct 28, 2023

Conversation

andrewiggins
Copy link
Member

As described to in #4170, when rerendering components, we copy the component's VNode to diff against. Currently we use this copy as the oldVNode. One problem with that approach is that the copy no longer exists in the parent component's child array, so attempts to navigate around the tree using the oldVNode may break if functions like indexOf are used.

This PR changes how we do rerendering so that the copied VNode is used as the newVNode and the existing VNode is used as the oldVNode. This preserves all the links (children arrays and parent pointers) while we perform the diff.

Stated another way, this mechanism more closely matches how we typically render, where a new element is created with createElement. That new element starts as the newVNode and after diffing is attached to the tree (i.e. added to its parent child array). This new approach mirrors that by adding the newVNode to it's parent array at the right index after rerendering.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -2% - +5% (-3.00ms - +6.18ms)
    preact-local vs preact-main
  • 03_update10th1k_x16: unsure 🔍 -3% - +6% (-1.33ms - +2.45ms)
    preact-local vs preact-main
  • 07_create10k: unsure 🔍 -1% - +1% (-18.33ms - +23.98ms)
    preact-local vs preact-main
  • filter_list: unsure 🔍 -4% - +2% (-0.90ms - +0.50ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -1% - +0% (-1.02ms - +0.51ms)
    preact-local vs preact-main
  • many_updates: unsure 🔍 -7% - +2% (-1.98ms - +0.61ms)
    preact-local vs preact-main
  • text_update: unsure 🔍 -6% - +6% (-0.24ms - +0.23ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +0% (-0.35ms - +0.08ms)
    preact-local vs preact-main

usedJSHeapSize

  • 02_replace1k: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • 03_update10th1k_x16: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • 07_create10k: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • filter_list: unsure 🔍 -1% - +1% (-0.02ms - +0.02ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 +0% - +0% (+0.00ms - +0.01ms)
    preact-local vs preact-main
  • many_updates: unsure 🔍 -1% - +0% (-0.04ms - +0.02ms)
    preact-local vs preact-main
  • text_update: unsure 🔍 -3% - +3% (-0.02ms - +0.02ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main

Results

02_replace1k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main125.24ms - 131.81ms-unsure 🔍
-5% - +2%
-6.18ms - +3.00ms		unsure 🔍
-2% - +5%
-2.38ms - +6.18ms
preact-local126.91ms - 133.32msunsure 🔍
-2% - +5%
-3.00ms - +6.18ms		-unsure 🔍
-1% - +6%
-0.72ms - +7.71ms
preact-hooks123.88ms - 129.36msunsure 🔍
-5% - +2%
-6.18ms - +2.38ms		unsure 🔍
-6% - +1%
-7.71ms - +0.72ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main3.31ms - 3.32ms-unsure 🔍
-0% - +0%
-0.01ms - +0.00ms		faster ✔
0% - 1%
0.02ms - 0.02ms
preact-local3.31ms - 3.32msunsure 🔍
-0% - +0%
-0.00ms - +0.01ms		-faster ✔
0% - 1%
0.01ms - 0.02ms
preact-hooks3.33ms - 3.34msslower ❌
0% - 1%
0.02ms - 0.02ms		slower ❌
0% - 1%
0.01ms - 0.02ms		-

run-warmup-0

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main49.53ms - 52.28ms-unsure 🔍
-5% - +3%
-2.59ms - +1.60ms		unsure 🔍
-5% - +2%
-2.58ms - +1.12ms
preact-local49.81ms - 52.98msunsure 🔍
-3% - +5%
-1.60ms - +2.59ms		-unsure 🔍
-4% - +3%
-2.24ms - +1.78ms
preact-hooks50.39ms - 52.87msunsure 🔍
-2% - +5%
-1.12ms - +2.58ms		unsure 🔍
-3% - +4%
-1.78ms - +2.24ms		-

run-warmup-1

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main58.90ms - 61.42ms-unsure 🔍
-3% - +2%
-2.12ms - +1.18ms		faster ✔
1% - 7%
0.49ms - 4.61ms
preact-local59.57ms - 61.69msunsure 🔍
-2% - +4%
-1.18ms - +2.12ms		-faster ✔
0% - 6%
0.13ms - 4.03ms
preact-hooks61.08ms - 64.34msslower ❌
1% - 8%
0.49ms - 4.61ms		slower ❌
0% - 7%
0.13ms - 4.03ms		-

run-warmup-2

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main49.57ms - 51.15ms-unsure 🔍
-5% - +0%
-2.57ms - +0.12ms		faster ✔
1% - 7%
0.54ms - 3.49ms
preact-local50.50ms - 52.67msunsure 🔍
-0% - +5%
-0.12ms - +2.57ms		-unsure 🔍
-5% - +2%
-2.45ms - +0.86ms
preact-hooks51.13ms - 53.62msslower ❌
1% - 7%
0.54ms - 3.49ms		unsure 🔍
-2% - +5%
-0.86ms - +2.45ms		-

run-warmup-3

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main41.10ms - 42.13ms-faster ✔
0% - 4%
0.12ms - 1.77ms		faster ✔
0% - 5%
0.17ms - 1.99ms
preact-local41.91ms - 43.20msslower ❌
0% - 4%
0.12ms - 1.77ms		-unsure 🔍
-3% - +2%
-1.13ms - +0.85ms
preact-hooks41.94ms - 43.45msslower ❌
0% - 5%
0.17ms - 1.99ms		unsure 🔍
-2% - +3%
-0.85ms - +1.13ms		-

run-warmup-4

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main62.75ms - 68.48ms-unsure 🔍
-9% - +4%
-5.96ms - +2.75ms		unsure 🔍
-11% - +0%
-7.92ms - +0.24ms
preact-local63.93ms - 70.50msunsure 🔍
-4% - +9%
-2.75ms - +5.96ms		-unsure 🔍
-9% - +3%
-6.62ms - +2.15ms
preact-hooks66.54ms - 72.36msunsure 🔍
-1% - +12%
-0.24ms - +7.92ms		unsure 🔍
-3% - +10%
-2.15ms - +6.62ms		-

run-final

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main34.97ms - 38.06ms-unsure 🔍
-12% - +3%
-4.49ms - +1.19ms		unsure 🔍
-7% - +7%
-2.44ms - +2.56ms
preact-local35.79ms - 40.55msunsure 🔍
-3% - +12%
-1.19ms - +4.49ms		-unsure 🔍
-4% - +13%
-1.38ms - +4.79ms
preact-hooks34.50ms - 38.42msunsure 🔍
-7% - +7%
-2.56ms - +2.44ms		unsure 🔍
-12% - +3%
-4.79ms - +1.38ms		-
03_update10th1k_x16

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main42.03ms - 44.60ms-unsure 🔍
-6% - +3%
-2.45ms - +1.33ms		unsure 🔍
-4% - +5%
-1.57ms - +2.13ms
preact-local42.49ms - 45.25msunsure 🔍
-3% - +6%
-1.33ms - +2.45ms		-unsure 🔍
-3% - +6%
-1.08ms - +2.76ms
preact-hooks41.70ms - 44.37msunsure 🔍
-5% - +4%
-2.13ms - +1.57ms		unsure 🔍
-6% - +2%
-2.76ms - +1.08ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main3.26ms - 3.27ms-unsure 🔍
-0% - +0%
-0.01ms - +0.00ms		faster ✔
0% - 1%
0.01ms - 0.03ms
preact-local3.26ms - 3.27msunsure 🔍
-0% - +0%
-0.00ms - +0.01ms		-faster ✔
0% - 1%
0.01ms - 0.02ms
preact-hooks3.28ms - 3.29msslower ❌
0% - 1%
0.01ms - 0.03ms		slower ❌
0% - 1%
0.01ms - 0.02ms		-
07_create10k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main1815.42ms - 1845.83ms-unsure 🔍
-1% - +1%
-23.98ms - +18.33ms		unsure 🔍
-2% - +1%
-28.17ms - +12.60ms
preact-local1818.74ms - 1848.15msunsure 🔍
-1% - +1%
-18.33ms - +23.98ms		-unsure 🔍
-1% - +1%
-24.97ms - +15.05ms
preact-hooks1824.84ms - 1851.98msunsure 🔍
-1% - +2%
-12.60ms - +28.17ms		unsure 🔍
-1% - +1%
-15.05ms - +24.97ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main26.31ms - 26.32ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms		unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-local26.32ms - 26.32msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms		-unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-hooks26.34ms - 26.34msunsure 🔍
+0% - +0%
+0.02ms - +0.02ms		unsure 🔍
+0% - +0%
+0.02ms - +0.02ms		-
filter_list

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main25.16ms - 26.02ms-unsure 🔍
-2% - +4%
-0.50ms - +0.90ms		unsure 🔍
-3% - +3%
-0.67ms - +0.73ms
preact-local24.84ms - 25.94msunsure 🔍
-4% - +2%
-0.90ms - +0.50ms		-unsure 🔍
-4% - +2%
-0.95ms - +0.61ms
preact-hooks25.01ms - 26.11msunsure 🔍
-3% - +3%
-0.73ms - +0.67ms		unsure 🔍
-2% - +4%
-0.61ms - +0.95ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main1.34ms - 1.36ms-unsure 🔍
-1% - +2%
-0.02ms - +0.02ms		faster ✔
0% - 3%
0.00ms - 0.04ms
preact-local1.34ms - 1.36msunsure 🔍
-1% - +1%
-0.02ms - +0.02ms		-faster ✔
0% - 3%
0.01ms - 0.04ms
preact-hooks1.36ms - 1.39msslower ❌
0% - 3%
0.00ms - 0.04ms		slower ❌
0% - 3%
0.01ms - 0.04ms		-
hydrate1k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main120.63ms - 121.79ms-unsure 🔍
-0% - +1%
-0.51ms - +1.02ms		unsure 🔍
-1% - +1%
-0.71ms - +1.06ms
preact-local120.45ms - 121.45msunsure 🔍
-1% - +0%
-1.02ms - +0.51ms		-unsure 🔍
-1% - +1%
-0.92ms - +0.75ms
preact-hooks120.36ms - 121.70msunsure 🔍
-1% - +1%
-1.06ms - +0.71ms		unsure 🔍
-1% - +1%
-0.75ms - +0.92ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main6.11ms - 6.12ms-unsure 🔍
-0% - -0%
-0.01ms - -0.00ms		faster ✔
0% - 1%
0.02ms - 0.03ms
preact-local6.12ms - 6.12msunsure 🔍
+0% - +0%
+0.00ms - +0.01ms		-unsure 🔍
-0% - -0%
-0.03ms - -0.02ms
preact-hooks6.14ms - 6.14msslower ❌
0% - 1%
0.02ms - 0.03ms		unsure 🔍
+0% - +0%
+0.02ms - +0.03ms		-
many_updates

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main28.85ms - 30.88ms-unsure 🔍
-2% - +7%
-0.61ms - +1.98ms		unsure 🔍
-4% - +5%
-1.19ms - +1.44ms
preact-local28.38ms - 29.98msunsure 🔍
-7% - +2%
-1.98ms - +0.61ms		-unsure 🔍
-6% - +2%
-1.71ms - +0.59ms
preact-hooks28.91ms - 30.57msunsure 🔍
-5% - +4%
-1.44ms - +1.19ms		unsure 🔍
-2% - +6%
-0.59ms - +1.71ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main4.51ms - 4.54ms-unsure 🔍
-0% - +1%
-0.02ms - +0.04ms		unsure 🔍
-1% - +0%
-0.03ms - +0.02ms
preact-local4.50ms - 4.53msunsure 🔍
-1% - +0%
-0.04ms - +0.02ms		-unsure 🔍
-1% - +0%
-0.04ms - +0.01ms
preact-hooks4.51ms - 4.55msunsure 🔍
-0% - +1%
-0.02ms - +0.03ms		unsure 🔍
-0% - +1%
-0.01ms - +0.04ms		-
text_update

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main3.69ms - 4.02ms-unsure 🔍
-6% - +6%
-0.23ms - +0.24ms		faster ✔
2% - 13%
0.07ms - 0.56ms
preact-local3.68ms - 4.01msunsure 🔍
-6% - +6%
-0.24ms - +0.23ms		-faster ✔
2% - 13%
0.07ms - 0.57ms
preact-hooks3.99ms - 4.35msslower ❌
1% - 15%
0.07ms - 0.56ms		slower ❌
2% - 15%
0.07ms - 0.57ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main0.65ms - 0.68ms-unsure 🔍
-3% - +3%
-0.02ms - +0.02ms		unsure 🔍
-5% - +1%
-0.03ms - +0.01ms
preact-local0.65ms - 0.67msunsure 🔍
-3% - +3%
-0.02ms - +0.02ms		-unsure 🔍
-5% - +1%
-0.03ms - +0.01ms
preact-hooks0.66ms - 0.69msunsure 🔍
-1% - +5%
-0.01ms - +0.03ms		unsure 🔍
-1% - +5%
-0.01ms - +0.03ms		-
todo

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main41.92ms - 42.23ms-unsure 🔍
-0% - +1%
-0.08ms - +0.35ms		faster ✔
2% - 3%
0.89ms - 1.36ms
preact-local41.80ms - 42.09msunsure 🔍
-1% - +0%
-0.35ms - +0.08ms		-faster ✔
2% - 3%
1.03ms - 1.49ms
preact-hooks43.03ms - 43.38msslower ❌
2% - 3%
0.89ms - 1.36ms		slower ❌
2% - 4%
1.03ms - 1.49ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main0.78ms - 0.78ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms		faster ✔
3% - 3%
0.02ms - 0.02ms
preact-local0.78ms - 0.78msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-faster ✔
3% - 3%
0.02ms - 0.03ms
preact-hooks0.80ms - 0.80msslower ❌
3% - 3%
0.02ms - 0.02ms		slower ❌
3% - 3%
0.02ms - 0.03ms		-

tachometer-reporter-action v2 for Benchmarks

@coveralls
Copy link

coveralls commented Oct 26, 2023 •
edited
Loading

Coverage Status

coverage: 99.426% (-0.004%) from 99.43% when pulling 1ea23af on copy-into-newVNode into 2019d65 on main.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023 •
edited
Loading

Size Change: +69 B (0%)

Total Size: 57.3 kB

Filename Size Change
dist/preact.js 4.43 kB +5 B (0%)
dist/preact.min.js 4.46 kB +13 B (0%)
dist/preact.min.module.js 4.46 kB +14 B (0%)
dist/preact.min.umd.js 4.49 kB +12 B (0%)
dist/preact.module.js 4.46 kB +13 B (0%)
dist/preact.umd.js 4.49 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.95 kB 0 B
compat/dist/compat.module.js 3.87 kB 0 B
compat/dist/compat.umd.js 4.01 kB 0 B
debug/dist/debug.js 3.5 kB 0 B
debug/dist/debug.module.js 3.5 kB 0 B
debug/dist/debug.umd.js 3.58 kB 0 B
devtools/dist/devtools.js 231 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
hooks/dist/hooks.js 1.53 kB 0 B
hooks/dist/hooks.module.js 1.56 kB 0 B
hooks/dist/hooks.umd.js 1.62 kB 0 B
jsx-runtime/dist/jsxRuntime.js 365 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 331 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 445 B 0 B
test-utils/dist/testUtils.js 453 B 0 B
test-utils/dist/testUtils.module.js 454 B 0 B
test-utils/dist/testUtils.umd.js 536 B 0 B

compressed-size-action

marvinhagemeister
marvinhagemeister approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@andrewiggins andrewiggins merged commit 8e5eac5 into main Oct 28, 2023
12 checks passed
@andrewiggins andrewiggins deleted the copy-into-newVNode branch October 28, 2023 00:49
@JoviDeCroock JoviDeCroock mentioned this pull request Nov 3, 2023
Merged
@JoviDeCroock JoviDeCroock mentioned this pull request Nov 11, 2023
Closed
andrewiggins added a commit that referenced this pull request Nov 12, 2023
@andrewiggins
Revert "Use copied VNode as newVNode instead of oldVNode when rerende…
7b18b6a 
…ring (#4171)"

This reverts commit 8e5eac5.
This was referenced Feb 14, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@developit developit Awaiting requested review from developit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrewiggins @coveralls @marvinhagemeister @JoviDeCroock