-
Notifications
You must be signed in to change notification settings - Fork 398
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: use constructable stylesheets #2460
Changes from all commits
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 | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,7 +4,7 @@ | |||||||||||||||||||||||||||||||||||||
* SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||
import { isArray, isUndefined, ArrayJoin, ArrayPush } from '@lwc/shared'; | ||||||||||||||||||||||||||||||||||||||
import { isArray, isUndefined, ArrayJoin, ArrayPush, isNull } from '@lwc/shared'; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import * as api from './api'; | ||||||||||||||||||||||||||||||||||||||
import { VNode } from '../3rdparty/snabbdom/types'; | ||||||||||||||||||||||||||||||||||||||
|
@@ -138,16 +138,42 @@ export function getStylesheetsContent(vm: VM, template: Template): string[] { | |||||||||||||||||||||||||||||||||||||
return content; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// It might be worth caching this to avoid doing the lookup repeatedly, but | ||||||||||||||||||||||||||||||||||||||
// perf testing has not shown it to be a huge improvement yet: | ||||||||||||||||||||||||||||||||||||||
// https://github.com/salesforce/lwc/pull/2460#discussion_r691208892 | ||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export function createStylesheet(vm: VM, stylesheets: string[]): VNode | null { | ||||||||||||||||||||||||||||||||||||||
const { renderer, renderMode, shadowMode } = vm; | ||||||||||||||||||||||||||||||||||||||
if (renderMode === RenderMode.Shadow && shadowMode === ShadowMode.Synthetic) { | ||||||||||||||||||||||||||||||||||||||
for (let i = 0; i < stylesheets.length; i++) { | ||||||||||||||||||||||||||||||||||||||
renderer.insertGlobalStylesheet(stylesheets[i]); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||
// native shadow or light DOM | ||||||||||||||||||||||||||||||||||||||
} else if (renderer.ssr) { | ||||||||||||||||||||||||||||||||||||||
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. 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. 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). 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. Sounds reasonable to me. |
||||||||||||||||||||||||||||||||||||||
// native shadow or light DOM, SSR | ||||||||||||||||||||||||||||||||||||||
const combinedStylesheetContent = ArrayJoin.call(stylesheets, '\n'); | ||||||||||||||||||||||||||||||||||||||
return createInlineStyleVNode(combinedStylesheetContent); | ||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||
// native shadow or light DOM, DOM renderer | ||||||||||||||||||||||||||||||||||||||
const root = getNearestNativeShadowComponent(vm); | ||||||||||||||||||||||||||||||||||||||
const isGlobal = isNull(root); | ||||||||||||||||||||||||||||||||||||||
for (let i = 0; i < stylesheets.length; i++) { | ||||||||||||||||||||||||||||||||||||||
if (isGlobal) { | ||||||||||||||||||||||||||||||||||||||
renderer.insertGlobalStylesheet(stylesheets[i]); | ||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||
// local level | ||||||||||||||||||||||||||||||||||||||
renderer.insertStylesheet(stylesheets[i], root!.cmpRoot); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+169
to
+176
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 wonder if the js engine optimizes this code, do you know @nolanlawson ?:
Suggested change
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. @jodarove I admit I don't know. The Unfortunately my benchmark wouldn't capture it, because it only has one stylesheet per template. 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. @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. |
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import { | |
create, | ||
hasOwnProperty, | ||
htmlPropertyToAttribute, | ||
isFunction, | ||
isUndefined, | ||
KEY__SHADOW_TOKEN, | ||
setPrototypeOf, | ||
|
@@ -29,6 +30,10 @@ if (process.env.NODE_ENV === 'development') { | |
} | ||
|
||
const globalStylesheetsParentElement: Element = document.head || document.body || document; | ||
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 }>(); | ||
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. The I would also prefer not to maintain a mapping of |
||
|
||
let getCustomElement, defineCustomElement, HTMLElementConstructor; | ||
|
||
|
@@ -54,6 +59,46 @@ function isCustomElementRegistryAvailable() { | |
} | ||
} | ||
|
||
function insertConstructableStyleSheet(content: string, target: Node) { | ||
// It's important for CSSStyleSheets to be unique based on their content, so that | ||
// `shadowRoot.adoptedStyleSheets.includes(sheet)` works. | ||
let styleSheet = styleSheets[content]; | ||
if (isUndefined(styleSheet)) { | ||
styleSheet = new CSSStyleSheet(); | ||
(styleSheet as any).replaceSync(content); | ||
styleSheets[content] = styleSheet; | ||
} | ||
if (!(target as any).adoptedStyleSheets.includes(styleSheet)) { | ||
(target as any).adoptedStyleSheets = [...(target as any).adoptedStyleSheets, styleSheet]; | ||
} | ||
} | ||
|
||
function insertStyleElement(content: string, target: Node) { | ||
// Avoid inserting duplicate `<style>`s | ||
let sheets = nodesToStyleSheets.get(target); | ||
if (isUndefined(sheets)) { | ||
sheets = create(null); | ||
nodesToStyleSheets.set(target, sheets!); | ||
} | ||
if (sheets![content]) { | ||
return; | ||
} | ||
sheets![content] = true; | ||
|
||
// This `<style>` may be repeated multiple times in the DOM, so cache it. It's a bit | ||
// faster to call `cloneNode()` on an existing node than to recreate it every time. | ||
let elm = styleElements[content]; | ||
if (isUndefined(elm)) { | ||
elm = document.createElement('style'); | ||
elm.type = 'text/css'; | ||
elm.textContent = content; | ||
styleElements[content] = elm; | ||
} else { | ||
elm = elm.cloneNode(true) as HTMLStyleElement; | ||
} | ||
target.appendChild(elm); | ||
} | ||
|
||
if (isCustomElementRegistryAvailable()) { | ||
getCustomElement = customElements.get.bind(customElements); | ||
defineCustomElement = customElements.define.bind(customElements); | ||
|
@@ -244,6 +289,15 @@ export const renderer: Renderer<Node, Element> = { | |
globalStylesheetsParentElement.appendChild(elm); | ||
}, | ||
|
||
insertStylesheet(content: string, target: Node): void { | ||
if (supportsConstructableStyleSheets) { | ||
insertConstructableStyleSheet(content, target); | ||
} else { | ||
// Fall back to <style> element | ||
insertStyleElement(content, target); | ||
} | ||
}, | ||
|
||
assertInstanceOfHTMLElement(elm: any, msg: string) { | ||
assert.invariant(elm instanceof HTMLElement, msg); | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,9 @@ describe('multiple templates', () => { | |
element.next(); | ||
return Promise.resolve().then(() => { | ||
expect(element.querySelector('div').textContent).toEqual('b'); | ||
expect(getComputedStyle(element.querySelector('div')).color).toEqual('rgb(0, 0, 0)'); | ||
expect(getComputedStyle(element.querySelector('div')).color).toEqual( | ||
'rgb(233, 150, 122)' | ||
); | ||
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. 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 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 opened a separate issue for this; it's a general problem: #2466 |
||
expect(getComputedStyle(element.querySelector('div')).marginLeft).toEqual('10px'); | ||
// element should not be dirty after template change | ||
expect(element.querySelector('div').hasAttribute('foo')).toEqual(false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { createElement } from 'lwc'; | ||
import Multi from 'x/multi'; | ||
|
||
if (process.env.NATIVE_SHADOW) { | ||
describe('Shadow DOM styling - multiple shadow DOM components', () => { | ||
it('Does not duplicate styles if template is re-rendered', () => { | ||
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. This test confirms we don't end up with duplicate 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 |
||
const element = createElement('x-multi', { is: Multi }); | ||
|
||
const getNumStyleSheets = () => { | ||
if (element.shadowRoot.adoptedStyleSheets) { | ||
return element.shadowRoot.adoptedStyleSheets.length; | ||
} else { | ||
return element.shadowRoot.querySelectorAll('style').length; | ||
} | ||
}; | ||
|
||
document.body.appendChild(element); | ||
return Promise.resolve() | ||
.then(() => { | ||
expect(getComputedStyle(element.shadowRoot.querySelector('div')).color).toEqual( | ||
'rgb(0, 0, 255)' | ||
); | ||
expect(getNumStyleSheets()).toEqual(1); | ||
element.next(); | ||
}) | ||
.then(() => { | ||
expect(getComputedStyle(element.shadowRoot.querySelector('div')).color).toEqual( | ||
'rgb(255, 0, 0)' | ||
); | ||
expect(getNumStyleSheets()).toEqual(2); | ||
element.next(); | ||
}) | ||
.then(() => { | ||
expect(getComputedStyle(element.shadowRoot.querySelector('div')).color).toEqual( | ||
'rgb(0, 0, 255)' | ||
); | ||
expect(getNumStyleSheets()).toEqual(2); | ||
}); | ||
}); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.blue { | ||
color: blue; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div class="blue">a</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.red { | ||
color: red; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div class="red">b</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
import A from './a.html'; | ||
import B from './b.html'; | ||
|
||
export default class Multi extends LightningElement { | ||
current = A; | ||
|
||
@api | ||
next() { | ||
this.current = this.current === A ? B : A; | ||
} | ||
|
||
render() { | ||
return this.current; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,14 @@ import path from 'path'; | |
import glob from 'glob'; | ||
import lwc from '@lwc/rollup-plugin'; | ||
import replace from '@rollup/plugin-replace'; | ||
import { generateStyledComponents } from './scripts/generate-styled-components'; | ||
|
||
const rootDir = path.join(__dirname, 'src'); | ||
const { tmpDir, styledComponents } = generateStyledComponents(); | ||
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. It is not ideal that we have to add a one-off for a specific test suite. 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. 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. |
||
|
||
function createConfig(componentFile, engineType) { | ||
const rootDir = componentFile.includes(tmpDir) | ||
? path.join(tmpDir, 'src') | ||
: path.join(__dirname, 'src'); | ||
const lwcImportModule = engineType === 'server' ? '@lwc/engine-server' : '@lwc/engine-dom'; | ||
return { | ||
input: componentFile, | ||
|
@@ -34,7 +38,7 @@ function createConfig(componentFile, engineType) { | |
}), | ||
], | ||
output: { | ||
file: componentFile.replace('/src/', `/dist/${engineType}/`), | ||
file: componentFile.replace(tmpDir, __dirname).replace('/src/', `/dist/${engineType}/`), | ||
format: 'esm', | ||
}, | ||
// These packages need to be external so that perf-benchmarks can potentially swap them out | ||
|
@@ -43,7 +47,7 @@ function createConfig(componentFile, engineType) { | |
}; | ||
} | ||
|
||
const components = glob.sync(path.join(__dirname, 'src/**/*.js')); | ||
const components = [...glob.sync(path.join(__dirname, 'src/**/*.js')), ...styledComponents]; | ||
|
||
const config = ['server', 'dom'] | ||
.map((engineType) => components.map((component) => createConfig(component, engineType))) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
* Copyright (c) 2018, salesforce.com, 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 path from 'path'; | ||
import fs from 'fs'; | ||
import os from 'os'; | ||
|
||
const NUM_COMPONENTS = 1000; | ||
|
||
// Generates some components with individual CSS for each one. | ||
// We could use @rollup/plugin-virtual for this, but @lwc/rollup-plugin deliberately | ||
// filters virtual modules, so it's simpler to just write to a temp dir. | ||
export function generateStyledComponents() { | ||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lwc-')); | ||
|
||
const components = Array(NUM_COMPONENTS) | ||
.fill() | ||
.map((_, i) => | ||
path.join(tmpDir, `src/benchmark/styledComponent${i}/styledComponent${i}.js`) | ||
); | ||
|
||
components.forEach((jsFilename, i) => { | ||
const cssFilename = jsFilename.replace('.js', '.css'); | ||
const htmlFilename = jsFilename.replace('.js', '.html'); | ||
|
||
const js = | ||
'import { LightningElement } from "lwc"; export default class extends LightningElement {}'; | ||
const css = `div { color: ${i.toString(16).padStart(6, '0')}}`; | ||
const html = '<template><div></div></template>'; | ||
|
||
fs.mkdirSync(path.dirname(jsFilename), { recursive: true }); | ||
fs.writeFileSync(jsFilename, js, 'utf-8'); | ||
fs.writeFileSync(cssFilename, css, 'utf-8'); | ||
fs.writeFileSync(htmlFilename, html, 'utf-8'); | ||
}); | ||
|
||
const oneComponentFilename = path.join(tmpDir, 'src/benchmark/styledComponent.js'); | ||
const oneComponent = `export { default } from ${JSON.stringify(components[0])};`; | ||
fs.writeFileSync(oneComponentFilename, oneComponent, 'utf-8'); | ||
|
||
const allComponentsFilename = path.join(tmpDir, 'src/benchmark/styledComponents.js'); | ||
const allComponents = ` | ||
${components.map((mod, i) => `import cmp${i} from ${JSON.stringify(mod)}`).join(';')}; | ||
export default [${components.map((_, i) => `cmp${i}`).join(',')}];`; | ||
fs.writeFileSync(allComponentsFilename, allComponents, 'utf-8'); | ||
|
||
return { | ||
styledComponents: [oneComponentFilename, allComponentsFilename], | ||
tmpDir, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright (c) 2018, salesforce.com, 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 components from 'perf-benchmarks-components/dist/dom/benchmark/styledComponents.js'; | ||
import { styledComponentBenchmark } from '../../../utils/styledComponentBenchmark'; | ||
|
||
// Create 1k components with different CSS in each component | ||
styledComponentBenchmark(`ss-benchmark-styled-component/create/1k/different`, components); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright (c) 2018, salesforce.com, 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 StyledComponent from 'perf-benchmarks-components/dist/dom/benchmark/styledComponent.js'; | ||
import { styledComponentBenchmark } from '../../../utils/styledComponentBenchmark'; | ||
|
||
// Create 1k components with the same CSS in each component | ||
styledComponentBenchmark(`ss-benchmark-styled-component/create/1k/same`, StyledComponent); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright (c) 2018, salesforce.com, 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 components from 'perf-benchmarks-components/dist/dom/benchmark/styledComponents.js'; | ||
import { styledComponentBenchmark } from '../../../utils/styledComponentBenchmark'; | ||
|
||
// Create 1k components with different CSS in each component | ||
styledComponentBenchmark(`benchmark-styled-component/create/1k/different`, components); |
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.
Would it be better performance-wise to store this on the VM instead of looking this up lazily?
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.
This is worth testing, although we'd have to be careful about when to invalidate the cache (e.g. if the component is moved).
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.
@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
(Note tip-of-tree is the
nolan/constructable-stylesheets-redux
branch in this case.)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.
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.