refactor(engine-core): POC splitting up diffing from rehydration #2608

pmdartus · 2021-12-16T15:43:12Z

Details

This PR is a proof-of-concept for splitting up the diffing logic from the rehydration logic. VNode hooks are mixing up today both diffing and hydration logics. By removing the concept of hooks, it becomes quite straightforward to split up to the logic.

The VNode hook field can be replaced with a type field. The type field is a new field present on all VNodes. It is used to invoke the appropriate mounting or patching function. One of the nice side-effects of this change is that it makes the diffing algo code way easier to follow and resonate about. With this change the code is now broken down:

src/framework/vnode.ts: VNode type definition
src/framework/rendering.ts: Component mounting and diffing logic with patchChildren for entry point
src/framework/hydration.ts: Component rehydration logic with hydrateChildren for entry point

Here are some other observations related to this change:

This code path in src/framework/modules/events.ts is never invoked.
The existing hooks.ts contains some duplicated code that can be simplified.
I ran some performance tests on VDOM traversal and it appears that JavaScript engines are capable to traverse deep VDOM trees quite fast. I am not convinced that the added complexity introduced by VM.velements is worth the performance improvement.
Remove the VNode.owner is not as simple as what I originally thought, mainly because of slotted content.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

nolanlawson · 2021-12-16T22:27:25Z

packages/@lwc/engine-core/src/framework/hydration.ts

+
+function throwHydrationError() {
+    assert.fail('Server rendered elements do not match client side generated elements');
+}


One benefit of having hydration in one place is that we can potentially remove this code for environments that don't use hydration. Svelte has a hydratable option that it uses for such optimizations: sveltejs/svelte#6525

nolanlawson · 2021-12-16T22:28:50Z

Remove the VNode.owner is not as simple as what I originally thought

Merging #2598 will make it easier to remove owner, because there's no more need for owner.renderer. 🙂

jodarove · 2022-01-07T13:47:57Z

packages/@lwc/engine-core/src/framework/rendering.ts

+            break;
+
+        case VNodeType.CustomElement: {
+            const vm = getAssociatedVMIfPresent(vnode.elm);


@pmdartus in your original refactor, we were doing vnode.vm, and I changed it back to getAssociatedVMIfPresent. What get's my attention is that it only was observable on the hydration tests. I can say for sure out the top of my mind, but maybe we should set a VM in the vnode as part of the hydration process?

jodarove

@pmdartus first pass, all looking good. i still need to dive into patch/mount(Element/customElement)

jodarove · 2022-01-10T16:56:49Z

packages/@lwc/engine-core/src/framework/api.ts

        ctor: Ctor,
        owner: vmBeingRendered,
        mode: 'open', // TODO [#1294]: this should be defined in Ctor
+        vm,


where is this vm being set?

jodarove · 2022-01-10T18:40:22Z

packages/@lwc/engine-core/src/framework/hydration.ts

+function processElement(vnode: VElement, node: Node) {
+    const elm = node as Element;


Suggested change

function processElement(vnode: VElement, node: Node) {

const elm = node as Element;

function processElement(vnode: VElement, elm: Element) {

jodarove · 2022-01-10T18:40:50Z

packages/@lwc/engine-core/src/framework/hydration.ts

+            break;
+
+        case VNodeType.Element:
+            processElement(vnode, node);


Suggested change

processElement(vnode, node);

processElement(vnode, node as Element);

jodarove · 2022-01-10T18:41:32Z

packages/@lwc/engine-core/src/framework/hydration.ts

+function processCustomElement(vnode: VCustomElement, node: Node) {
+    const elm = node as Element;
+


Suggested change

function processCustomElement(vnode: VCustomElement, node: Node) {

const elm = node as Element;

function processCustomElement(vnode: VCustomElement, elm: Element) {

jodarove · 2022-01-10T18:41:57Z

packages/@lwc/engine-core/src/framework/hydration.ts

+            break;
+
+        case VNodeType.CustomElement:
+            processCustomElement(vnode, node);


Suggested change

processCustomElement(vnode, node);

processCustomElement(vnode, node as Element);

jodarove · 2022-01-10T20:36:28Z

packages/@lwc/engine-core/src/framework/modules/events.ts

-        renderer.addEventListener(elm, name, listener);
+    for (const name in on) {
+        const handler = on[name];
+        renderer.addEventListener(elm, name, (event: Event) => {


Some questions:

before we used to have 1 handler for all events, now we have 1 handler per event (as many handlers as events). Pls, confirm.

why can we skip on && on[type] check? because it will always be defined? is the handler is our own invokeEventListener from the bind function?

why not just do renderer.addEventListener(elm, name, handler);?

jodarove · 2022-01-10T21:22:43Z

packages/@lwc/engine-core/src/framework/rendering.ts

+    }
+}
+
+function patch(n1: VNode | null, n2: VNode, parent: ParentNode, anchor: Node | null) {


@pmdartus something feels off about this function. basically is doing 2 different things based on the parameters. if n1 is null, then is mounting (create+insert right?), otherwise patching (I'm still trying to figure out the fix me comment). Then in the process* we make the same differentiation with a condition for isNull (mount/patch).

What do you think if we separate the patch logic into patch and mount, and also the process* into patch* and mount* (you already have them for Element and CustomeElements)? (imho) Such a way is clearer, and we can skip the multiple isNull check which are on the critical path of the diffing algo.

jodarove · 2022-01-10T21:31:24Z

packages/@lwc/engine-core/src/framework/rendering.ts

+    } else {
+        n2.elm = n1.elm;
+
+        // FIXME: Comment nodes should be static, we shouldn't need to diff them together. However


true, but, iirc text and comments do not have keys. I'm thinking of foreachs, maybe ifs (this is the same as texts), in which the diff may reuse other vnodses. we diff the texts so we do not need to access the DOM API.

jodarove · 2022-01-10T21:33:55Z

packages/@lwc/engine-core/src/framework/rendering.ts

+        // FIXME: Comment nodes should be static, we shouldn't need to diff them together. However
+        // it is the case today.
+        if (n2.text !== n1.text) {
+            updateTextContent(n2);


I like the abstraction. don't know if will help but here is super micro-optimization: we could pass (node, renderer, text) and avoid destructing the vnode in updateTextContent. we have all those here.

jodarove

couldn't find any issues in the individual hooks migrations. just one question

jodarove · 2022-01-11T15:46:45Z

packages/@lwc/engine-core/src/framework/rendering.ts

+        throw new TypeError(`Incorrect Component Constructor`);
+    }
+
+    applyStyleScoping(elm, vnode);


applyStyleScoping was done within the callback to UpgradableConstructor (part of createViewModelHook). Do you foresee any issues moving it outside?

nolanlawson · 2022-01-11T17:17:06Z

Sorry for bitrotting this PR; I really like the direction of getting rid of hooks.

pmdartus · 2022-01-12T09:52:49Z

Thanks for the feedback @jodarove. There is a lot going on in the core engine these days, it will be better to go with an incremental approach to ease the review process and avoid merge conflicts. I will try to break this change into smaller chunks. Here is the first PR: #2630

pmdartus · 2022-02-04T15:11:59Z

Superseded by #2677

pmdartus added 13 commits December 14, 2021 07:24

chore: move all the diffing APIs in hooks.ts

15183da

chore: remove duplicate patchChildren

1e8f7e9

chore: move all the hooks to hooks

7b96e6f

refactor: add type to each VNode

6f01f12

chore: rework diffing modules

273b43f

chore: remove duplicate patching code

0f8b3ae

refactor: remove hooks

02053e5

refactor: move hydration in a different file

567ce7d

chore: fix comment diffing

d923dfd

chore: simpify stylesheet injection

fa79c48

chore: inline createVMHook

c0f066d

chore: remove need to lookup vm

878b6e4

chore: move the vnode types under the main directory

0b9e7f8

pmdartus added the nomerge label Dec 16, 2021

test: fix snapshot tests

372fb61

nolanlawson reviewed Dec 16, 2021

View reviewed changes

pmdartus and others added 3 commits December 19, 2021 18:43

chore: fix slot patching

59bc913

fix: portals in native shadow

68ff7df

fix: disconnectedCallback hydration test

c728a6b

jodarove reviewed Jan 7, 2022

View reviewed changes

jodarove suggested changes Jan 10, 2022

View reviewed changes

jodarove reviewed Jan 11, 2022

View reviewed changes

pmdartus mentioned this pull request Jan 12, 2022

refactor(engine-core): Simplify props and attrs patching #2630

Merged

pmdartus closed this Feb 4, 2022

ravijayaramappa deleted the pmdartus/renderer-refactor branch July 22, 2022 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine-core): POC splitting up diffing from rehydration #2608

refactor(engine-core): POC splitting up diffing from rehydration #2608

pmdartus commented Dec 16, 2021

nolanlawson Dec 16, 2021

nolanlawson commented Dec 16, 2021 •

edited

Loading

jodarove Jan 7, 2022

jodarove left a comment •

edited

Loading

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove Jan 10, 2022

jodarove left a comment

jodarove Jan 11, 2022

nolanlawson commented Jan 11, 2022

pmdartus commented Jan 12, 2022

pmdartus commented Feb 4, 2022

		function processElement(vnode: VElement, node: Node) {
		const elm = node as Element;

	processElement(vnode, node);
	processElement(vnode, node as Element);

		function processCustomElement(vnode: VCustomElement, node: Node) {
		const elm = node as Element;

	processCustomElement(vnode, node);
	processCustomElement(vnode, node as Element);

refactor(engine-core): POC splitting up diffing from rehydration #2608

refactor(engine-core): POC splitting up diffing from rehydration #2608

Conversation

pmdartus commented Dec 16, 2021

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

Choose a reason for hiding this comment

nolanlawson commented Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

jodarove left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodarove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson commented Jan 11, 2022

pmdartus commented Jan 12, 2022

pmdartus commented Feb 4, 2022

nolanlawson commented Dec 16, 2021 •

edited

Loading

jodarove left a comment •

edited

Loading