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

Fix child reordering with memo #2896

Merged
merged 1 commit into from
Jan 1, 2021
Merged

Fix child reordering with memo #2896

merged 1 commit into from
Jan 1, 2021

Conversation

andrewiggins
Copy link
Member

Read the return value of reorderChildren in reorderChildren so that the new value of oldDom bubbles up through the recursion. This update ensures the next iteration of the loop inside diffChildren begins with the correct oldDom value

Fixes #2895

@github-actions
Copy link

github-actions bot commented Jan 1, 2021 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -1% - +2% (-1.58ms - +4.13ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -5% - +3% (-1.54ms - +0.82ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -2% - +2% (-33.17ms - +32.31ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -1% - +3% (-0.30ms - +0.75ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -1% - +1% (-1.71ms - +1.73ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -9% - +4% (-4.72ms - +2.03ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 -2% - +2% (-0.08ms - +0.09ms)
    preact-local vs preact-master

usedJSHeapSize

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

Results

02_replace1k
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 60
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master196.16ms - 200.74ms-unsure 🔍
-2% - +1%
-4.13ms - +1.58ms
preact-local198.02ms - 201.43msunsure 🔍
-1% - +2%
-1.58ms - +4.13ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master3.60ms - 3.60ms-unsure 🔍
-0% - +0%
-0.01ms - +0.00ms
preact-local3.60ms - 3.61msunsure 🔍
-0% - +0%
-0.00ms - +0.01ms		-

run-warmup-0

VersionAvg timevs preact-mastervs preact-local
preact-master72.78ms - 74.51ms-unsure 🔍
-2% - +1%
-1.32ms - +1.00ms
preact-local73.03ms - 74.58msunsure 🔍
-1% - +2%
-1.00ms - +1.32ms		-

run-warmup-1

VersionAvg timevs preact-mastervs preact-local
preact-master110.76ms - 112.39ms-unsure 🔍
-2% - +1%
-2.17ms - +0.90ms
preact-local110.92ms - 113.51msunsure 🔍
-1% - +2%
-0.90ms - +2.17ms		-

run-warmup-2

VersionAvg timevs preact-mastervs preact-local
preact-master126.39ms - 131.11ms-unsure 🔍
-2% - +2%
-3.13ms - +3.15ms
preact-local126.67ms - 130.81msunsure 🔍
-2% - +2%
-3.15ms - +3.13ms		-

run-warmup-3

VersionAvg timevs preact-mastervs preact-local
preact-master73.12ms - 76.92ms-unsure 🔍
-4% - +3%
-2.82ms - +2.15ms
preact-local73.75ms - 76.95msunsure 🔍
-3% - +4%
-2.15ms - +2.82ms		-

run-warmup-4

VersionAvg timevs preact-mastervs preact-local
preact-master140.05ms - 144.90ms-unsure 🔍
-2% - +4%
-2.26ms - +5.63ms
preact-local137.68ms - 143.90msunsure 🔍
-4% - +2%
-5.63ms - +2.26ms		-

run-final

VersionAvg timevs preact-mastervs preact-local
preact-master74.30ms - 76.84ms-unsure 🔍
-2% - +2%
-1.67ms - +1.47ms
preact-local74.75ms - 76.59msunsure 🔍
-2% - +2%
-1.47ms - +1.67ms		-
03_update10th1k_x16
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 130
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master30.07ms - 31.69ms-unsure 🔍
-3% - +5%
-0.82ms - +1.54ms
preact-local29.66ms - 31.37msunsure 🔍
-5% - +3%
-1.54ms - +0.82ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master3.52ms - 3.53ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-local3.52ms - 3.53msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms		-
07_create10k
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 50
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master1820.38ms - 1865.59ms-unsure 🔍
-2% - +2%
-32.31ms - +33.17ms
preact-local1818.87ms - 1866.24msunsure 🔍
-2% - +2%
-33.17ms - +32.31ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master25.99ms - 25.99ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-local25.99ms - 25.99msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-
filter_list
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 50
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master28.75ms - 29.56ms-unsure 🔍
-3% - +1%
-0.75ms - +0.30ms
preact-local29.04ms - 29.71msunsure 🔍
-1% - +3%
-0.30ms - +0.75ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master1.60ms - 1.60ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-local1.60ms - 1.60msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-
hydrate1k
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 50
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master136.10ms - 138.48ms-unsure 🔍
-1% - +1%
-1.73ms - +1.71ms
preact-local136.06ms - 138.55msunsure 🔍
-1% - +1%
-1.71ms - +1.73ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master6.19ms - 6.21ms-unsure 🔍
-0% - +0%
-0.02ms - +0.02ms
preact-local6.19ms - 6.22msunsure 🔍
-0% - +0%
-0.02ms - +0.02ms		-
many_updates
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 70
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master47.30ms - 52.12ms-unsure 🔍
-4% - +10%
-2.03ms - +4.72ms
preact-local46.00ms - 50.72msunsure 🔍
-9% - +4%
-4.72ms - +2.03ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master4.85ms - 4.85ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-local4.85ms - 4.85msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-
text_update
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 50
  • Built by: CI #637

  • Commit: 8293aad

duration

VersionAvg timevs preact-mastervs preact-local
preact-master3.63ms - 3.74ms-unsure 🔍
-2% - +2%
-0.09ms - +0.08ms
preact-local3.63ms - 3.75msunsure 🔍
-2% - +2%
-0.08ms - +0.09ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master0.83ms - 0.83ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-local0.83ms - 0.83msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-

tachometer-reporter-action v2 for CI

andrewiggins
andrewiggins commented Jan 1, 2021
View reviewed changes
test/browser/fragments.test.js
@@ -568,7 +568,7 @@ describe('Fragment', () => {
expect(scratch.innerHTML).to.equal('<div>Hello</div>');
});

it.skip('should preserve state between double nested fragment and double nested array', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was skipped in the world where we flattened arrays. Since we now convert arrays to Fragments it passes. Went ahead and enabled it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.607% when pulling db4da7a on child-reordering-fix into a9f7e67 on master.

@github-actions
Copy link

github-actions bot commented Jan 1, 2021

Size Change: 0 B

Total Size: 41.6 kB

ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.23 kB 0 B
compat/dist/compat.module.js 3.24 kB 0 B
compat/dist/compat.umd.js 3.29 kB 0 B
debug/dist/debug.js 2.99 kB 0 B
debug/dist/debug.module.js 2.98 kB 0 B
debug/dist/debug.umd.js 3.07 kB 0 B
devtools/dist/devtools.js 185 B 0 B
devtools/dist/devtools.module.js 195 B 0 B
devtools/dist/devtools.umd.js 261 B 0 B
dist/preact.js 4.03 kB 0 B
dist/preact.min.js 4.06 kB 0 B
dist/preact.module.js 4.05 kB 0 B
dist/preact.umd.js 4.1 kB 0 B
hooks/dist/hooks.js 1.13 kB 0 B
hooks/dist/hooks.module.js 1.15 kB 0 B
hooks/dist/hooks.umd.js 1.2 kB 0 B
jsx-runtime/dist/jsxRuntime.js 327 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 335 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 405 B 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

marvinhagemeister
marvinhagemeister approved these changes Jan 1, 2021
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.

This is fantastic 🙌

@marvinhagemeister marvinhagemeister merged commit ea3b0ac into master Jan 1, 2021
@marvinhagemeister marvinhagemeister deleted the child-reordering-fix branch January 1, 2021 18:56
@jembach jembach mentioned this pull request Jan 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

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

Successfully merging this pull request may close these issues.

Child order is not maintained between re-renders, when using compat memo
4 participants
@andrewiggins @coveralls @marvinhagemeister @JoviDeCroock