Skip to content

Conversation

@JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Mar 17, 2025

Fixes preactjs/signals#659

Avoid cloning nodes that we are re-using, the diffChildren algorithm will already ensure we clone these and we do it in a more correct way there https://github.com/preactjs/preact/blob/main/src/diff/children.js#L210. In signals we rely on this cloning with https://github.com/preactjs/signals/blob/main/packages/preact/src/index.ts#L164

I have yet to test this fix, but this looks like the right approach 😅

EDIT: does seem to work https://stackblitz.com/edit/vitejs-vite-jdff8rn3?file=src%2Fsignals.js,src%2Fapp.tsx,src%2Fpreact.js,src%2Fhooks.js,src%2Fmain.tsx

@github-actions
Copy link

github-actions bot commented Mar 17, 2025

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -0% - +1% (-2.42ms - +11.57ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -1% - +1% (-0.21ms - +0.11ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -1% - +1% (-1.17ms - +1.00ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +1% (-0.15ms - +0.12ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +2% (-0.10ms - +1.46ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -2% - +5% (-0.04ms - +0.11ms)
    preact-local vs preact-main
  • todo: faster ✔ 0% - 2% (0.12ms - 0.59ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -4% - +3% (-1.39ms - +1.13ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -2% - +2% (-0.03ms - +0.03ms)
    preact-local vs preact-main
  • hydrate1k: faster ✔ 2% - 11% (0.13ms - 0.94ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +1% (-0.02ms - +0.05ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -1% - +0% (-0.04ms - +0.00ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +1% (-0.06ms - +0.02ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +2% (-0.01ms - +0.02ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -1% - +1% (-0.03ms - +0.02ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local938.03ms - 949.68ms-unsure 🔍
-0% - +1%
-2.42ms - +11.57ms
preact-main935.40ms - 943.15msunsure 🔍
-1% - +0%
-11.57ms - +2.42ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local19.19ms - 19.20ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main19.19ms - 19.20msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.55ms - 16.61ms-unsure 🔍
-1% - +1%
-0.21ms - +0.11ms
preact-main16.48ms - 16.78msunsure 🔍
-1% - +1%
-0.11ms - +0.21ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.56ms - 1.60ms-unsure 🔍
-2% - +2%
-0.03ms - +0.03ms
preact-main1.56ms - 1.60msunsure 🔍
-2% - +2%
-0.03ms - +0.03ms
-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local77.78ms - 79.37ms-unsure 🔍
-1% - +1%
-1.17ms - +1.00ms
preact-main77.91ms - 79.39msunsure 🔍
-1% - +1%
-1.00ms - +1.17ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local7.92ms - 8.50ms-faster ✔
2% - 11%
0.13ms - 0.94ms
preact-main8.46ms - 9.03msslower ❌
1% - 12%
0.13ms - 0.94ms
-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.40ms - 16.63ms-unsure 🔍
-1% - +1%
-0.15ms - +0.12ms
preact-main16.46ms - 16.60msunsure 🔍
-1% - +1%
-0.12ms - +0.15ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.82ms - 3.86ms-unsure 🔍
-1% - +1%
-0.02ms - +0.05ms
preact-main3.80ms - 3.85msunsure 🔍
-1% - +1%
-0.05ms - +0.02ms
-
replace1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local74.02ms - 75.08ms-unsure 🔍
-0% - +2%
-0.10ms - +1.46ms
preact-main73.30ms - 74.44msunsure 🔍
-2% - +0%
-1.46ms - +0.10ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.99ms - 3.01ms-unsure 🔍
-1% - +0%
-0.04ms - +0.00ms
preact-main3.00ms - 3.04msunsure 🔍
-0% - +1%
-0.00ms - +0.04ms
-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local31.01ms - 31.66ms-unsure 🔍
-2% - +1%
-0.79ms - +0.24ms
preact-main31.21ms - 32.01msunsure 🔍
-1% - +3%
-0.24ms - +0.79ms
-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local35.25ms - 36.32ms-faster ✔
0% - 4%
0.10ms - 1.58ms
preact-main36.12ms - 37.14msslower ❌
0% - 4%
0.10ms - 1.58ms
-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local27.36ms - 27.97ms-unsure 🔍
-0% - +2%
-0.09ms - +0.63ms
preact-main27.20ms - 27.59msunsure 🔍
-2% - +0%
-0.63ms - +0.09ms
-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local27.99ms - 28.69ms-unsure 🔍
-2% - +2%
-0.51ms - +0.53ms
preact-main27.94ms - 28.72msunsure 🔍
-2% - +2%
-0.53ms - +0.51ms
-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local28.16ms - 28.81ms-unsure 🔍
-0% - +3%
-0.11ms - +0.89ms
preact-main27.71ms - 28.47msunsure 🔍
-3% - +0%
-0.89ms - +0.11ms
-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local21.30ms - 21.68ms-unsure 🔍
-2% - +1%
-0.34ms - +0.27ms
preact-main21.28ms - 21.76msunsure 🔍
-1% - +2%
-0.27ms - +0.34ms
-
text-update
  • Browser: chrome-headless
  • Sample size: 230
  • Built by: CI #4627
  • Commit: e6a489d

duration

VersionAvg timevs preact-localvs preact-main
preact-local2.08ms - 2.21ms-unsure 🔍
-2% - +5%
-0.04ms - +0.11ms
preact-main2.07ms - 2.16msunsure 🔍
-5% - +2%
-0.11ms - +0.04ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.17ms - 1.22ms-unsure 🔍
-5% - +1%
-0.06ms - +0.02ms
preact-main1.19ms - 1.25msunsure 🔍
-1% - +5%
-0.02ms - +0.06ms
-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local34.39ms - 34.69ms-faster ✔
0% - 2%
0.12ms - 0.59ms
preact-main34.71ms - 35.07msslower ❌
0% - 2%
0.12ms - 0.59ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.24ms - 1.26ms-unsure 🔍
-1% - +2%
-0.01ms - +0.02ms
preact-main1.23ms - 1.26msunsure 🔍
-2% - +1%
-0.02ms - +0.01ms
-
update10th1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local31.13ms - 32.97ms-unsure 🔍
-4% - +3%
-1.39ms - +1.13ms
preact-main31.33ms - 33.05msunsure 🔍
-4% - +4%
-1.13ms - +1.39ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.96ms - 2.98ms-unsure 🔍
-1% - +1%
-0.03ms - +0.02ms
preact-main2.96ms - 2.99msunsure 🔍
-1% - +1%
-0.02ms - +0.03ms
-

tachometer-reporter-action v2 for CI

@github-actions
Copy link

github-actions bot commented Mar 17, 2025

Size Change: +44 B (+0.06%)

Total Size: 78.5 kB

Filename Size Change
dist/preact.js 4.75 kB +6 B (+0.13%)
dist/preact.min.js 4.77 kB +7 B (+0.15%)
dist/preact.min.module.js 4.77 kB +6 B (+0.13%)
dist/preact.min.umd.js 4.8 kB +7 B (+0.15%)
dist/preact.mjs 4.76 kB +5 B (+0.11%)
dist/preact.module.js 4.76 kB +5 B (+0.11%)
dist/preact.umd.js 4.8 kB +8 B (+0.17%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.12 kB
compat/dist/compat.mjs 4.05 kB
compat/dist/compat.module.js 4.05 kB
compat/dist/compat.umd.js 4.19 kB
debug/dist/debug.js 3.82 kB
debug/dist/debug.mjs 3.82 kB
debug/dist/debug.module.js 3.82 kB
debug/dist/debug.umd.js 3.9 kB
devtools/dist/devtools.js 260 B
devtools/dist/devtools.mjs 274 B
devtools/dist/devtools.module.js 274 B
devtools/dist/devtools.umd.js 346 B
hooks/dist/hooks.js 1.54 kB
hooks/dist/hooks.mjs 1.57 kB
hooks/dist/hooks.module.js 1.57 kB
hooks/dist/hooks.umd.js 1.61 kB
jsx-runtime/dist/jsxRuntime.js 978 B
jsx-runtime/dist/jsxRuntime.mjs 952 B
jsx-runtime/dist/jsxRuntime.module.js 952 B
jsx-runtime/dist/jsxRuntime.umd.js 1.05 kB
test-utils/dist/testUtils.js 473 B
test-utils/dist/testUtils.mjs 477 B
test-utils/dist/testUtils.module.js 477 B
test-utils/dist/testUtils.umd.js 555 B

compressed-size-action

@coveralls
Copy link

coveralls commented Mar 17, 2025

Coverage Status

coverage: 99.61%. remained the same
when pulling e6a489d on avoid-cloning-reused-nodes
into 64242f4 on main.

@JoviDeCroock JoviDeCroock force-pushed the avoid-cloning-reused-nodes branch 2 times, most recently from 683e355 to 230ae8d Compare March 18, 2025 09:55
@JoviDeCroock JoviDeCroock merged commit adb1414 into main Mar 18, 2025
12 checks passed
@JoviDeCroock JoviDeCroock deleted the avoid-cloning-reused-nodes branch March 18, 2025 10:14
@JoviDeCroock JoviDeCroock mentioned this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#4716l Causing issue with signals

4 participants