perf: hoist static objects/arrays in templates

[nolanlawson]

Details

Partially fixes #2559.

This adds an optimization to the template compiler where it identifies fully-static arrays and objects and hoists them into const variables outside of the tmpl function.

E.g.:

+const stc0 = { static: true };
+const stc1 = { really: { static: ['yep', 'totally', 'static' ] } };
 function tmpl() {
   return {
     foo: dynamic(),
-    bar: { static: true },
-    baz: { really: { static: ['yep', 'totally', 'static' ] } }
+    bar: stc0,
+    baz: stc1
   };
 }

This improves our "geometric mean" score on the js-framework-benchmark from 1.61 to 1.55. The main improvement is in "select row" which improves from 53.1 ±1.7 to 48.0 ±2.1.

This also improves many of our benchmarks, although two are modestly regressed.

I should probably investigate to see why these two are regressed, but the regression is small in absolute terms (0.85ms on one benchmark and 4.56ms on the other), so I'm not too concerned.

This PR is deliberately focused and conservative to avoid a risky change. In the future, we can do additional optimizations, e.g. to extract the VNodes themselves when they are inside of an iteration loop.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.

The format of compiled template code has changed. I.e., this is a template compiler change.

GUS work item

W-10113376

[nolanlawson]

If you're reading this in the GitHub UI, you can skip to here because everything above is just snapshot changes.

[pmdartus]

I am extremely surprised that some benchmarks regressed with this change. Is the regression consistent across multiple runs?

[nolanlawson]

I am extremely surprised that some benchmarks regressed with this change. Is the regression consistent across multiple runs?

Yes, I actually did two runs to verify the results. I also bumped the sample size to min 300 and set the horizon at 1% to really be sure. My hunch is that the update-slotted-content test is so small that it's exposing a one-time setup cost that is not amortized across many iterations (e.g. in the 1k or 10k tests).

[nolanlawson]

/nucleus test

[nolanlawson]

/nucleus test

[nolanlawson]

This actually broke a test in LWR (outlet.spec.ts) because an error callback is being fired twice. The root issue is that these two vnodes being compared have the exact same object for vnode.data, which is not the case in master. I assume somewhere in our vdom code we are mutating the data object, which is safe when we're dealing with copies but not when the objects may be the same.

This PR may need some work to avoid downstream breakages.

[nolanlawson]

The error only happens with dynamic components. I should write a test for this, but here is the fix to see if it unbreaks Nucleus ^

[nolanlawson]

OK, so when I bump the slot-update-slotted-content benchmark from 500 rows to 10k, there is no difference between master and this PR. So I think it is indeed just measuring a one-time setup cost.

[nolanlawson]

Re-ran the benchmarks with the new array fix, and the results are almost exactly the same as before. js-framework-benchmark has also not changed since the compiled template is the same.

[nolanlawson]

I did not add any new tests, because it turns out I can just use readonly in TypeScript to avoid calling setters on VNodeData/VElementData.

[pmdartus]

The LWC compiler has been emitting unique keys to each VNode to speed up DOM diffing. The main downside with this is that it produces a lot of garbage in the generated template code.

I think there is certainly a better way to handle this without adding more junk to the generated code.

[nolanlawson]

Ah, I was wondering what the key is for. If it's only to speed up DOM diffing, then what about removing it and relying on object equality? (Since objects are effectively immutable.) Or is the optimization still necessary?

[pmdartus]

Keys are used to checking if vnodes are equal. it is primarily used whenever nodes are dynamic (if block, each block). In those cases, we can't use object equality as those VNodes can't be hoisted.

[caridy]

keys are to denotate placeables for elements in the template, that's the way I think about them, and we use that information in multiple places to determine whether or not a vnode is the result of a particular placeable or another.

[caridy]

another alternative here is to not use ... due to the perf implications, and instead just use obj inheritance via Object.create(data, { key: { value: dc:${idx}:${data.key} } }). Considering that we never use hasOwnProperty with such object, we should be fine with this as well.

[caridy]

again, this is not a critical path since dynamic components are almost never used, just saying! :)

[nolanlawson]

Hmm... I would personally prefer to keep ... because it's idiomatic. Also, I'm guessing the perf implications are only from Locker, and we disallow lwc:dynamic on the platform, so I'm assuming that this line of code will never run with Locker enabled?

[pmdartus]

When reaching this code path, there is no way the vnode.data is hoisted as the innerHTML prop requires an invocation to api_sanitize_html_content. It is safe to mutate it in place by case it to any.

By the way, this block of code looks a little misplaced.

[nolanlawson]

I would kind of prefer to leave it as a clone. It seems unsafe for a developer to rely on quirks of the optimization algorithm. In the future, we may very well want to hoist:

api_sanitize_html_content("static string")

In fact, we are already caching the output on the $ctx anyway:

lwc/packages/@lwc/template-compiler/src/__tests__/fixtures/directive-inner-html/valid/expected.js

Lines 8 to 14 in 52e0a2b

	props: {
	innerHTML:
	$ctx._sanitizedHtml$0 ||
	($ctx._sanitizedHtml$0 = api_sanitize_html_content(
	"Hello <b>world</b>!"
	)),
	},

So if we apply the hosting optimization, arguably we may not need the $ctx optimization anymore.

[caridy]

uff, uff! elm.innerHTML === props.innerHTML broke my heart! I will have never approve that change. The whole point of the diffing algo is to avoid touching the DOM at all cost to compare things... reading the innerHTML from the element to compare it with the vnode is just wrong. We should fix that.

Additionally, the innerHTML should NOT be a regular property, it should have been a snabbdom module who's job is to compare two vnodes to determine whether or not the innerHTML needs to be set, rather than tricking the props module to not see the innerHTML for some cases when we know that it is the same. This is obviously not related to this PR, but needs to be fixed at some point. I might be missing something obvious here.

[nolanlawson]

@caridy Opened an issue to track this: #2603

[nolanlawson]

Due to VNodes being read-only now, I had to remove this line. AFAICT, it is not needed. I don't understand why we would need to set this to undefined in the oldCh array. The old children should just be garbage-collected, as they either belong to the old VNode:

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

Line 179 in c26277b

fn(vnode.elm!, oldVnode.children, children);

Or are replaced on the current VM:

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

Lines 418 to 419 in c26277b

	const children = renderComponent(vm);
	patchShadowRoot(vm, children);

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

Line 444 in c26277b

vm.children = newCh;

[pmdartus]

I don't yet fully understand the implications of removing this line here. The fact that it breaks no test is good news to me.

[caridy]

You CANNOT remove this line! :)

Basically, this line actuates when there is a new element who's key is found on the old vnodes, there are 2 possibilities:

1. the old node is in a position prior to the new position.
2. the old node is in a position after the new position.

The posibility of being in the same position is not possible in this branch of the code.

On top of that, there is another problem: "whether or not the tagName (sel) matches.

So, the problem is that if the sel matches, and the position is prior the new position (remember that this algo goes backward), since we are in a loop, there is a possibility that later in the game you encounter another element that must be inserted in the position marked by the old position, and the diffing algo tries to remove the element associated to the old vnodes.

My hunch is that this will result on the moved node to disappear from the DOM, and it is very hard to write a test that validate this assumption, but we can certainly try.

[nolanlawson]

@caridy Thanks for the explanation. I'm looking into it, and it turns out that this else block is never hit in either our Jest tests or Karma tests:

lwc/packages/@lwc/engine-core/src/3rdparty/snabbdom/snabbdom.ts

Lines 127 to 146 in d93e296

	idxInOld = oldKeyToIdx[newStartVnode.key!];
	if (isUndef(idxInOld)) {
	// New element
	newStartVnode.hook.create(newStartVnode);
	newStartVnode.hook.insert(newStartVnode, parentElm, oldStartVnode.elm!);
	newStartVnode = newCh[++newStartIdx];
	} else {
	elmToMove = oldCh[idxInOld];
	if (isVNode(elmToMove)) {
	if (elmToMove.sel !== newStartVnode.sel) {
	// New element
	newStartVnode.hook.create(newStartVnode);
	newStartVnode.hook.insert(newStartVnode, parentElm, oldStartVnode.elm!);
	} else {
	patchVnode(elmToMove, newStartVnode);
	oldCh[idxInOld] = undefined as any;
	newStartVnode.hook.move(elmToMove, parentElm, oldStartVnode.elm!);
	}
	}
	newStartVnode = newCh[++newStartIdx];

In other words, isUndef(idxInOld) is always true. I'll try to write a test where it is false.

[nolanlawson]

OK, I managed to find a Karma test to hit this line of code (I'll open a separate PR to improve our test coverage). Based on the usage patterns, I think we can do this:

1. Instead of using oldCh directly, use a clone ([...oldCh]) inside of updateDynamicChildren. AFAICT, oldCh is only used inside of that function – afterwards, it is discard and GC'ed.
2. To avoid the perf hit of cloning arrays, we can do the clone only on line 142 (i.e. when calling oldCh[idxInOld] = undefined as any; ).

@caridy How does this sound?

[caridy]

I'm not sure the clone solves the problem, unless you always rely on the cloned array for the different paths of the algo.

[caridy]

also closing via [...oldCh] will trigger the iterable protocol that is not ideal.

[nolanlawson]

@caridy I'm saying we should do:

- oldCh[idxInOld] = undefined as any;
+ const oldChClone = [...oldCh];
+ oldChClone[idxInOld] = undefined as any;
+ oldCh = oldChClone;

It solves the problem because the oldCh array is Readonly – although only because of the empty array [], which may be shared between VNodes due to the optimization in this PR. (A non-empty array cannot be hoisted because it will contain VNodes, not static objects. This is why we only need a shallow clone, not a deep clone.)

also closing via [...oldCh] will trigger the iterable protocol that is not ideal.

Can you elaborate? Are you concerned about performance? It seems to me that this code path is very infrequent, so I'm not really concerned about the perf implications there.

[caridy]

Ok, I'm fine with this approach. In locker code, we try to stay away from [...array] due to the perf implications of the iterables. cc @jdalton, but as you said, this is a very rare code path anyways.

[nolanlawson]

Re-ran against the updated benchmarks (#2596) and everything is green now!

[pmdartus]

I am conditionally approving this PR until we figure out what the oldCh[idxInOld] = undefined as any; used to do and explain why this isn't needed anymore.

[nolanlawson]

@pmdartus Sounds good, let me do that research. I'm encouraged by the fact that none of our downstreams seem to be failing, but I'm also nervous that removing that line of code seems to have no impact.