perf(engine-core): reduce fragment cache objects #4431

nolanlawson · 2024-07-31T23:34:15Z

Details

This is a small perf improvement to how we handle fragment caches. The main benefit is in using less memory and creating fewer objects.

Fragment caches are used in compiled templates. They're used whenever you see e.g.:

const $fragment1 = parseFragment`<div ${3}>yolo</div>`;

Under the hood, parseFragment will cache based on 1) the tagged template literal and 2) a cache key based on synthetic-vs-native shadow and whether scoped styles are present. The reason for the second cache key is that the HTML string will be different in different cases:

<!-- no styles-->
<div>yolo</div>

<!-- synthetic shadow with styles -->
<div lwc-123abc>yolo</div>

<!-- scoped styles -->
<div class="lwc-123abc">yolo</div>

<!-- synthetic shadow with scoped styles -->
<div lwc-123abc class="lwc-123abc">yolo</div>

(The main reasons for this are 1) historical – synthetic shadow scoping uses attributes whereas scoped styles use classes, and 2) performance – we avoid rendering attributes/classes unless absolutely necessary.)

The problem with the current fragment cache is that it creates one cache object per tagged template literal. Each cache object is just a mapping from number to Element. This is wasteful because there are only 4 numbers we have to worry about (corresponding to the 4 cases above). So we only really need 4 maps. I.e.:

// wasteful!
taggedTemplate -> number -> Element

// better!
number -> taggedTemplate -> Element

Benchmark

This PR is perf-neutral for our current benchmarks, but I did write a new benchmark to demonstrate the improvement. This benchmark works because it measures the time to evaluate the compiled LWC template, rather than the time to render it. The improvement is 2-6%:

This number is actually an undercount, since the benchmark actually measures the time to download the .js file as well as the time to evaluate it:

(Sadly I could not get this benchmark to work in Best, which is why I'm not committing it to master. Also it's not very good due to the above issue with network vs evaluation time. Deferred import evaluation would help a lot here.)

Tests

I also added a bunch of new Karma tests to ensure I didn't break anything. The tests pass with and without this PR.

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 · 2024-07-31T23:34:43Z

packages/@lwc/engine-core/src/framework/fragment-cache.ts

+}
+
+// Mapping of cacheKeys to token array (assumed to come from a tagged template literal) to an Element
+const fragmentCache: Map<number, WeakMap<string[], Element>> = new Map();


I considered just creating 4 WeakMaps but it didn't seem to offer a perf improvement.

nolanlawson · 2024-07-31T23:35:31Z

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

 function buildParseFragmentFn(
 createFragmentFn: (html: string, renderer: RendererAPI) => Element
 ): (strings: string[], ...keys: (string | number)[]) => () => Element {
- return (strings: string[], ...keys: (string | number)[]) => {
- const cache = create(null);


Note this cache was effectively tied to every tagged template literal. Each tagged template literal has a unique string[] object, so using the string[] as the cache key in a WeakMap effectively gives us a unique-per-tagged-template-literal cache.

nolanlawson · 2024-07-31T23:35:52Z

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

-
- return function (parts?: VStaticPart[]): Element {
+ return function parseFragment(strings: string[], ...keys: (string | number)[]) {
+ return function applyFragmentParts(parts?: VStaticPart[]): Element {


I decided to name these functions because it's easier to read in a perf trace.

nolanlawson · 2024-07-31T23:44:04Z

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

+ // Cache is only here to prevent calling innerHTML multiple times which doesn't happen on the server.
+ if (process.env.IS_BROWSER) {
+ setInFragmentCache(cacheKey, strings, element);
+ }


Small bug I noticed – the fragment cache is only ever used in the browser, but we were still (uselessly) setting it here even on the server.

🤔 Would it make sense to put the browser check within the function so that we don't forget about it in other places?

Done: 3fdcaf4

nolanlawson · 2024-08-01T16:24:34Z

/nucleus test

salesforce-nucleus · 2024-08-01T16:24:38Z

Well that's strange! A workflow could not be found for this PR. Please try running the /nucleus start command to start the workflow.

nolanlawson · 2024-08-01T16:24:45Z

/nucleus start

…-cache

nolanlawson · 2024-08-05T22:11:47Z

packages/@lwc/engine-core/src/framework/fragment-cache.ts

+ new WeakMap(),
+ new WeakMap(),
+ new WeakMap(),
+];


A bit embarrassing, but using ArrayFill/ArrayFrom is really gnarly given our @lwc/shared conventions. This is a lot more readable than those options.

ArrayFrom({length: 4}, () => new WeakMap())

Doesn't seem too gnarly to me. 🤷

Ah, I didn't know about that shorthand where you pass in the function as the second arg! You're right, that's much nicer.

d437138

wjhsf · 2024-08-06T14:47:07Z

packages/@lwc/engine-core/src/framework/fragment-cache.ts

+ new WeakMap(),
+ new WeakMap(),
+ new WeakMap(),
+];


ArrayFrom({length: 4}, () => new WeakMap())

Doesn't seem too gnarly to me. 🤷

wjhsf · 2024-08-06T15:03:00Z

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

+ // Cache is only here to prevent calling innerHTML multiple times which doesn't happen on the server.
+ if (process.env.IS_BROWSER) {
+ setInFragmentCache(cacheKey, strings, element);
+ }


🤔 Would it make sense to put the browser check within the function so that we don't forget about it in other places?

perf(engine-core): reduce fragment cache objects

1c59f92

nolanlawson requested a review from a team as a code owner July 31, 2024 23:34

nolanlawson commented Jul 31, 2024

View reviewed changes

nolanlawson added 3 commits July 31, 2024 16:39

fix: improve comment

1bd8045

fix: whoops header

3753ba2

fix: only set in server

c3194cc

nolanlawson commented Jul 31, 2024

View reviewed changes

nolanlawson added 4 commits August 5, 2024 14:55

refactor: use an array instead of a Map

e48d37e

fix: comment

39db455

Merge remote-tracking branch 'origin/master' into nolan/perf-fragment…

fba2ebd

…-cache

refactor: simplify further

bd092fd

nolanlawson commented Aug 5, 2024

View reviewed changes

wjhsf approved these changes Aug 6, 2024

View reviewed changes

nolanlawson added 2 commits August 6, 2024 10:37

refactor: use ArrayFrom

d437138

refactor: add extra guard for browser vs server

3fdcaf4

nolanlawson merged commit fd3c9e6 into master Aug 6, 2024
11 checks passed

nolanlawson deleted the nolan/perf-fragment-cache branch August 6, 2024 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(engine-core): reduce fragment cache objects #4431

perf(engine-core): reduce fragment cache objects #4431

nolanlawson commented Jul 31, 2024

nolanlawson Jul 31, 2024

nolanlawson Jul 31, 2024

nolanlawson Jul 31, 2024

nolanlawson Jul 31, 2024

wjhsf Aug 6, 2024

nolanlawson Aug 6, 2024

nolanlawson commented Aug 1, 2024

salesforce-nucleus bot commented Aug 1, 2024

nolanlawson commented Aug 1, 2024

nolanlawson Aug 5, 2024

wjhsf Aug 6, 2024 •

edited

Loading

nolanlawson Aug 6, 2024

wjhsf Aug 6, 2024 •

edited

Loading

wjhsf Aug 6, 2024

perf(engine-core): reduce fragment cache objects #4431

perf(engine-core): reduce fragment cache objects #4431

Conversation

nolanlawson commented Jul 31, 2024

Details

Benchmark

Tests

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

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 commented Aug 1, 2024

salesforce-nucleus bot commented Aug 1, 2024

nolanlawson commented Aug 1, 2024

Choose a reason for hiding this comment

wjhsf Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhsf Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhsf Aug 6, 2024 •

edited

Loading

wjhsf Aug 6, 2024 •

edited

Loading