-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(engine-core): reduce fragment cache objects #4431
Changes from all commits
1c59f92
1bd8045
3753ba2
c3194cc
e48d37e
39db455
fba2ebd
bd092fd
d437138
3fdcaf4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* Copyright (c) 2024, Salesforce, Inc. | ||
* All rights reserved. | ||
* SPDX-License-Identifier: MIT | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||
*/ | ||
import { ArrayFrom } from '@lwc/shared'; | ||
|
||
export const enum FragmentCacheKey { | ||
HAS_SCOPED_STYLE = 1, | ||
SHADOW_MODE_SYNTHETIC = 2, | ||
} | ||
|
||
// HAS_SCOPED_STYLE | SHADOW_MODE_SYNTHETIC = 3 | ||
const MAX_CACHE_KEY = 3; | ||
|
||
// Mapping of cacheKeys to `string[]` (assumed to come from a tagged template literal) to an Element. | ||
// Note that every unique tagged template literal will have a unique `string[]`. So by using `string[]` | ||
// as the WeakMap key, we effectively associate each Element with a unique tagged template literal. | ||
// See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#tagged_templates | ||
// Also note that this array only needs to be large enough to account for the maximum possible cache key | ||
const fragmentCache: WeakMap<string[], Element>[] = ArrayFrom( | ||
{ length: MAX_CACHE_KEY + 1 }, | ||
() => new WeakMap() | ||
); | ||
|
||
// Only used in LWC's Karma tests | ||
if (process.env.NODE_ENV === 'test-karma-lwc') { | ||
(window as any).__lwcResetFragmentCache = () => { | ||
for (let i = 0; i < fragmentCache.length; i++) { | ||
fragmentCache[i] = new WeakMap(); | ||
} | ||
}; | ||
} | ||
|
||
function checkIsBrowser() { | ||
// The fragment cache only serves prevent calling innerHTML multiple times which doesn't happen on the server. | ||
/* istanbul ignore next */ | ||
if (!process.env.IS_BROWSER) { | ||
throw new Error( | ||
'The fragment cache is intended to only be used in @lwc/engine-dom, not @lwc/engine-server' | ||
); | ||
} | ||
} | ||
|
||
export function getFromFragmentCache(cacheKey: number, strings: string[]) { | ||
checkIsBrowser(); | ||
return fragmentCache[cacheKey].get(strings); | ||
} | ||
|
||
export function setInFragmentCache(cacheKey: number, strings: string[], element: Element) { | ||
checkIsBrowser(); | ||
fragmentCache[cacheKey].set(strings, element); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ import { | |
isUndefined, | ||
KEY__SCOPED_CSS, | ||
keys, | ||
noop, | ||
StringCharAt, | ||
STATIC_PART_TOKEN_ID, | ||
toString, | ||
|
@@ -47,6 +46,7 @@ import { getTemplateOrSwappedTemplate, setActiveVM } from './hot-swaps'; | |
import { MutableVNodes, VNodes, VStaticPart, VStaticPartElement, VStaticPartText } from './vnodes'; | ||
import { RendererAPI } from './renderer'; | ||
import { getMapFromClassName } from './modules/computed-class-attr'; | ||
import { FragmentCacheKey, getFromFragmentCache, setInFragmentCache } from './fragment-cache'; | ||
|
||
export interface Template { | ||
(api: RenderAPI, cmp: object, slotSet: SlotSet, cache: TemplateCache): VNodes; | ||
|
@@ -238,40 +238,11 @@ function serializeClassAttribute(part: VStaticPartElement, classToken: string) { | |
return computedClassName.length ? ` class="${htmlEscape(computedClassName, true)}"` : ''; | ||
} | ||
|
||
const enum FragmentCache { | ||
HAS_SCOPED_STYLE = 1, | ||
SHADOW_MODE_SYNTHETIC = 2, | ||
} | ||
|
||
// This should be a no-op outside of LWC's Karma tests, where it's not needed | ||
let registerFragmentCache: (fragmentCache: any) => void = noop; | ||
|
||
// Only used in LWC's Karma tests | ||
if (process.env.NODE_ENV === 'test-karma-lwc') { | ||
// Keep track of fragmentCaches, so we can clear them in LWC's Karma tests | ||
const fragmentCaches: any[] = []; | ||
registerFragmentCache = (fragmentCache: any) => { | ||
fragmentCaches.push(fragmentCache); | ||
}; | ||
|
||
(window as any).__lwcResetFragmentCaches = () => { | ||
for (const fragmentCache of fragmentCaches) { | ||
for (const key of keys(fragmentCache)) { | ||
delete fragmentCache[key]; | ||
} | ||
} | ||
}; | ||
} | ||
|
||
function buildParseFragmentFn( | ||
createFragmentFn: (html: string, renderer: RendererAPI) => Element | ||
): (strings: string[], ...keys: (string | number)[]) => () => Element { | ||
return (strings: string[], ...keys: (string | number)[]) => { | ||
const cache = create(null); | ||
|
||
registerFragmentCache(cache); | ||
|
||
return function (parts?: VStaticPart[]): Element { | ||
return function parseFragment(strings: string[], ...keys: (string | number)[]) { | ||
return function applyFragmentParts(parts?: VStaticPart[]): Element { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to name these functions because it's easier to read in a perf trace. |
||
const { | ||
context: { hasScopedStyles, stylesheetToken, legacyStylesheetToken }, | ||
shadowMode, | ||
|
@@ -284,16 +255,16 @@ function buildParseFragmentFn( | |
|
||
let cacheKey = 0; | ||
if (hasStyleToken && hasScopedStyles) { | ||
cacheKey |= FragmentCache.HAS_SCOPED_STYLE; | ||
cacheKey |= FragmentCacheKey.HAS_SCOPED_STYLE; | ||
} | ||
if (hasStyleToken && isSyntheticShadow) { | ||
cacheKey |= FragmentCache.SHADOW_MODE_SYNTHETIC; | ||
cacheKey |= FragmentCacheKey.SHADOW_MODE_SYNTHETIC; | ||
} | ||
|
||
// Cache is only here to prevent calling innerHTML multiple times which doesn't happen on the server. | ||
if (process.env.IS_BROWSER) { | ||
// Disable this on the server to prevent cache poisoning when expressions are used. | ||
const cached = cache[cacheKey]; | ||
const cached = getFromFragmentCache(cacheKey, strings); | ||
if (!isUndefined(cached)) { | ||
return cached; | ||
} | ||
|
@@ -343,9 +314,14 @@ function buildParseFragmentFn( | |
|
||
htmlFragment += strings[strings.length - 1]; | ||
|
||
cache[cacheKey] = createFragmentFn(htmlFragment, renderer); | ||
const element = createFragmentFn(htmlFragment, renderer); | ||
|
||
// 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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 Would it make sense to put the browser check within the function so that we don't forget about it in other places? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: 3fdcaf4 |
||
|
||
return cache[cacheKey]; | ||
return element; | ||
}; | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
import { createElement } from 'lwc'; | ||
import { LOWERCASE_SCOPE_TOKENS } from 'test-utils'; | ||
|
||
import NativeScopedStyles from 'x/nativeScopedStyles'; | ||
import NativeStyles from 'x/nativeStyles'; | ||
import NoStyles from 'x/noStyles'; | ||
import ScopedStyles from 'x/scopedStyles'; | ||
import Styles from 'x/styles'; | ||
|
||
const scenarios = [ | ||
{ | ||
name: 'no styles', | ||
Ctor: NoStyles, | ||
tagName: 'x-no-styles', | ||
expectedColor: 'rgb(0, 0, 0)', | ||
expectClass: false, | ||
expectAttribute: false, | ||
}, | ||
{ | ||
name: 'styles', | ||
Ctor: Styles, | ||
tagName: 'x-styles', | ||
expectedColor: 'rgb(255, 0, 0)', | ||
expectClass: false, | ||
expectAttribute: !process.env.NATIVE_SHADOW, | ||
}, | ||
{ | ||
name: 'scoped styles', | ||
Ctor: ScopedStyles, | ||
tagName: 'x-scoped-styles', | ||
expectedColor: 'rgb(0, 128, 0)', | ||
expectClass: true, | ||
expectAttribute: !process.env.NATIVE_SHADOW, | ||
}, | ||
{ | ||
name: 'native styles', | ||
Ctor: NativeStyles, | ||
tagName: 'x-native-styles', | ||
expectedColor: 'rgb(255, 0, 0)', | ||
expectClass: false, | ||
expectAttribute: false, | ||
}, | ||
{ | ||
name: 'native scoped styles', | ||
Ctor: NativeScopedStyles, | ||
tagName: 'x-native-scoped-styles', | ||
expectedColor: 'rgb(0, 128, 0)', | ||
expectClass: true, | ||
expectAttribute: false, | ||
}, | ||
]; | ||
|
||
// These tests confirm that the fragment cache (from `fragment-cache.ts`) is working correctly. Fragments should be | ||
// unique based on 1) synthetic vs native shadow, and 2) presence or absence of scoped styles. If the fragment cache is | ||
// not working correctly, then we may end up rendering the wrong styles or the wrong attribute/class scope token due to | ||
// the cache being poisoned, e.g. an HTML string for scoped styles being rendered for non-scoped styles. | ||
// To test this, we re-use the same `template.html` but change the `static stylesheets` in each component. | ||
scenarios.forEach(({ name, Ctor, tagName, expectedColor, expectClass, expectAttribute }) => { | ||
describe(name, () => { | ||
let h1; | ||
|
||
beforeEach(async () => { | ||
const elm = createElement(tagName, { is: Ctor }); | ||
document.body.appendChild(elm); | ||
await Promise.resolve(); | ||
h1 = elm.shadowRoot.querySelector('h1'); | ||
}); | ||
|
||
it('renders the correct styles', () => { | ||
expect(getComputedStyle(h1).color).toBe(expectedColor); | ||
}); | ||
|
||
it('renders the correct attributes/classes', () => { | ||
const scopeToken = LOWERCASE_SCOPE_TOKENS ? 'lwc-2it5vhebv0i' : 'x-template_template'; | ||
|
||
expect(h1.getAttribute('class')).toBe(expectClass ? scopeToken : null); | ||
expect(h1.hasAttribute(scopeToken)).toBe(expectAttribute); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { LightningElement } from 'lwc'; | ||
import template from '../template/template.html'; | ||
import styles from '../stylesheets/scopedStyles.scoped.css'; | ||
|
||
export default class extends LightningElement { | ||
static shadowSupportMode = 'native'; | ||
static stylesheets = [styles]; | ||
|
||
render() { | ||
return template; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { LightningElement } from 'lwc'; | ||
import template from '../template/template.html'; | ||
import styles from '../stylesheets/styles.css'; | ||
|
||
export default class extends LightningElement { | ||
static shadowSupportMode = 'native'; | ||
static stylesheets = [styles]; | ||
|
||
render() { | ||
return template; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { LightningElement } from 'lwc'; | ||
import template from '../template/template.html'; | ||
|
||
export default class extends LightningElement { | ||
render() { | ||
return template; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { LightningElement } from 'lwc'; | ||
import template from '../template/template.html'; | ||
import styles from '../stylesheets/scopedStyles.scoped.css'; | ||
|
||
export default class extends LightningElement { | ||
static stylesheets = [styles]; | ||
|
||
render() { | ||
return template; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { LightningElement } from 'lwc'; | ||
import template from '../template/template.html'; | ||
import styles from '../stylesheets/styles.css'; | ||
|
||
export default class extends LightningElement { | ||
static stylesheets = [styles]; | ||
|
||
render() { | ||
return template; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
h1 { | ||
color: green; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
h1 { | ||
color: red; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<h1>hello</h1> | ||
</template> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this cache was effectively tied to every tagged template literal. Each tagged template literal has a unique
string[]
object, so using thestring[]
as the cache key in a WeakMap effectively gives us a unique-per-tagged-template-literal cache.