perf: use constructable stylesheets #2460

nolanlawson · 2021-08-17T20:47:37Z

Details

After looking into the synthetic-to-native shadow migration, it's clear to me that we need a performant solution for including the same stylesheet in multiple components (e.g. @import 'shared.css'). And through manual testing, I saw that constructable stylesheets really are a significant perf improvement in Chrome.

This PR is a revised version of #2290 that aims to be less invasive to the codebase while still bringing perf improvements. I also added some new Tachometer benchmarks to verify the perf gain.

My goal is to improve native shadow performance without regressing synthetic shadow performance. To achieve this, we're only using constructable stylesheets in the per-component level, not at the global level. (See this doc for why that matters.)

Benchmark results

Benchmark	Browser	Before	After	Delta
dom-ss-styled-component-create-1k-different	chrome	334ms-337ms	335ms-338ms	-0.2% - 0.8% (~same)
dom-ss-styled-component-create-1k-different	firefox	296ms-298ms	297ms-299ms	-0.2% - 0.9% (~same)
dom-ss-styled-component-create-1k-different	safari	161ms-163ms	161ms-163ms	-0.8% - 0.9% (~same)
dom-ss-styled-component-create-1k-same	chrome	105ms-107ms	105ms-107ms	-0.6% - 1.0% (~same)
dom-ss-styled-component-create-1k-same	firefox	87ms-89ms	87ms-89ms	-1.0% - 0.9% (~same)
dom-ss-styled-component-create-1k-same	safari	54ms-55ms	54ms-54ms	-0.9% - 0.9% (~same)
dom-styled-component-create-1k-different	chrome	330ms-332ms	294ms-295ms	-11.5% - -10.6% (faster)
dom-styled-component-create-1k-different	firefox	182ms-187ms	173ms-177ms	-7.0% - -3.6% (faster)
dom-styled-component-create-1k-different	safari	150ms-158ms	146ms-149ms	-6.8% - -1.7% (faster)
dom-styled-component-create-1k-same	chrome	148ms-150ms	101ms-103ms	-32.0% - -30.9% (faster)
dom-styled-component-create-1k-same	firefox	99ms-103ms	83ms-87ms	-18.6% - -13.5% (faster)
dom-styled-component-create-1k-same	safari	63ms-64ms	55ms-56ms	-13.4% - -11.5% (faster)

(The ss- ones use synthetic shadow.) Basically, the native shadow ones are massively improved (especially in Chrome), but the synthetic shadow ones are the same.

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

There is a small change in how light DOM components work. Previously, if you switched templates on a light DOM component, then the old <style> would be removed from the DOM and replaced with the new <style>. Now the new <style> is merely added, while the old <style> remains. I don't think this will be a big problem in practice.

GUS work item

W-9125079

nolanlawson · 2021-08-17T20:49:23Z

packages/@lwc/engine-dom/src/renderer.ts

+const supportsConstructableStyleSheets = isFunction((CSSStyleSheet.prototype as any).replaceSync);
+const styleElements: { [content: string]: HTMLStyleElement } = create(null);
+const styleSheets: { [content: string]: CSSStyleSheet } = create(null);
+const nodesToStyleSheets = new WeakMap<Node, { [content: string]: true }>();


The styleElements and styleSheets are technically a memory leak, but I don't think it's a big deal in practice. With synthetic shadow, we already don't clean up <style> tags added to the <head>, so we're already leaking there.

I would also prefer not to maintain a mapping of string -> CSSStyleSheet at all, but that would require modifying the style-compiler, and I made a point of not doing that because I tried that in #2290 and it adds a lot of complexity.

nolanlawson · 2021-08-17T20:52:10Z

packages/integration-karma/test/light-dom/multiple-templates/index.spec.js

-            expect(getComputedStyle(element.querySelector('div')).color).toEqual('rgb(0, 0, 0)');
+            expect(getComputedStyle(element.querySelector('div')).color).toEqual(
+                'rgb(233, 150, 122)'
+            );


This is the thing that changed in light DOM. Basically the styles from template A remain even when template B is rendered.

I don't think this is a big deal, because it's already how synthetic shadow works – styles at the global <head> are never cleaned up. Many CSS-in-JS libraries also work this way.

I opened a separate issue for this; it's a general problem: #2466

nolanlawson · 2021-08-17T20:53:53Z

packages/integration-karma/test/shadow-dom/multiple-templates/index.spec.js

+
+if (process.env.NATIVE_SHADOW) {
+    describe('Shadow DOM styling - multiple shadow DOM components', () => {
+        it('Does not duplicate styles if template is re-rendered', () => {


This test confirms we don't end up with duplicate <style>s/CSSStyleSheets in the shadow root when we render template A, then B, then A again, etc.

Arguably we could just not care about this, and then the logic would be simpler because we wouldn't have to check for existing styles. I could go either way, but I kind of like that we don't endlessly add <style>s if (for whatever reason) the component is alternating between A and B repeatedly.

pmdartus · 2021-08-18T12:50:56Z

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

-        return null;
-    } else {
-        // native shadow or light DOM
+    } else if (renderer.ssr) {


Side note: It will be more complex to do rehydration if SSR produces a different HTML structure than client-side rendering. @jodarove @divmain

This is a good point, yes. And constructable styles for SSR have not been specced yet: WICG/webcomponents#939

I consider the tradeoff worth it given the massive perf improvement in our benchmark (~30% in dom-styled-component-create-1k-same). Plus in the real-world app I tested, it was such a big difference that it was visually noticeable (~4.3s versus ~2.1s).

Sounds reasonable to me.

pmdartus · 2021-08-18T12:54:49Z

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

+function getNearestNativeShadowComponent(vm: VM): VM | null {
+    let owner: VM | null = vm;
+    while (!isNull(owner)) {
+        if (owner.renderMode === RenderMode.Shadow && owner.shadowMode === ShadowMode.Native) {
+            return owner;
+        }
+        owner = owner.owner;
+    }
+    return owner;
+}


Would it be better performance-wise to store this on the VM instead of looking this up lazily?

This is worth testing, although we'd have to be careful about when to invalidate the cache (e.g. if the component is moved).

@pmdartus I tried it (nolanlawson@87345ee), but it doesn't seem to be a statistically-significant perf improvement, at least in these benchmarks. I also worry that it introduces footguns (e.g. if we don't invalidate the cache at the right time).

There might be some benchmark that can prove an improvement (e.g. rendering many stylesheets on deeply-nested components inside a native shadow root), but I don't know how realistic this is.

Benchmark results

⠋ Auto-sample 550 (timeout in 0m0s) ┌─────────────┬─────────────────┐ │ Browser │ chrome-headless │ │ │ 92.0.4515.159 │ ├─────────────┼─────────────────┤ │ Sample size │ 600 │ └─────────────┴─────────────────┘ ┌─────────────────────────────────────────────────┬─────────────┬────────────┬───────────────────┬────────────────────────────────────────────────────┬────────────────────────────────────────────────────┐ │ Benchmark │ Version │ Bytes │ Avg time │ vs dom-styled-component-create-1k-same-this-change │ vs dom-styled-component-create-1k-same-tip-of-tree │ │ │ │ │ │ │ tip-of-tree │ ├─────────────────────────────────────────────────┼─────────────┼────────────┼───────────────────┼────────────────────────────────────────────────────┼────────────────────────────────────────────────────┤ │ dom-styled-component-create-1k-same-this-change │ <none> │ 220.48 KiB │ 52.77ms - 53.43ms │ │ unsure │ │ │ │ │ │ - │ -0% - +1% │ │ │ │ │ │ │ -0.19ms - +0.77ms │ ├─────────────────────────────────────────────────┼─────────────┼────────────┼───────────────────┼────────────────────────────────────────────────────┼────────────────────────────────────────────────────┤ │ dom-styled-component-create-1k-same-tip-of-tree │ tip-of-tree │ 220.18 KiB │ 52.46ms - 53.16ms │ unsure │ │ │ │ │ │ │ -1% - +0% │ - │ │ │ │ │ │ -0.77ms - +0.19ms │ │ └─────────────────────────────────────────────────┴─────────────┴────────────┴───────────────────┴────────────────────────────────────────────────────┴────────────────────────────────────────────────────┘ NOTE Hit 5 minute auto-sample timeout trying to resolve horizon(s) ⠋ Auto-sample 330 (timeout in 0m44s) ┌─────────────┬─────────────────┐ │ Browser │ chrome-headless │ │ │ 92.0.4515.159 │ ├─────────────┼─────────────────┤ │ Sample size │ 380 │ └─────────────┴─────────────────┘ ┌──────────────────────────────────────────────────────┬─────────────┬────────────┬─────────────────────┬─────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────┐ │ Benchmark │ Version │ Bytes │ Avg time │ vs dom-styled-component-create-1k-different-this-change │ vs dom-styled-component-create-1k-different-tip-of-tree │ │ │ │ │ │ │ tip-of-tree │ ├──────────────────────────────────────────────────────┼─────────────┼────────────┼─────────────────────┼─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤ │ dom-styled-component-create-1k-different-this-change │ <none> │ 977.19 KiB │ 157.52ms - 159.63ms │ │ unsure │ │ │ │ │ │ - │ -1% - +1% │ │ │ │ │ │ │ -1.20ms - +1.57ms │ ├──────────────────────────────────────────────────────┼─────────────┼────────────┼─────────────────────┼─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤ │ dom-styled-component-create-1k-different-tip-of-tree │ tip-of-tree │ 976.89 KiB │ 157.50ms - 159.29ms │ unsure │ │ │ │ │ │ │ -1% - +1% │ - │ │ │ │ │ │ -1.57ms - +1.20ms │ │ └──────────────────────────────────────────────────────┴─────────────┴────────────┴─────────────────────┴─────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────┘

(Note tip-of-tree is the nolan/constructable-stylesheets-redux branch in this case.)

It might be worth adding this as a comment as it is certainly something that I would ask myself when looking at this code in the future.

pmdartus

Regarding the light DOM issue and template replacement, I think it will be even less of an issue once we land scoped styles.

packages/perf-benchmarks/src/utils/styledComponentBenchmark.js

packages/perf-benchmarks-components/scripts/generate-styled-components.js

pmdartus · 2021-08-20T14:54:15Z

packages/perf-benchmarks-components/rollup.config.js


-const rootDir = path.join(__dirname, 'src');
+const { tmpDir, styledComponents } = generateStyledComponents();


It is not ideal that we have to add a one-off for a specific test suite.

Agreed, but I figure in the future we may want to use similar functionality for other benchmarks. At that point, we can refactor it to generalize it.

pmdartus · 2021-08-23T14:46:34Z

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

+function getNearestNativeShadowComponent(vm: VM): VM | null {
+    let owner: VM | null = vm;
+    while (!isNull(owner)) {
+        if (owner.renderMode === RenderMode.Shadow && owner.shadowMode === ShadowMode.Native) {
+            return owner;
+        }
+        owner = owner.owner;
+    }
+    return owner;
+}


It might be worth adding this as a comment as it is certainly something that I would ask myself when looking at this code in the future.

pmdartus · 2021-08-23T14:54:34Z

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

-        return null;
-    } else {
-        // native shadow or light DOM
+    } else if (renderer.ssr) {


Sounds reasonable to me.

pmdartus · 2021-08-23T15:54:07Z

I am also adding @jodarove and @divmain to the review to make sure this change can align with rehydration.

…ed-components.js Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

nolanlawson · 2021-08-31T22:47:04Z

@jodarove @divmain Do you foresee any problems with this PR and rehydration? My assumption was:

SSR'd content would use regular <style>s (until declarative constructable stylesheets ship)
During hydration, the client will remove the <style> and replace it with a constructable stylesheet (or keep it, I dunno, depends on perf/FOUC implications).

jodarove · 2021-09-01T14:38:24Z

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

+        for (let i = 0; i < stylesheets.length; i++) {
+            if (isGlobal) {
+                renderer.insertGlobalStylesheet(stylesheets[i]);
+            } else {
+                // local level
+                renderer.insertStylesheet(stylesheets[i], root!.cmpRoot);
+            }
+        }


I wonder if the js engine optimizes this code, do you know @nolanlawson ?:

Suggested change

for (let i = 0; i < stylesheets.length; i++) {

if (isGlobal) {

renderer.insertGlobalStylesheet(stylesheets[i]);

} else {

// local level

renderer.insertStylesheet(stylesheets[i], root!.cmpRoot);

}

}

if (isGlobal) {

for (let i = 0; i < stylesheets.length; i++) {

renderer.insertGlobalStylesheet(stylesheets[i]);

}

} else {

for (let i = 0; i < stylesheets.length; i++) {

// local level

renderer.insertStylesheet(stylesheets[i], root!.cmpRoot);

}

}

@jodarove I admit I don't know. The isGlobal is a constant, so I would hope the engine optimizes it?

Unfortunately my benchmark wouldn't capture it, because it only has one stylesheet per template.

@jodarove The difference is pretty small (if there even is a difference – I seem to get different results in JSBench if I run multiple times). I'd say let's stick with the more readable version, especially given that most templates will have just one stylesheet.

nolanlawson requested review from ekashida and pmdartus August 17, 2021 20:47

nolanlawson commented Aug 17, 2021

View reviewed changes

nolanlawson force-pushed the nolan/constructable-stylesheets-redux branch from 29539ee to 2214d32 Compare August 17, 2021 20:55

nolanlawson requested a review from abdulsattar August 17, 2021 20:55

pmdartus reviewed Aug 18, 2021

View reviewed changes

nolanlawson mentioned this pull request Aug 20, 2021

Constructible Stylesheet Support #1346

Closed

nolanlawson requested a review from pmdartus August 20, 2021 18:54

pmdartus approved these changes Aug 23, 2021

View reviewed changes

pmdartus requested review from jodarove and divmain August 23, 2021 15:53

nolanlawson and others added 4 commits August 23, 2021 09:46

perf: use constructable stylesheets

1aa0bcd

fix: update packages/perf-benchmarks-components/scripts/generate-styl…

ac969ba

…ed-components.js Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

fix: respond to PR comments

98e1683

fix: fix formatting

4f1707f

nolanlawson force-pushed the nolan/constructable-stylesheets-redux branch from f7d2055 to 4f1707f Compare August 23, 2021 16:46

jodarove reviewed Sep 1, 2021

View reviewed changes

nolanlawson merged commit 1898df2 into master Sep 7, 2021

nolanlawson deleted the nolan/constructable-stylesheets-redux branch September 7, 2021 21:31

jodarove mentioned this pull request Sep 27, 2021

poc: ssr client rehydration #2442

Merged

jmrog mentioned this pull request Dec 16, 2021

Error when rendering the same stylesheet to two different documents (iframes, templates) #2609

Closed

nolanlawson mentioned this pull request May 10, 2022

fix(engine-dom): refactor stylesheet API for consistency #2827

Merged

nolanlawson mentioned this pull request Jul 6, 2022

Insert global stylesheets into document.adoptedStyleSheets #2922

Open

nolanlawson mentioned this pull request Apr 17, 2023

Declarative CSS Module Scripts WICG/webcomponents#939

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: use constructable stylesheets #2460

perf: use constructable stylesheets #2460

nolanlawson commented Aug 17, 2021 •

edited

Loading

nolanlawson Aug 17, 2021

nolanlawson Aug 17, 2021 •

edited

Loading

nolanlawson Aug 20, 2021

nolanlawson Aug 17, 2021

pmdartus Aug 18, 2021

nolanlawson Aug 18, 2021

pmdartus Aug 23, 2021

pmdartus Aug 18, 2021

nolanlawson Aug 18, 2021

nolanlawson Aug 20, 2021

pmdartus Aug 23, 2021

pmdartus left a comment

pmdartus Aug 20, 2021

nolanlawson Aug 23, 2021

pmdartus Aug 23, 2021

pmdartus Aug 23, 2021

pmdartus commented Aug 23, 2021

nolanlawson commented Aug 31, 2021

jodarove Sep 1, 2021 •

edited

Loading

nolanlawson Sep 1, 2021

nolanlawson Sep 1, 2021


		const rootDir = path.join(__dirname, 'src');
		const { tmpDir, styledComponents } = generateStyledComponents();

perf: use constructable stylesheets #2460

perf: use constructable stylesheets #2460

Conversation

nolanlawson commented Aug 17, 2021 • edited Loading

Details

Does this PR introduce breaking changes?

GUS work item

Choose a reason for hiding this comment

nolanlawson Aug 17, 2021 • 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

pmdartus 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

pmdartus commented Aug 23, 2021

nolanlawson commented Aug 31, 2021

jodarove Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson commented Aug 17, 2021 •

edited

Loading

nolanlawson Aug 17, 2021 •

edited

Loading

jodarove Sep 1, 2021 •

edited

Loading