perf: apply static parts optimization to dynamic attributes - part 2 #4056

jmsjtu · 2024-03-13T19:14:42Z

Details

This is part 2 of 2 to apply static parts optimization to dynamic attributes as part of #3624.

This second PR handles the actual changes to our static content optimization to allow attributes with expressions.

The commits in the PR are broken down to more easily review:

e98ebdb contains the code changes to enable the optimization
f09d7d2 contains the template compiler test fixture updates
1b3b461 contains the @lwc/engine-server test fixture changes
5ec09d0 contains the karma tests
f1257b0 contains the hydration tests

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.

There is an observable change in SSR rendering for MATHML elements that use an attribute as an expression.

When the element contains an attribute expression and has no children it will now render with a self-closing tag. This is the correct behavior and it looks like the previous behavior did not capture it.

EX:

<mo form={expression}></mo>

In SSR this will now serialize as:

<mo form="expression value" />

Previously it would render without the closing tag:

<mo form="expression value"></mo>

GUS work item

W-15174789

…ntent-optimization-attributes

packages/@lwc/template-compiler/src/codegen/helpers.ts

packages/@lwc/template-compiler/src/codegen/codegen.ts

jmsjtu · 2024-03-13T19:17:49Z

packages/@lwc/template-compiler/src/codegen/static-element-serializer.ts

-    return (attrs.length > 0 ? ' ' : '') + attrs.join(' ') + (hasClassAttr ? '${2}' : '${3}');
+    return attrs.join('') + (hasClassAttr ? '${2}' : '${3}');


I modified the spacing here so that it's easier to join at runtime, the main issue before was removing the leading space when running on a browser (serialization not required).

jmsjtu · 2024-03-13T19:18:29Z

packages/@lwc/template-compiler/src/codegen/static-element-serializer.ts

-            attrs.push(`${name}="${htmlEscape(v, true)}"`);
+            // Inject a placeholder where the staticPartId will go when an expression occurs.
+            // This is only needed for SSR to inject the expression value during serialization.
+            attrs.push(hasExpression ? `\${"${v}"}` : ` ${name}="${htmlEscape(v, true)}"`);


When an expression is encountered, a token is inserted in its place, see the template compiler test fixtures for details.

jmsjtu · 2024-03-13T19:19:55Z

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

+    oldnode: VBaseElement | VStaticPart | null,
+    node: VBaseElement | VStaticPart,


In the browser, attribute patching for static parts is the same as with other elements.

jmsjtu · 2024-03-13T19:29:54Z

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

+    return (partToken: string) => {
+        // The partTokens are split into 3 section:
+        // 1. The first character represents the expression type (attribute, class, style, or text).
+        // 2. The characters from index 1 to the first occurrence of a ':' is the partId.
+        // 3. Everything after the first ':' represents the attribute name.
+        // For example: a0:data-name, a = an attribute, 0 = partId, data-name = attribute name.
+        const type = partToken.charAt(0);
+        const delimiterIndex = partToken.indexOf(':');
+        const partId = partToken.substring(1, delimiterIndex);
+        const rest = partToken.substring(delimiterIndex + 1);
+        const part = partIdsToParts.get(partId) ?? EmptyObject;


The format of the tokens will be {type}{partId}:{attributeName}, for example a0:data-name.

The token serves as an index to determine which static part value to use for serialization.

The reason this is required is that our current SSR implementation is not compatible with how we are applying attributes in the browser.

The main issue is the vnode.fragment used in mountStatic has already been assembled by this routine during the mount process.

In the browser, we rely on cloneNode to create the DOM elements, loop through those elements and apply the attributes to them.

However, in SSR, the cloneNode function just returns the string value in vnode.fragment. This is why we need to insert the expression values at the time the string is being built.

I thought about replicating the cloneNode behavior to reproduce the nodes in SSR but it would require an HTML parser to generate the nodes.

I feel that this is the most straight forward approach, although I think we should refactor this function to have different implementations between @lwc/engine-dom and @lwc/engine-server.

Great description! We should capture this in a high-level issue and add a TODO here. I think it would be great to have one place where we document this (very big) difference between engine-server and engine-dom.

It may ultimately get resolved by the dedicated SSR compiler.

Add a issue! #4078

jmsjtu · 2024-03-13T19:31:52Z

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

+
+    if (!isUndefined(parts)) {
+        // Elements must first be set on the static part to validate against.
+        traverseAndSetElements(elm, parts, renderer);


This step is needed before the validation of the static parts can occur since we compare vnodes against the actual element.

We could alternatively validate while we're traversing down the DOM nodes but I thought this was easier.

jmsjtu · 2024-03-13T19:33:58Z

packages/@lwc/template-compiler/src/codegen/codegen.ts

+                for (const attribute of node.attributes) {
+                    const { name, value } = attribute;
+                    if (isExpression(value)) {
+                        this.staticExpressionMap.set(attribute, `a${partId}:${name}`);


Plan to stash all the tokens at this point and they will all follow a similar format:

{type}{partId}:{rest}

a -> attribute
c -> class
s -> style
t -> text

Only attributes will have a value following the : since we need to identify which attribute to map it to.

jmsjtu · 2024-03-13T19:35:39Z

...ate-compiler/src/__tests__/fixtures/attributes/attribute-href-with-id-expression/expected.js

@@ -1,25 +1,25 @@
-import { registerTemplate } from "lwc";
+import { parseFragment, registerTemplate } from "lwc";
+const $fragment1 = parseFragment`<a${"a0:href"}${3}>KIX</a>`;


This is an example of how the tokens will appear.

…ce/lwc into jtu/static-content-optimization-attributes

jmsjtu · 2024-03-13T23:50:12Z

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


 const ColonCharCode = 58;

 export function patchAttributes(
-    oldVnode: VBaseElement | null,
-    vnode: VBaseElement,
+    oldnode: VBaseElement | VStaticPart | null,


I changed the variable names to node and oldNode because VStaticPart technically isn't a vnode as we define it.

Open to suggestions though!

oldnode: VBaseElement | VStaticPart | null,

I changed the variable names to node and oldNode

🤨 🤭

nolanlawson

First pass review, will do more reviewing later!

nolanlawson · 2024-03-14T18:15:24Z

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

+        }
+    }
+    return true;
+}


I think an important invariant here is:

It's never the case that parts is undefined on the server but defined on the client (or vice-versa)

It's never the case that parts has one length on the server but another on the client

This might be worth a comment.

Added a comment!

nolanlawson · 2024-03-14T18:17:12Z

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

+        // Event listeners only need to be applied once when mounting
+        applyEventListeners(part, renderer);
+        // Refs must be updated after every render due to refVNodes getting reset before every render
+        applyRefs(part, owner);


Why don't we handle attributes here? Because, if there is no hydration mismatch, then we assume that SSR gave us the correct attributes?

Yup, exactly. If there are no hydration mismatches then there shouldn't be a need to patch attributes (because they will be the same).

I'll add a comment explaining it here.

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

nolanlawson · 2024-03-14T18:27:56Z

packages/@lwc/template-compiler/src/codegen/static-element-serializer.ts

+    element: Element,
+    preserveComments: boolean,
+    codeGen: CodeGen
+): string {


If we're passing in the whole codeGen then there's no need to pass in preserveComments as well. You can get it from the codeGen.

packages/@lwc/template-compiler/src/codegen/codegen.ts

packages/@lwc/template-compiler/src/codegen/helpers.ts

packages/@lwc/template-compiler/src/codegen/static-element-serializer.ts

wjhsf · 2024-03-14T19:35:14Z

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


 const ColonCharCode = 58;

 export function patchAttributes(
-    oldVnode: VBaseElement | null,
-    vnode: VBaseElement,
+    oldnode: VBaseElement | VStaticPart | null,


oldnode: VBaseElement | VStaticPart | null,

I changed the variable names to node and oldNode

🤨 🤭

nolanlawson · 2024-03-14T20:13:30Z

packages/@lwc/integration-karma/test/static-content/index.spec.js

+        expect(nodes['deep3nested'].getAttribute('data-dynamic')).toEqual('3');
+        expect(nodes['deep4'].getAttribute('data-dynamic')).toEqual('4');
+        expect(nodes['deep4nested'].getAttribute('data-dynamic')).toEqual('4');
+    });


It might be interesting to add a test with a variation on this template where everything is in one <div>. That way, they are all separate parts of a single static vnode.

I'll make a note and include with the style and class attributes.

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

nolanlawson

This LGTM. It's a bit disappointing that we have to add so much complexity to handle @lwc/engine-server, but 1) that should hopefully go away thanks to the SSR compiler, and 2) it's not adding much overhead, thanks to tree-shaking and the minimal "a0:data-foo" string system you came up with. (A throwaway string is lower overhead than, for instance, a throwaway array.)

I think we should add some additional fixture tests to the template compiler to ensure that the indexes for this string format (a0, a1, etc.) work correctly with code comments and preserve-comments on vs off. This is something that's bit us in the past: #3933.

I also think this is a good opportunity to do a manual Nucleus before/after comparison before merging, just as a sanity check.

Nice work!

…ce/lwc into jtu/static-content-optimization-attributes

packages/@lwc/template-compiler/src/shared/types.ts

…ntent-optimization-attributes

jmsjtu added 6 commits March 13, 2024 11:49

perf: enable static content optimization for attribute expressions

e98ebdb

test: update template compiler tests

f09d7d2

test: update @lwc/engine-server tests

1b3b461

test: add karma tests

5ec09d0

test: add hydration tests

f1257b0

Merge branch 'master' of github.com:salesforce/lwc into jtu/static-co…

69f52ea

…ntent-optimization-attributes

jmsjtu requested a review from a team as a code owner March 13, 2024 19:14

jmsjtu commented Mar 13, 2024

View reviewed changes

packages/@lwc/template-compiler/src/codegen/helpers.ts Show resolved Hide resolved

jmsjtu commented Mar 13, 2024

View reviewed changes

packages/@lwc/template-compiler/src/codegen/codegen.ts Show resolved Hide resolved

jmsjtu commented Mar 13, 2024

View reviewed changes

jmsjtu added 3 commits March 13, 2024 12:42

Merge branch 'jtu/static-content-optimization' of github.com:salesfor…

2416be4

…ce/lwc into jtu/static-content-optimization-attributes

Merge branch 'jtu/static-content-optimization' of github.com:salesfor…

8e3c9d1

…ce/lwc into jtu/static-content-optimization-attributes

chore: fix merge conflicts

a0b7650

jmsjtu requested review from ekashida, nolanlawson and divmain March 13, 2024 20:03

jmsjtu mentioned this pull request Mar 13, 2024

perf: apply static parts optimization to dynamic attributes #4055

Merged

jmsjtu commented Mar 13, 2024

View reviewed changes

nolanlawson reviewed Mar 14, 2024

View reviewed changes

wjhsf reviewed Mar 14, 2024

View reviewed changes

nolanlawson reviewed Mar 14, 2024

View reviewed changes

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

nolanlawson approved these changes Mar 14, 2024

View reviewed changes

jmsjtu mentioned this pull request Mar 15, 2024

perf: apply static parts optimization to dynamic class attributes #4071

Merged

jmsjtu and others added 5 commits March 18, 2024 11:36

Merge branch 'jtu/static-content-optimization' of github.com:salesfor…

7d43caa

…ce/lwc into jtu/static-content-optimization-attributes

Merge branch 'jtu/static-content-optimization' of github.com:salesfor…

a9f2113

…ce/lwc into jtu/static-content-optimization-attributes

chore: release v6.3.4 (#4074)

13a7fa1

refactor: update based on PR feedback

93f51d8

test: add unit tests

95fd4ae

jmsjtu commented Mar 19, 2024

View reviewed changes

packages/@lwc/template-compiler/src/shared/types.ts Outdated Show resolved Hide resolved

jmsjtu added 4 commits March 19, 2024 10:19

refactor: use a map again

6462930

chore: revert variable naming

e21d85a

Merge branch 'master' of github.com:salesforce/lwc into jtu/static-co…

a89836c

…ntent-optimization-attributes

chore: properly warn engine-server

bf73b09

jmsjtu mentioned this pull request Mar 19, 2024

Discrepancy between @lwc/engine-dom and @lwc/engine-server static parts application #4078

Open

chore: add todos

f4caffc

jmsjtu merged commit b198989 into jtu/static-content-optimization Mar 19, 2024
3 checks passed

jmsjtu deleted the jtu/static-content-optimization-attributes branch March 19, 2024 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: apply static parts optimization to dynamic attributes - part 2 #4056

perf: apply static parts optimization to dynamic attributes - part 2 #4056

jmsjtu commented Mar 13, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024 •

edited

nolanlawson Mar 14, 2024

jmsjtu Mar 19, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024

jmsjtu Mar 13, 2024

wjhsf Mar 14, 2024

nolanlawson left a comment

nolanlawson Mar 14, 2024

jmsjtu Mar 19, 2024

nolanlawson Mar 14, 2024

jmsjtu Mar 18, 2024

nolanlawson Mar 14, 2024

wjhsf Mar 14, 2024

nolanlawson Mar 14, 2024

jmsjtu Mar 19, 2024

nolanlawson left a comment

		return (attrs.length > 0 ? ' ' : '') + attrs.join(' ') + (hasClassAttr ? '${2}' : '${3}');
		return attrs.join('') + (hasClassAttr ? '${2}' : '${3}');

		oldnode: VBaseElement \| VStaticPart \| null,
		node: VBaseElement \| VStaticPart,

perf: apply static parts optimization to dynamic attributes - part 2 #4056

perf: apply static parts optimization to dynamic attributes - part 2 #4056

Conversation

jmsjtu commented Mar 13, 2024

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsjtu Mar 13, 2024 • edited

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

nolanlawson left a 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

nolanlawson left a comment

Choose a reason for hiding this comment

jmsjtu Mar 13, 2024 •

edited