refactor(engine-core): Remove patching hooks #2677

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

Details

This is a follow-up to #2648. This PR finishes the transition from hooks to VNodes with a type property.

Performance test results

Benchmark	Before (low)	Before (high)	Before (avg)	After (low)	After (high)	After (avg)	Delta (low)	Delta (high)	Delta (avg)	Delta perc (low)	Delta perc (high)	Delta perc (avg)
dom-light-styled-component-create-10k-same	143.02	144.89	143.95	142.88	145.00	143.94	-1.43	1.40	-0.01	-0.01	0.01	-0.00
dom-light-styled-component-create-1k-different	158.83	160.78	159.80	158.93	160.87	159.90	-1.28	1.48	0.10	-0.01	0.01	0.00
dom-ss-native-styled-component-create-10k-same	212.97	215.12	214.05	212.36	214.70	213.53	-2.10	1.07	-0.52	-0.01	0.00	-0.00
dom-ss-native-styled-component-create-1k-different	157.28	159.44	158.36	157.33	159.45	158.39	-1.49	1.55	0.03	-0.01	0.01	0.00
dom-ss-slot-create-container-5k	492.97	497.88	495.42	483.23	489.29	486.26	-13.06	-5.26	-9.16	-0.03	-0.01	-0.02
dom-ss-slot-update-slotted-content-5k	322.77	325.23	324.00	320.97	323.70	322.34	-3.50	0.17	-1.67	-0.01	0.00	-0.01
dom-ss-styled-component-create-10k-same	232.78	235.51	234.15	232.41	235.24	233.83	-2.29	1.65	-0.32	-0.01	0.01	-0.00
dom-ss-styled-component-create-1k-different	177.48	179.75	178.62	177.48	179.67	178.57	-1.62	1.53	-0.04	-0.01	0.01	-0.00
dom-styled-component-create-10k-same	208.71	209.85	209.28	209.93	211.12	210.53	0.42	2.07	1.25	0.00	0.01	0.01
dom-styled-component-create-1k-different	161.91	163.46	162.68	162.49	163.97	163.23	-0.53	1.62	0.54	-0.00	0.01	0.00
dom-table-create-10k	160.62	162.50	161.56	160.39	162.55	161.47	-1.52	1.35	-0.09	-0.01	0.01	-0.00
dom-table-hydrate-1k	39.05	39.58	39.32	39.09	39.57	39.33	-0.35	0.37	0.01	-0.01	0.01	0.00
dom-tablecmp-append-1k	45.41	45.78	45.60	45.69	46.06	45.87	0.01	0.53	0.27	0.00	0.01	0.01
dom-tablecmp-create-10k	393.07	396.61	394.84	392.46	396.40	394.43	-3.06	2.23	-0.42	-0.01	0.01	-0.00
dom-tablecmp-create-1k	62.81	63.53	63.17	62.75	63.57	63.16	-0.56	0.54	-0.01	-0.01	0.01	-0.00
dom-tablecmp-hydrate-1k	41.64	42.59	42.12	39.90	40.84	40.37	-2.42	-1.08	-1.75	-0.06	-0.03	-0.04
dom-trackedcmp-create-10k	353.07	356.08	354.57	354.20	357.37	355.78	-0.98	3.39	1.21	-0.00	0.01	0.00
server-table-render-10k	208.17	208.85	208.51	209.88	210.50	210.19	1.22	2.14	1.68	0.01	0.01	0.01
server-tablecmp-render-10k	211.24	211.85	211.54	212.93	213.60	213.27	1.27	2.17	1.72	0.01	0.01	0.01

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 · 2022-02-07T20:55:24Z

This is awesome. I'm a bit surprised that the perf results show some scenarios getting slightly slower.

(Same data, but pasted into a spreadsheet to visualize.)

If you used the default Tachometer settings, it may be worthwhile to bump it up to a sample size of 200 and timeout of 15 minutes or something. Although usually the results are repeatable (even if they're surprising).

I'm still +1 on this PR since regressing server rendering in favor of DOM rendering is a worthwhile tradeoff to me.

jodarove

I think this can be improved.

jodarove · 2022-02-09T16:09:56Z

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

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


@pmdartus I wouldn't say I like that this function is doing two different things based on the parameters. If n1 is null, it is mounting (create+insert right?), otherwise patching. Then in the process*, we make the same differentiation with a condition for isNull (mount/patch).

imho, (because the diffing algo is already saying whether to mount/patch) a better approach will be to separate the current patch logic into patch and mount and the process* into patch* and mount* (you already have them for Element and CustomeElements)? (imo) Such a way is more explicit, where you have patch(null,...) will become mount(n2,...) and we can skip the multiple isNull checks on the critical path of the diffing algo.

@jodarove I went ahead and implemented this: 0094a68

I'll re-run the perf tests, because maybe this actually has a perf impact? Checking isNull() over and over does seem like it may have a small cost.

caridy

I'm supportive of this change!

…from-hooks

nolanlawson · 2022-02-16T00:07:29Z

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

+        unmount(n1, parent, true);
+        mount(n2, parent, nextSibling(n1.elm));
+        return;
+    }


None of our Jest/Karma tests hit this condition, but I'm not sure if it's impossible to hit...

I think it's impossible to hit this condition. Here's why:

Call sites that already check isSameVnode()

Most places that call patch() already check for isSameVnode(), e.g.:

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

Lines 604 to 605 in 0094a68

} else if (isSameVnode(oldEndVnode, newEndVnode)) {

patch(oldEndVnode, newEndVnode, parent);

Call site that verifies key and sel

As for the call sites that don't check, there are two. First, there's this one:

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

Line 636 in 0094a68

patch(elmToMove, newStartVnode, parent);

But this one has already guaranteed that newStartVnode and elmToMove have the same key:

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

Line 624 in 0094a68

idxInOld = oldKeyToIdx[newStartVnode.key!];

... and the same sel:

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

Lines 632 to 635 in 0094a68

if (elmToMove.sel !== newStartVnode.sel) {

// New element

mount(newStartVnode, parent, oldStartVnode.elm!);

} else {

... which is the definition of isSameVnode:

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

Lines 89 to 91 in 0094a68

export function isSameVnode(vnode1: VNode, vnode2: VNode): boolean {

return vnode1.key === vnode2.key && vnode1.sel === vnode2.sel;

}

Call site that relies on static children ordering

The other call site is in updateStaticChildren:

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

Line 701 in 0094a68

patch(n1, n2, parent);

... in which case, we are dealing with two arrays of children that are already guaranteed to be the same (static), and we are taking the same item at the same index for both:

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

Lines 693 to 695 in 0094a68

for (let i = c2Length - 1; i >= 0; i -= 1) {

const n1 = c1[i];

const n2 = c2[i];

So I think this code is safe to remove!

It turns out that the last scenario (updateStaticChildren) can hit this code path, if the key is defined outside of an iteration: #2697.

We can add code to handle this particular scenario.

Good catch! I added a bunch of thoughts about this issue on the issue: #2697 (comment)

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

nolanlawson · 2022-02-16T16:48:36Z

Re-ran the benchmarks, it's looking good:

The 1% regression in dom-table-create-10k is odd; may be worth looking into.

nolanlawson · 2022-02-16T21:16:46Z

Well, thanks to ui-b2b-components tests, we now know that this condition is possible to hit. Looking into it.

nolanlawson · 2022-02-16T23:58:17Z

OK, added a bunch of tests, and that should cover it.

pmdartus

I can't approve my own PR, but I think it's in a good enough state merged. We can tackle #2697 in a follow-up one. 👍

pmdartus · 2022-02-17T09:27:15Z

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

+        unmount(n1, parent, true);
+        mount(n2, parent, nextSibling(n1.elm));
+        return;
+    }


Good catch! I added a bunch of thoughts about this issue on the issue: #2697 (comment)

nolanlawson · 2022-02-17T19:41:24Z

Re-ran the benchmarks, got similar results. This LGTM.

pmdartus mentioned this pull request Feb 4, 2022

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

Closed

pmdartus added 5 commits February 5, 2022 05:47

refactor: first draft

055d382

refactor: simplify mount and unmount

c45ae7b

refactor: fix patch types

d7c57f5

refactor: fix patching algo

d740ccd

refactor: fix hydration

14d43ba

pmdartus force-pushed the pmdartus/move-away-from-hooks branch from 79cb3f7 to 14d43ba Compare February 5, 2022 04:49

pmdartus marked this pull request as ready for review February 7, 2022 17:10

nolanlawson approved these changes Feb 7, 2022

View reviewed changes

jodarove reviewed Feb 9, 2022

View reviewed changes

pmdartus mentioned this pull request Feb 11, 2022

[Perf] Hoist static VNodes in template compiler #2688

Closed

5 tasks

caridy reviewed Feb 15, 2022

View reviewed changes

nolanlawson added 2 commits February 15, 2022 15:46

Merge remote-tracking branch 'origin/master' into pmdartus/move-away-…

37b6166

…from-hooks

refactor: split into patch* and mount*

0094a68

nolanlawson reviewed Feb 16, 2022

View reviewed changes

fix: remove unused code block

f95d694

pmdartus commented Feb 16, 2022

View reviewed changes

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

nolanlawson added 2 commits February 16, 2022 08:52

fix: add dev-only test

73dbf78

fix: fix json.stringify

d610f82

nolanlawson mentioned this pull request Feb 16, 2022

"key" attribute can be used outside of iteration #2697

Closed

nolanlawson added 4 commits February 16, 2022 15:17

test: add test for key attribute outside of iteration

53eb14e

fix: fix key defined outside of iteration

df6c672

test: add more tests and a fix

b2d0129

test: add more tests

35fb87a

pmdartus commented Feb 17, 2022

View reviewed changes

jodarove approved these changes Feb 17, 2022

View reviewed changes

nolanlawson approved these changes Feb 17, 2022

View reviewed changes

nolanlawson merged commit 9e8a116 into master Feb 17, 2022

nolanlawson deleted the pmdartus/move-away-from-hooks branch February 17, 2022 19:42

nolanlawson mentioned this pull request Feb 17, 2022

fix(template-compiler): omit api_key for key outside iteration #2699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine-core): Remove patching hooks #2677

refactor(engine-core): Remove patching hooks #2677

pmdartus commented Feb 4, 2022 •

edited

Loading

nolanlawson commented Feb 7, 2022

jodarove left a comment

jodarove Feb 9, 2022 •

edited

Loading

nolanlawson Feb 16, 2022

caridy left a comment

nolanlawson Feb 16, 2022

nolanlawson Feb 16, 2022

nolanlawson Feb 16, 2022

pmdartus Feb 17, 2022

nolanlawson commented Feb 16, 2022

nolanlawson commented Feb 16, 2022

nolanlawson commented Feb 16, 2022

pmdartus left a comment

pmdartus Feb 17, 2022

nolanlawson commented Feb 17, 2022

	} else if (isSameVnode(oldEndVnode, newEndVnode)) {
	patch(oldEndVnode, newEndVnode, parent);

	if (elmToMove.sel !== newStartVnode.sel) {
	// New element
	mount(newStartVnode, parent, oldStartVnode.elm!);
	} else {

	export function isSameVnode(vnode1: VNode, vnode2: VNode): boolean {
	return vnode1.key === vnode2.key && vnode1.sel === vnode2.sel;
	}

	for (let i = c2Length - 1; i >= 0; i -= 1) {
	const n1 = c1[i];
	const n2 = c2[i];

refactor(engine-core): Remove patching hooks #2677

refactor(engine-core): Remove patching hooks #2677

Conversation

pmdartus commented Feb 4, 2022 • edited Loading

Details

Performance test results

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

nolanlawson commented Feb 7, 2022

jodarove left a comment

Choose a reason for hiding this comment

jodarove Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

nolanlawson Feb 16, 2022

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

nolanlawson Feb 16, 2022

Choose a reason for hiding this comment

nolanlawson Feb 16, 2022

Choose a reason for hiding this comment

Call sites that already check isSameVnode()

Call site that verifies key and sel

Call site that relies on static children ordering

nolanlawson Feb 16, 2022

Choose a reason for hiding this comment

pmdartus Feb 17, 2022

Choose a reason for hiding this comment

nolanlawson commented Feb 16, 2022

nolanlawson commented Feb 16, 2022

nolanlawson commented Feb 16, 2022

pmdartus left a comment

Choose a reason for hiding this comment

pmdartus Feb 17, 2022

Choose a reason for hiding this comment

nolanlawson commented Feb 17, 2022

pmdartus commented Feb 4, 2022 •

edited

Loading

jodarove Feb 9, 2022 •

edited

Loading

Call sites that already check `isSameVnode()`

Call site that verifies `key` and `sel`