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.

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

Skip HTML comments during hydration #3771

Closed
wants to merge 3 commits into from
Closed

Conversation

marvinhagemeister
Copy link
Member

@marvinhagemeister marvinhagemeister commented Oct 16, 2022

We'd unnecessarily move nodes when the HTML we're going to hydrate contains comments. This is expected because it's sort of a mismatch between the SSR'd tree vs the virtual tree. But it turns out that this feature is needed for streaming SSR as comment nodes are used to indicate hydration boundaries.

Fixes #3767 .

@github-actions
Copy link

github-actions bot commented Oct 16, 2022

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -3% - +2% (-5.94ms - +3.53ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -3% - +5% (-1.37ms - +2.27ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -1% - +1% (-8.71ms - +10.34ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -2% - +0% (-0.49ms - +0.07ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -2% - +3% (-2.44ms - +3.75ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -2% - +5% (-0.43ms - +1.00ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 -2% - +2% (-0.05ms - +0.06ms)
    preact-local vs preact-master
  • todo: slower ❌ 2% - 3% (0.82ms - 1.50ms)
    preact-local vs preact-master

usedJSHeapSize

  • 02_replace1k: unsure 🔍 -12% - +0% (-0.46ms - +0.03ms)
    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.04ms - +0.02ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -1% - +6% (-0.08ms - +0.95ms)
    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
  • 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-master169.49ms - 176.26ms-unsure 🔍
-2% - +3%
-3.53ms - +5.94ms
unsure 🔍
-1% - +5%
-1.28ms - +8.28ms
preact-local168.37ms - 174.97msunsure 🔍
-3% - +2%
-5.94ms - +3.53ms
-unsure 🔍
-1% - +4%
-2.43ms - +7.02ms
preact-hooks166.00ms - 172.75msunsure 🔍
-5% - +1%
-8.28ms - +1.28ms
unsure 🔍
-4% - +1%
-7.02ms - +2.43ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.51ms - 3.99ms-unsure 🔍
-1% - +13%
-0.03ms - +0.46ms
unsure 🔍
-5% - +11%
-0.17ms - +0.39ms
preact-local3.52ms - 3.55msunsure 🔍
-12% - +0%
-0.46ms - +0.03ms
-unsure 🔍
-7% - +1%
-0.25ms - +0.04ms
preact-hooks3.50ms - 3.79msunsure 🔍
-10% - +4%
-0.39ms - +0.17ms
unsure 🔍
-1% - +7%
-0.04ms - +0.25ms
-

run-warmup-0

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master52.80ms - 55.07ms-unsure 🔍
-4% - +2%
-2.44ms - +1.24ms
unsure 🔍
-5% - -0%
-3.04ms - +0.02ms
preact-local53.09ms - 55.98msunsure 🔍
-2% - +5%
-1.24ms - +2.44ms
-unsure 🔍
-5% - +2%
-2.69ms - +0.86ms
preact-hooks54.43ms - 56.48msunsure 🔍
-0% - +6%
-0.02ms - +3.04ms
unsure 🔍
-2% - +5%
-0.86ms - +2.69ms
-

run-warmup-1

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master81.10ms - 83.38ms-unsure 🔍
-4% - +1%
-3.09ms - +0.98ms
faster ✔
2% - 7%
1.52ms - 6.14ms
preact-local81.61ms - 84.98msunsure 🔍
-1% - +4%
-0.98ms - +3.09ms
-faster ✔
0% - 6%
0.15ms - 5.40ms
preact-hooks84.06ms - 88.08msslower ❌
2% - 8%
1.52ms - 6.14ms
slower ❌
0% - 7%
0.15ms - 5.40ms
-

run-warmup-2

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master55.71ms - 60.29ms-unsure 🔍
-6% - +6%
-3.35ms - +3.39ms
unsure 🔍
-12% - +1%
-7.44ms - +0.65ms
preact-local55.50ms - 60.45msunsure 🔍
-6% - +6%
-3.39ms - +3.35ms
-unsure 🔍
-12% - +1%
-7.57ms - +0.74ms
preact-hooks58.05ms - 64.73msunsure 🔍
-1% - +13%
-0.65ms - +7.44ms
unsure 🔍
-1% - +13%
-0.74ms - +7.57ms
-

run-warmup-3

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master69.11ms - 74.37ms-unsure 🔍
-8% - +1%
-6.31ms - +1.17ms
unsure 🔍
-10% - +1%
-7.78ms - +0.81ms
preact-local71.65ms - 76.98msunsure 🔍
-2% - +9%
-1.17ms - +6.31ms
-unsure 🔍
-7% - +4%
-5.23ms - +3.40ms
preact-hooks71.83ms - 78.62msunsure 🔍
-1% - +11%
-0.81ms - +7.78ms
unsure 🔍
-5% - +7%
-3.40ms - +5.23ms
-

run-warmup-4

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master72.89ms - 76.11ms-unsure 🔍
-5% - +1%
-3.79ms - +0.81ms
unsure 🔍
-5% - +1%
-3.96ms - +0.74ms
preact-local74.35ms - 77.63msunsure 🔍
-1% - +5%
-0.81ms - +3.79ms
-unsure 🔍
-3% - +3%
-2.49ms - +2.25ms
preact-hooks74.40ms - 77.82msunsure 🔍
-1% - +5%
-0.74ms - +3.96ms
unsure 🔍
-3% - +3%
-2.25ms - +2.49ms
-

run-final

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master52.83ms - 58.68ms-unsure 🔍
-4% - +12%
-1.94ms - +6.20ms
unsure 🔍
-5% - +12%
-2.42ms - +6.31ms
preact-local50.80ms - 56.45msunsure 🔍
-11% - +3%
-6.20ms - +1.94ms
-unsure 🔍
-8% - +8%
-4.48ms - +4.12ms
preact-hooks50.57ms - 57.05msunsure 🔍
-11% - +4%
-6.31ms - +2.42ms
unsure 🔍
-8% - +8%
-4.12ms - +4.48ms
-
03_update10th1k_x16

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master47.93ms - 50.44ms-unsure 🔍
-5% - +3%
-2.27ms - +1.37ms
unsure 🔍
-6% - +2%
-2.97ms - +0.84ms
preact-local48.32ms - 50.95msunsure 🔍
-3% - +5%
-1.37ms - +2.27ms
-unsure 🔍
-5% - +3%
-2.56ms - +1.33ms
preact-hooks48.81ms - 51.68msunsure 🔍
-2% - +6%
-0.84ms - +2.97ms
unsure 🔍
-3% - +5%
-1.33ms - +2.56ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.44ms - 3.45ms-unsure 🔍
-0% - +0%
-0.01ms - +0.00ms
faster ✔
0% - 1%
0.02ms - 0.03ms
preact-local3.44ms - 3.45msunsure 🔍
-0% - +0%
-0.00ms - +0.01ms
-faster ✔
0% - 1%
0.01ms - 0.02ms
preact-hooks3.46ms - 3.47msslower ❌
0% - 1%
0.02ms - 0.03ms
slower ❌
0% - 1%
0.01ms - 0.02ms
-
07_create10k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1309.76ms - 1324.10ms-unsure 🔍
-1% - +1%
-10.34ms - +8.71ms
faster ✔
0% - 2%
0.08ms - 20.45ms
preact-local1311.47ms - 1324.01msunsure 🔍
-1% - +1%
-8.71ms - +10.34ms
-unsure 🔍
-1% - +0%
-19.02ms - +0.12ms
preact-hooks1319.96ms - 1334.43msslower ❌
0% - 2%
0.08ms - 20.45ms
unsure 🔍
-0% - +1%
-0.12ms - +19.02ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master25.40ms - 25.44ms-unsure 🔍
-0% - +0%
-0.02ms - +0.04ms
unsure 🔍
-0% - -0%
-0.07ms - -0.01ms
preact-local25.38ms - 25.43msunsure 🔍
-0% - +0%
-0.04ms - +0.02ms
-unsure 🔍
-0% - -0%
-0.08ms - -0.02ms
preact-hooks25.44ms - 25.48msunsure 🔍
+0% - +0%
+0.01ms - +0.07ms
unsure 🔍
+0% - +0%
+0.02ms - +0.08ms
-
filter_list

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master22.74ms - 23.19ms-unsure 🔍
-0% - +2%
-0.07ms - +0.49ms
unsure 🔍
-1% - +2%
-0.23ms - +0.43ms
preact-local22.58ms - 22.93msunsure 🔍
-2% - +0%
-0.49ms - +0.07ms
-unsure 🔍
-2% - +1%
-0.41ms - +0.19ms
preact-hooks22.63ms - 23.11msunsure 🔍
-2% - +1%
-0.43ms - +0.23ms
unsure 🔍
-1% - +2%
-0.19ms - +0.41ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.58ms - 1.58ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
faster ✔
1% - 1%
0.02ms - 0.02ms
preact-local1.58ms - 1.58msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-faster ✔
1% - 1%
0.02ms - 0.02ms
preact-hooks1.61ms - 1.61msslower ❌
1% - 1%
0.02ms - 0.02ms
slower ❌
1% - 1%
0.02ms - 0.02ms
-
hydrate1k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master133.52ms - 138.09ms-unsure 🔍
-3% - +2%
-3.75ms - +2.44ms
unsure 🔍
-2% - +2%
-3.02ms - +2.71ms
preact-local134.37ms - 138.55msunsure 🔍
-2% - +3%
-2.44ms - +3.75ms
-unsure 🔍
-2% - +2%
-2.22ms - +3.20ms
preact-hooks134.24ms - 137.70msunsure 🔍
-2% - +2%
-2.71ms - +3.02ms
unsure 🔍
-2% - +2%
-3.20ms - +2.22ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master14.67ms - 15.40ms-unsure 🔍
-6% - +0%
-0.95ms - +0.08ms
unsure 🔍
-5% - +1%
-0.84ms - +0.12ms
preact-local15.11ms - 15.82msunsure 🔍
-1% - +6%
-0.08ms - +0.95ms
-unsure 🔍
-3% - +4%
-0.40ms - +0.55ms
preact-hooks15.08ms - 15.70msunsure 🔍
-1% - +6%
-0.12ms - +0.84ms
unsure 🔍
-4% - +3%
-0.55ms - +0.40ms
-
many_updates

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master21.24ms - 22.22ms-unsure 🔍
-5% - +2%
-1.00ms - +0.43ms
faster ✔
1% - 6%
0.11ms - 1.37ms
preact-local21.50ms - 22.53msunsure 🔍
-2% - +5%
-0.43ms - +1.00ms
-unsure 🔍
-5% - +1%
-1.11ms - +0.20ms
preact-hooks22.07ms - 22.87msslower ❌
0% - 6%
0.11ms - 1.37ms
unsure 🔍
-1% - +5%
-0.20ms - +1.11ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master4.63ms - 4.63ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
unsure 🔍
-0% - +0%
-0.02ms - +0.00ms
preact-local4.63ms - 4.63msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-hooks4.62ms - 4.64msunsure 🔍
-0% - +0%
-0.00ms - +0.02ms
unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-
text_update

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master2.83ms - 2.89ms-unsure 🔍
-2% - +2%
-0.06ms - +0.05ms
faster ✔
5% - 8%
0.14ms - 0.24ms
preact-local2.82ms - 2.91msunsure 🔍
-2% - +2%
-0.05ms - +0.06ms
-faster ✔
4% - 8%
0.13ms - 0.24ms
preact-hooks3.01ms - 3.09msslower ❌
5% - 9%
0.14ms - 0.24ms
slower ❌
4% - 9%
0.13ms - 0.24ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.81ms - 0.81ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
faster ✔
2% - 2%
0.01ms - 0.01ms
preact-local0.81ms - 0.81msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-faster ✔
2% - 2%
0.01ms - 0.01ms
preact-hooks0.83ms - 0.83msslower ❌
2% - 2%
0.01ms - 0.01ms
slower ❌
2% - 2%
0.01ms - 0.01ms
-
todo

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master52.49ms - 53.00ms-faster ✔
2% - 3%
0.82ms - 1.50ms
faster ✔
4% - 5%
2.07ms - 2.80ms
preact-local53.68ms - 54.13msslower ❌
2% - 3%
0.82ms - 1.50ms
-faster ✔
2% - 3%
0.92ms - 1.62ms
preact-hooks54.91ms - 55.44msslower ❌
4% - 5%
2.07ms - 2.80ms
slower ❌
2% - 3%
0.92ms - 1.62ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.07ms - 1.07ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
faster ✔
2% - 3%
0.02ms - 0.03ms
preact-local1.07ms - 1.07msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-faster ✔
2% - 2%
0.02ms - 0.02ms
preact-hooks1.09ms - 1.10msslower ❌
2% - 3%
0.02ms - 0.03ms
slower ❌
2% - 2%
0.02ms - 0.02ms
-

tachometer-reporter-action v2 for Benchmarks

@coveralls
Copy link

coveralls commented Oct 16, 2022

Coverage Status

Coverage: 99.551% (+0.003%) from 99.548% when pulling d35efab on comment-nodes into 87e5083 on master.

@github-actions
Copy link

github-actions bot commented Oct 16, 2022

Size Change: +137 B (0%)

Total Size: 54.5 kB

Filename Size Change
dist/preact.js 4.23 kB +23 B (0%)
dist/preact.min.js 4.26 kB +23 B (0%)
dist/preact.min.module.js 4.26 kB +23 B (0%)
dist/preact.min.umd.js 4.29 kB +23 B (0%)
dist/preact.module.js 4.25 kB +22 B (0%)
dist/preact.umd.js 4.3 kB +23 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.91 kB 0 B
compat/dist/compat.module.js 3.84 kB 0 B
compat/dist/compat.umd.js 3.98 kB 0 B
debug/dist/debug.js 3 kB 0 B
debug/dist/debug.module.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
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
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.61 kB 0 B
jsx-runtime/dist/jsxRuntime.js 360 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 326 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 441 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

@@ -92,6 +92,7 @@ describe('hydrate()', () => {
scratch
);
expect(scratch.innerHTML).to.equal('<p><i>0</i><b>1</b></p>');
expect(getLog()).to.deep.equal(['Comment.remove()']);
Copy link
Member

Choose a reason for hiding this comment

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

It's strange that we end up removing the comment. I would have thought it would be skipped and ignored?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I assume for streaming we need to preserve these nodes until they are revived?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Ideally we would just walk over them and ignore, but I guess we need to do that in two places due to this being a new type of Node to deal with (vs skipped Text/Element nodes, which we always walk here)

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative would be to special case a branch for selective hydration where we manually walk over the comment nodes as part of diff:

if (vnode.type === Island) {
  // We know that island is surrounded with comment nodes
  oldDom = oldDom.nextSibling
}

Was thinking some more about comment nodes and I'm not sure how they would behave if we'd just skip over them. Imagine a scenario where we'll support selective hydration and have a non-hydrated subtree marked with comment nodes that is only activated when the user hovers with the mouse over the area or otherwise interacts with it. In that scenario we'd likely be rendering on the client already before all subtrees are hydrated.

This could have an effect on our ordering:

<div>
  <A />
  <B />
  <NonHydrated />
  <C />
</div>

Now, if we render on the client and need to re-order nodes due to various conditional rendering, our skip comment logic would likely bias in favor of moving the comments at the end of the childNode list.

I'm wondering if that means that we'd need to move the comment node as part of NonHydrated here. That would ensure that the order is always correct.

// in diff so that the vnode tree matches the DOM tree again.
do {
oldDom = oldDom.nextSibling;
} while (oldDom !== null && oldDom.nodeType === 8);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be possible to use a similar (or the same somehow?) check to the one we have for node types here?

preact/src/diff/index.js

Lines 358 to 359 in de08e91

'setAttribute' in child === !!nodeType &&
(nodeType ? child.localName === nodeType : child.nodeType === 3)

The other one knows what type it's looking for, so maybe it isn't possible? Just trying to think of ways to avoid checking the nodeType getter.

@JoviDeCroock
Copy link
Member

Superseded by #4128

@JoviDeCroock JoviDeCroock deleted the comment-nodes branch July 1, 2024 11:07
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.

Hydration removes and add nodes again when there are HTML comments just before them
5 participants