Skip to content

Correct _nextDom pointer if it is being unmounted#2889

Merged
marvinhagemeister merged 1 commit intomasterfrom
improve-fragment-efficiency
Dec 29, 2020
Merged

Correct _nextDom pointer if it is being unmounted#2889
marvinhagemeister merged 1 commit intomasterfrom
improve-fragment-efficiency

Conversation

@andrewiggins
Copy link
Copy Markdown
Member

While investigating ways to simplify and improve placeChild I stumbled upon a bug in how we diff Fragments. If child of a Fragment is unmounted, a Fragment's _nextDom pointer could point to this unmounted child. In situations where this happens, the Fragment's parent will correct this by re-appending all the children after the Fragment. While the DOM output is correct, the way we go about achieving it is sub-optimal. This change fixes the _nextDom to not point at a DOM that is going to be unmounted.

For example, take the following code:

<div>
  <Fragment> <!-- Fragment 1 -->
    <div>1</div>
    <div>2</div> <!-- this node will get unmounted -->
  </Fragment>
  <Fragment> <!-- Fragment 2 -->
    <div>A</div>
  </Fragment>
</div>

If doing a diff from the top where <div>2 gets unmounted, Fragment 1 should finish it's diffChildren call with a _nextDom pointer to <div>A. However, before this fix, when Fragment 1 diffs <div>1, it sets its _nextDom pointer to <div>1.nextSibling which is <div>2 (it hasn't been unmounted yet). This fix detects that the _nextDom pointer will be unmounted and resets it to the next DOM sibling, in this case <div>A.

This PR fixes a long standing Fragment issue that prevented us from asserting expected DOM operations on Fragment diffing and this PR enables us to them turn on! We can now catch regressions in how we mount & move around fragment children!

Specifically, on master the test "should handle changing node type within a Component that returns a Fragment #1326" in fragments.test.js has more than necessary dom operations. The same is true for the tests I've added. All have reduced number of DOM ops.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 29, 2020

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -2% - +0% (-2.80ms - +0.57ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -1% - +6% (-0.36ms - +1.77ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -2% - +1% (-35.69ms - +21.06ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -3% - +3% (-0.89ms - +1.05ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -2% - +1% (-2.71ms - +2.04ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -6% - +8% (-2.39ms - +3.19ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 -1% - +5% (-0.03ms - +0.18ms)
    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.00ms - +0.01ms)
    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.01ms - +0.00ms)
    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: 80
  • Built by: CI #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master182.77ms - 185.48ms-unsure 🔍
-0% - +2%
-0.57ms - +2.80ms
preact-local182.01ms - 184.01msunsure 🔍
-2% - +0%
-2.80ms - +0.57ms
-

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-master67.94ms - 69.82ms-unsure 🔍
-1% - +3%
-0.37ms - +1.79ms
preact-local67.64ms - 68.70msunsure 🔍
-3% - +1%
-1.79ms - +0.37ms
-

run-warmup-1

VersionAvg timevs preact-mastervs preact-local
preact-master104.48ms - 106.23ms-unsure 🔍
-1% - +1%
-1.37ms - +0.91ms
preact-local104.86ms - 106.32msunsure 🔍
-1% - +1%
-0.91ms - +1.37ms
-

run-warmup-2

VersionAvg timevs preact-mastervs preact-local
preact-master121.01ms - 122.96ms-unsure 🔍
-0% - +3%
-0.34ms - +3.56ms
preact-local118.68ms - 122.06msunsure 🔍
-3% - +0%
-3.56ms - +0.34ms
-

run-warmup-3

VersionAvg timevs preact-mastervs preact-local
preact-master66.15ms - 67.94ms-unsure 🔍
-3% - +1%
-2.12ms - +0.91ms
preact-local66.43ms - 68.87msunsure 🔍
-1% - +3%
-0.91ms - +2.12ms
-

run-warmup-4

VersionAvg timevs preact-mastervs preact-local
preact-master124.18ms - 130.77ms-unsure 🔍
-4% - +3%
-4.69ms - +4.04ms
preact-local124.94ms - 130.66msunsure 🔍
-3% - +4%
-4.04ms - +4.69ms
-

run-final

VersionAvg timevs preact-mastervs preact-local
preact-master65.64ms - 66.43ms-unsure 🔍
-1% - +1%
-0.62ms - +0.72ms
preact-local65.44ms - 66.53msunsure 🔍
-1% - +1%
-0.72ms - +0.62ms
-
03_update10th1k_x16
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 130
  • Built by: CI #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master29.44ms - 30.97ms-unsure 🔍
-6% - +1%
-1.77ms - +0.36ms
preact-local30.16ms - 31.65msunsure 🔍
-1% - +6%
-0.36ms - +1.77ms
-

usedJSHeapSize

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

duration

VersionAvg timevs preact-mastervs preact-local
preact-master1880.07ms - 1919.06ms-unsure 🔍
-1% - +2%
-21.06ms - +35.69ms
preact-local1871.63ms - 1912.86msunsure 🔍
-2% - +1%
-35.69ms - +21.06ms
-

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 #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master30.07ms - 31.47ms-unsure 🔍
-3% - +3%
-1.05ms - +0.89ms
preact-local30.18ms - 31.53msunsure 🔍
-3% - +3%
-0.89ms - +1.05ms
-

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 #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master154.49ms - 158.02ms-unsure 🔍
-1% - +2%
-2.04ms - +2.71ms
preact-local154.33ms - 157.51msunsure 🔍
-2% - +1%
-2.71ms - +2.04ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master6.19ms - 6.21ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-local6.19ms - 6.19msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms
-
many_updates
  • Browser: chrome-headless 87.0.4280.88
  • Sample size: 80
  • Built by: CI #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master36.98ms - 40.97ms-unsure 🔍
-8% - +6%
-3.19ms - +2.39ms
preact-local37.42ms - 41.33msunsure 🔍
-6% - +8%
-2.39ms - +3.19ms
-

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: 60
  • Built by: CI #616
  • Commit: a42da10

duration

VersionAvg timevs preact-mastervs preact-local
preact-master3.53ms - 3.62ms-unsure 🔍
-5% - +1%
-0.18ms - +0.03ms
preact-local3.56ms - 3.74msunsure 🔍
-1% - +5%
-0.03ms - +0.18ms
-

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

@github-actions
Copy link
Copy Markdown

Size Change: +100 B (0%)

Total Size: 41.6 kB

Filename Size Change
dist/preact.js 4.04 kB +23 B (0%)
dist/preact.min.js 4.07 kB +25 B (0%)
dist/preact.module.js 4.06 kB +25 B (0%)
dist/preact.umd.js 4.11 kB +27 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.23 kB 0 B
compat/dist/compat.module.js 3.23 kB 0 B
compat/dist/compat.umd.js 3.28 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
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

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 2150fc8 on improve-fragment-efficiency into 264c5e5 on master.

expect(ops).to.deep.equal([]); // Component should not have updated (empty op log)
expect(scratch.innerHTML).to.equal(html);
expectDomLogToBe([
'<div>1Hello1.insertBefore(<span>1, <span>1)',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This original dom ops were just wrong. They don't match what the test was doing.

clearLog();
render(<Foo condition={false} />, scratch);
expect(scratch.innerHTML).to.equal(html, 'rendering from true to false');
expectDomLogToBe(
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since this test was written we converted all nested arrays to Fragments so in this test where DOM ops previously happened (due to nested arrays previously being flattened) there are now none since nested arrays become Fragments

Copy link
Copy Markdown
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Nice find!

@marvinhagemeister marvinhagemeister merged commit 91016bc into master Dec 29, 2020
@marvinhagemeister marvinhagemeister deleted the improve-fragment-efficiency branch December 29, 2020 09:31
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.

4 participants