perf(engine-dom): refactor style cache to reduce lookups #2832

nolanlawson · 2022-05-12T16:06:03Z

Details

I couldn't resist fixing some of the perf regressions introduced in #2827. This refactors the stylesheet cache to avoid making a lot of lookups, which provides some perf wins:

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-05-12T16:08:18Z

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

+    stylesheet: HTMLStyleElement;
+}
+
+const stylesheetCache: Map<String, CacheData> = new Map();


The main insight I had is that we only need one cache, keyed on the string contents of the stylesheet. So we only need to do one lookup per stylesheet.

If we insert 10k native shadow components with the same stylesheet, then rather than creating a Set<string> for each shadow root (i.e. WeakMap<ShadowRoot, Set<string>>), we can just have one WeakSet<ShadowRoot> for the stylesheet itself. Global styles can also rely on this same cache.

nolanlawson · 2022-05-12T16:08:59Z

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


-function createOrGetConstructableStylesheet(content: string) {
-    // It's important for CSSStyleSheets to be unique based on their content, so
-    // that adoptedStyleSheets.indexOf(sheet) works


This comment was outdated; we don't use adoptedStyleSheets.indexOf anymore.

pmdartus

Add a couple of nitpicks (feel free to ignore).

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

pmdartus · 2022-05-16T12:59:20Z

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

+    // Keep track of whether the style element has been used already, so we know if we need to clone it.
+    used: boolean;


It took me some time to wrap my head around the used flag.

What do you think about setting the stylesheet lazily to make the code more streamlined? If we update the stylesheet property to stylesheet: CSSStyleSheet | HTMLStyleElement | undefined; and set it to undefined in getCacheData. Later in createStyleElement and insertConstructableStylesheet you can create the sheet if not present and clone it otherwise. It would remove the need to have another property tracking wether the sheet should be cloned or not.

Makes sense to me. Let's do it and I'll re-run the perf numbers just to be safe.

pmdartus · 2022-05-16T13:04:10Z

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

+    used: boolean;
+};
+
+interface ConstrutableStylesheetCacheData extends CacheData {


Suggested change

interface ConstrutableStylesheetCacheData extends CacheData {

interface ConstructableStylesheetCacheData extends CacheData {

pmdartus · 2022-05-16T13:06:35Z

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

-    const stylesheet = createOrGetConstructableStylesheet(content);
+function insertConstructableStylesheet(
+    content: string,
+    target: ShadowRoot | Document,


There is a specific interface for this.

Suggested change

target: ShadowRoot | Document,

target: DocumentOrShadowRoot,

Interestingly, this doesn't work. TypeScript complains that Property 'appendChild' does not exist on type 'DocumentOrShadowRoot'. Somehow Document | ShadowRoot is different from DocumentOrShadowRoot, at least in TypeScript. 😛

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

nolanlawson · 2022-05-16T20:40:51Z

Looks like removing used actually reduces the perf improvement:

Assuming this isn't just a quirk of the benchmark, my guess is that the object shape is more ambiguous now, so V8 is confused about the hidden shape of the object. Let's keep used and I'll just add a better comment explaining it.

wip: add a static vnode type wip: needs key and patch wip: all tests passing exept a few because of simple things wip: set style tokens for fragments during template evaluation wip: set element shadow token for fragments during template evaluation wip: propagate the shadow resolver key of static fragments wip: do not gen static nodes for text or comments wip: use tagedTemplate expression to replate stylesheetToken wip: use cloneNode from renderer wip: treeWalker to work in ie11 refactor: do not strip empty attr or empty class attr fix: using incorrect key wip: trim value of textNodes and review feedback fix: hydration feat: custom static element serializer wip: remove unessesary import fix: hydration fix: snapshot tests fix: missing karma test fix: test due rebase test: add test for static content needing nodeOwner fix: escape strings in serializer refactor: remove unused apis on generated code refactor: review suggestions fix: support mixed mode wip: fix compilation snapshots fix: increase 0.5kb bundlesize for engine dom fix: flapper wip: helpers.ts review wip: codegen.ts review wip: missing items from pm review wip: review comments fix: respect preserveComments and fuse $1,2 into 3 fix: svg content with the correct namespace feat(template-compiler): add option to disable static content optimizations wip: remove invalid comment chore: bump version to v2.13.0 (#2784) chore: dependencies upgrade (#2785) test: fix Node warning about event emitters (#2789) chore: run karma and integration tests in parallel (#2792) * chore: run karma and integration tests in parallel * fix: remove log lines fix(babel-plugin-component): remove import validation (#2719) test: remove flakey IE integration test (#2796) test: update test to use lwc imports (#2794) chore: Restrict further import order (#2795) chore: bump version to v2.13.1 (#2804) refactor(engine): moving vm references from dom into core (#2801) * refactor(engine): moving vm references from dom into core chore(nucleus): remove salesforcedevs/developer-website (#2807) test(integration-karma): small quality-of-life improvements (#2809) chore(deps): bump ejs from 3.1.6 to 3.1.7 (#2810) chore: weekly dependencies upgrade (#2816) * chore: weekly dependencies upgrade * fix: update yarn.lock` refactor(engine): optimize computation of transitive shadow mode (#2803) chore(deps): bump async from 2.6.3 to 2.6.4 (#2815) Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump version to v2.13.2 (#2819) chore: retry failed Circle CI tests (#2814) * chore: retry failed Circle CI tests W-10946477 * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: deliberately fail a test to see what happens * chore: improve retry script * fix: whitespace * Revert "chore: deliberately fail a test to see what happens" This reverts commit 611fc34. * chore: rename to retry_karma fix(engine-core): add shim for old stylesheetTokens internal API (#2821) W-11093934 chore: bump version to v2.13.3 (#2823) fix(build): remove swc, switch back to babel and terser (#2818) feat: add freezeTemplate() API, warn on mutation (#2825) * feat: add freezeTemplate() API, warn on mutation * fix: warn on slots/renderMode as well * fix: add comment * fix: remove duplicate process.env.NODE_ENV check fix(engine-dom): refactor stylesheet API for consistency (#2827) * fix(engine-dom): refactor stylesheet API for consistency * fix: remove useless code comment * test: remove unnecessary test * test: remove unnecessary test * refactor: slight refactor * fix: add code comments * fix: add code comments * fix: add better comment fix: relax static id validation in iterations (#2830) fix(rollup-plugin): emit warnings during compilation (#2833) * fix(rollup-plugin): emit warnings during compilation Fixes #2771 W-10930894 * fix: add code comment fix(engine-dom): make feature flags work (#2812) * fix(engine-dom): make feature flags work Fixes #2811 * fix: license headers * test: fix jest tests * test: fix test * test: fix test * fix: use Eugene's technique instead * Revert "fix: use Eugene's technique instead" This reverts commit 72afdc0. * fix: use Eugene's technique instead * fix: revert unnecessary test change * fix: revert, use the elaborate test instead * fix: fix feature flags in engine-server as well perf(engine-dom): refactor style cache to reduce lookups (#2832) * perf(engine-dom): refactor style cache to reduce lookups * fix: tidy up comments * fix: update packages/@lwc/engine-dom/src/styles.ts Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> * fix: remove semi * fix: remove "used" flag * fix: refactor * fix: refactor * fix: bring back "used" flag * fix: typo Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> chore: update deps (#2838) test: run feature flag test code only in karma (#2835) fix: trigger slotchange event on removing slot (#2840) test(integration-karma): silence lwc rollup plugin warnings (#2836) * test(integration-karma): silence lwc rollup plugin warnings * fix: use warn API v2.11.7 (#2842) chore: release v2.14.0 (#2846) fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block fix: only add version mismatch test code in karma (#2852) test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style chore(nucleus): remove more downstreams (#2855) chore(nucleus): remove another downstream (#2857) docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> chore: fix lint test: refactor test, remove test covered in #2859 test: on second thought, bring test back

nolanlawson commented May 12, 2022

View reviewed changes

nolanlawson requested a review from pmdartus May 12, 2022 16:51

pmdartus approved these changes May 16, 2022

View reviewed changes

nolanlawson and others added 5 commits May 16, 2022 10:44

perf(engine-dom): refactor style cache to reduce lookups

66067ac

fix: tidy up comments

585e2e6

fix: update packages/@lwc/engine-dom/src/styles.ts

472df96

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

fix: remove semi

0f8f59f

fix: remove "used" flag

4fc0273

nolanlawson force-pushed the nolan/refactor-stylesheets-perf branch from 944fffc to 4fc0273 Compare May 16, 2022 17:44

nolanlawson added 2 commits May 16, 2022 10:48

fix: refactor

b7ef3b1

fix: refactor

75adb48

nolanlawson added 2 commits May 16, 2022 13:49

fix: bring back "used" flag

49d838f

fix: typo

adc2764

nolanlawson merged commit b4ac608 into master May 16, 2022

nolanlawson deleted the nolan/refactor-stylesheets-perf branch May 16, 2022 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf(engine-dom): refactor style cache to reduce lookups #2832

perf(engine-dom): refactor style cache to reduce lookups #2832

Uh oh!

nolanlawson commented May 12, 2022

Uh oh!

nolanlawson May 12, 2022 •

edited

Loading

Uh oh!

nolanlawson May 12, 2022

Uh oh!

pmdartus left a comment

Uh oh!

Uh oh!

pmdartus May 16, 2022

Uh oh!

nolanlawson May 16, 2022

Uh oh!

pmdartus May 16, 2022

Uh oh!

nolanlawson May 16, 2022

Uh oh!

pmdartus May 16, 2022

Uh oh!

nolanlawson May 16, 2022

Uh oh!

nolanlawson commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// Keep track of whether the style element has been used already, so we know if we need to clone it.
		used: boolean;

	interface ConstrutableStylesheetCacheData extends CacheData {
	interface ConstructableStylesheetCacheData extends CacheData {

perf(engine-dom): refactor style cache to reduce lookups #2832

perf(engine-dom): refactor style cache to reduce lookups #2832

Uh oh!

Conversation

nolanlawson commented May 12, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

Uh oh!

nolanlawson May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolanlawson May 12, 2022

Choose a reason for hiding this comment

Uh oh!

pmdartus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmdartus May 16, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson May 16, 2022

Choose a reason for hiding this comment

Uh oh!

pmdartus May 16, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson May 16, 2022

Choose a reason for hiding this comment

Uh oh!

pmdartus May 16, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson May 16, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nolanlawson May 12, 2022 •

edited

Loading