perf: move renderer to top-level module #2598

nolanlawson · 2021-12-07T22:51:23Z

Details

Hoists renderer.ts into a top-level module so that it's no longer its own object. Avoids the need to pass renderer around in the engine APIs. Allows Rollup to more heavily optimize the bundle, since the renderer APIs are now effectively global to the entire JS file.

This reduces the size of engine-dom.min.js from 49.7 kB to 47.8 (-1.9k, -3.8%) and improves many benchmarks by 1-4%:

I also did so by maintaining type safety. In fact, it seems the existing type system wasn't catching some subtle incompatibilities between the server renderer and the DOM renderer (like optional arguments). Those incompatibilities are now fixed.

The main downside of this approach is that we have a new public package, @lwc/renderer-abstract. The whole point of this package is just to define the types for the renderer API. It's a peerDep of @lwc/engine-core (where it's designed to be replaced at build time), and a dep of @lwc/engine-dom and @lwc/engine-server (which need the types).

I think the downsides are worth it, because it will be hard to find such a low-risk 1-4% perf improvement anywhere else. Plus, losing 3.8% of our minified bundle size is pretty significant.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

If someone is trying to use @lwc/engine-core directly, it will now throw an error unless they swap out @lwc/renderer-abstract at build time:

lwc/packages/@lwc/renderer-abstract/index.js

Line 7 in 38b0c1c

throw new Error('@lwc/renderer-abstract should not be imported directly!');

I doubt anyone is doing this though.

nolanlawson · 2021-12-07T22:52:09Z

packages/@lwc/engine-core/src/framework/html-element.ts

+// to inject at runtime.
+export const HTMLElementConstructor: typeof HTMLElement =
+    typeof HTMLElement !== 'undefined' ? HTMLElement : (function () {} as any);
+export const HTMLElementPrototype = HTMLElementConstructor.prototype;


Copied from the old renderer.ts. I didn't want to put this in @lwc/renderer-abstract because it's only needed in @lwc/engine-core.

HTMLElement is already exposed by the renderer. It might be something we can refactor in a follow-up PR.

nolanlawson · 2021-12-07T22:53:20Z

packages/@lwc/engine-dom/scripts/rollup.config.js

+                }
+                return null;
+            },
+        },


I finally figured out how to replace packages in Rollup; I should apply this technique to perf-benchmarks instead of using string replacement...

nolanlawson · 2021-12-07T22:55:03Z

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

+typecheckedRenderer = abstractRenderer;
+
+// eslint-disable-next-line no-console
+console.log(typecheckedRenderer); // use the variable so TypeScript doesn't complain


There is probably a more elegant way to do this typechecking, but I couldn't find it. TypeScript doesn't seem to have a concept of "abstract interface for a whole module, not just an object."

Luckily we can just import * as foo from "module" and compare the types on the imported object. This will fail if there is any discrepancy between the abstract renderer and the implemented renderer.

nolanlawson · 2021-12-07T22:55:42Z

packages/@lwc/renderer-abstract/index.d.ts

+export declare function getCustomElement(name: string): CustomElementConstructor | undefined;
+
+declare const HTMLElementInner: typeof HTMLElement;
+export { HTMLElementInner as HTMLElement };


I could have used actual TypeScript here, but all I need is the .d.ts file, so it seemed like overkill.

nolanlawson · 2021-12-07T22:57:15Z

packages/@lwc/renderer-abstract/package.json

+    "publishConfig": {
+        "access": "public"
+    }
+}


Technically we don't need to publish @lwc/renderer-abstract, since we only need it at build time. But I guess we might as well? It's just an index.js file and an index.d.ts file.

nolanlawson · 2021-12-07T23:03:49Z

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

+    } else {
+        element.removeAttributeNS(namespace, name);
+    }
+}


The setAttribute/removeAttribute/getAttribute APIs were inconsistent before:

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

Line 27 in 43bad75

setAttribute(element: E, name: string, value: string, namespace?: string | null): void;

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

Line 225 in 43bad75

setAttribute(element: Element, name: string, value: string, namespace?: string): void {

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

Line 230 in 43bad75

setAttribute(element, name, value, namespace = null) {

I suppose technically we should be handling the null case for namespace in @lwc/engine-dom, but we weren't doing it before, so I kept it as-is. I made an effort not to actually change the implementation of the dom/server renderers anywhere; just the types.

nolanlawson · 2021-12-07T23:05:19Z

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

+    options?: EventListenerOptions | boolean
+): void {
+    target.removeEventListener(type, callback, options);
+}


The addEventListener/removeEventListener options were inconsistent before:

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

Lines 29 to 34 in 43bad75

addEventListener(

target: E,

type: string,

callback: (event: Event) => any,

options?: AddEventListenerOptions | boolean

): void;

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

Lines 239 to 243 in 43bad75

addEventListener(

target: Node,

type: string,

callback: EventListener,

options: AddEventListenerOptions | boolean

nolanlawson · 2022-01-03T23:55:58Z

I'll add this to Dev Sync. I think these changes are worth making, but it's probably controversial since it's a big refactor.

nolanlawson · 2022-01-05T17:21:20Z

The good news is that, with @divmain's suggestion, the bundle size still decreases by quite a bit, and the perf benchmarks are still improved. It turns out that terser is able to mangle this:

let foo

function setFoo(_foo) {
  foo = _foo
}

setFoo("foo")

into this:

let foo, foo = "foo"

...which is just about as good as const foo = "foo".

Bundle size difference (including the previous version of this PR):

52K     __base/engine-dom/iife/es2017/engine-dom.min.js
48K     after-renderer-hoist/engine-dom/iife/es2017/engine-dom.min.js
48K     after-renderer-hoist-2/engine-dom/iife/es2017/engine-dom.min.js

Brotli-compressed:

14.7 kB     __base/engine-dom/iife/es2017/engine-dom.min.js
14.2 kB     after-renderer-hoist/engine-dom/iife/es2017/engine-dom.min.js
14.4 kB     after-renderer-hoist-2/engine-dom/iife/es2017/engine-dom.min.js

The perf tests don't use the minified format, but they still show an improvement:

pmdartus · 2022-01-06T15:28:04Z

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

+//
+
+export let ssr: boolean;
+export function setSsr(ssrImpl: boolean) {


What do you think about creating a single setter accepting an object instead of multiple setters?

The only downside I see with this approach is that it will increase the bundle size a little, but it should preserve the same performance benefits.

I'll need to test to see what the impact is. I suspect you're right that the minified size will not be as small, but the runtime perf would be about the same.

It turns out that "one big setter" actually increases the bundle size, rather than decreases it:

Version min min+brotli

Baseline 49.6k 14.7k

Original PR 47.7 14.2k

Individual setters 48.2k 14.4k

One big setter 49.7k 14.8k

Looking at the code though, it seems like it should probably have the same runtime performance as individual setters. Each exported function is just a top-level let that is set in the one big setter.

Overall I think I would prefer to keep the individual setters, even if the code is a bit ugly. The ugliness is limited to initializeRenderer.ts and engine-core/src/renderer.ts, and we only have to add new setters if we add new APIs to the renderer.

I wish there were a way to have the same perf as my original PR, but without the ugly code. 😛

packages/@lwc/engine-core/src/framework/base-lightning-element.ts

pmdartus · 2022-01-06T15:42:32Z

packages/@lwc/engine-core/src/framework/html-element.ts

+// to inject at runtime.
+export const HTMLElementConstructor: typeof HTMLElement =
+    typeof HTMLElement !== 'undefined' ? HTMLElement : (function () {} as any);
+export const HTMLElementPrototype = HTMLElementConstructor.prototype;


HTMLElement is already exposed by the renderer. It might be something we can refactor in a follow-up PR.

pmdartus · 2022-01-06T15:44:50Z

packages/@lwc/engine-core/src/framework/html-element.ts

+// This is a temporary workaround to get the @lwc/engine-server to evaluate in node without having
+// to inject at runtime.


This comment is not accurate any more as we are now injecting the APIs. The HTMLElement is one of the injected APIs. It's something we can probably refactor in a follow-up PR.

caridy · 2022-01-06T19:24:24Z

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

@@ -51,3 +50,46 @@ export type {
    WireAdapterConstructor,
    WireAdapterSchemaValue,
 } from './wiring';
+
+export {


Can we add a note here saying that all these methods are intended to be used for packages overriding the dom mechanism? Also, can we use export * from instead here?

I can add a note. But I don't think we can export * because the renderer.ts exposes plenty of APIs that are only used internally to engine-core that we don't want exposed (getAttribute, hasAttribute, etc.).

caridy · 2022-01-06T19:34:23Z

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

+    defineCustomElement = defineCustomElementImpl;
+}
+
+type getCustomElementFunc = (name: string) => CustomElementConstructor | undefined;


not sure why it is | undefined, some weird stuff related to SSR?

This was the pre-existing type:

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

Line 64 in 1b2e246

getCustomElement(name: string): CustomElementConstructor | undefined;

I think you're right though that we don't need the | undefined. Can we address in another PR? I tried to avoid changing any of these types.

caridy · 2022-01-06T19:35:18Z

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

+}
+
+type assertInstanceOfHTMLElementFunc = (elm: any, msg: string) => void;
+export let assertInstanceOfHTMLElement: assertInstanceOfHTMLElementFunc;


It seem that this trick is enough to overcome the issue with TS, exporting something that is never set during initialization, but has a specific type makes it more difficult to TS to assume the problem of being undefined when used from another file, nice!

caridy

LGTM, added few notes...

packages/@lwc/engine-core/src/framework/base-lightning-element.ts

* perf: move renderer to top-level module * fix: add tsconfig so my IDE stops complaining * fix: use dependency injection pattern instead * fix: add missing header * test: fix test * test: fix test * fix: use "as const"` * fix: add comment

nolanlawson requested review from ekashida, pmdartus and divmain December 7, 2021 22:51

nolanlawson commented Dec 7, 2021

View reviewed changes

nolanlawson mentioned this pull request Dec 16, 2021

refactor(engine-core): POC splitting up diffing from rehydration #2608

Closed

nolanlawson force-pushed the nolan/hoist-renderer branch from 38b0c1c to faa3a31 Compare December 16, 2021 22:33

perf: move renderer to top-level module

2d548bd

nolanlawson force-pushed the nolan/hoist-renderer branch from faa3a31 to 2d548bd Compare January 3, 2022 23:55

nolanlawson requested a review from jodarove January 3, 2022 23:55

nolanlawson added 2 commits January 4, 2022 09:47

fix: add tsconfig so my IDE stops complaining

7c607b2

fix: use dependency injection pattern instead

53cb079

nolanlawson added 3 commits January 5, 2022 09:22

fix: add missing header

128c797

test: fix test

3a7654b

test: fix test

bb5d5ce

pmdartus reviewed Jan 6, 2022

View reviewed changes

fix: use "as const"`

234edbf

caridy reviewed Jan 6, 2022

View reviewed changes

caridy approved these changes Jan 6, 2022

View reviewed changes

fix: add comment

e47081e

pmdartus approved these changes Jan 10, 2022

View reviewed changes

packages/@lwc/engine-core/src/framework/base-lightning-element.ts Show resolved Hide resolved

nolanlawson merged commit 838035d into master Jan 10, 2022

nolanlawson deleted the nolan/hoist-renderer branch January 10, 2022 22:44

nolanlawson mentioned this pull request Jun 2, 2022

refactor(engine-core): encapsulate renderer as an object and allow it to be injectable in vnodes #2763

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: move renderer to top-level module #2598

perf: move renderer to top-level module #2598

nolanlawson commented Dec 7, 2021 •

edited

Loading

nolanlawson Dec 7, 2021

pmdartus Jan 6, 2022

nolanlawson Dec 7, 2021

nolanlawson Dec 7, 2021

nolanlawson Dec 7, 2021

nolanlawson Dec 7, 2021 •

edited

Loading

nolanlawson Dec 7, 2021

nolanlawson Dec 7, 2021

nolanlawson commented Jan 3, 2022

nolanlawson commented Jan 5, 2022

pmdartus Jan 6, 2022

nolanlawson Jan 6, 2022

nolanlawson Jan 6, 2022

pmdartus Jan 6, 2022

pmdartus Jan 6, 2022

caridy Jan 6, 2022

nolanlawson Jan 6, 2022

caridy Jan 6, 2022

nolanlawson Jan 6, 2022

caridy Jan 6, 2022

caridy left a comment

	addEventListener(
	target: E,
	type: string,
	callback: (event: Event) => any,
	options?: AddEventListenerOptions \| boolean
	): void;

	addEventListener(
	target: Node,
	type: string,
	callback: EventListener,
	options: AddEventListenerOptions \| boolean

Version	min	min+brotli
Baseline	49.6k	14.7k
Original PR	47.7	14.2k
Individual setters	48.2k	14.4k
One big setter	49.7k	14.8k

		// This is a temporary workaround to get the @lwc/engine-server to evaluate in node without having
		// to inject at runtime.

perf: move renderer to top-level module #2598

perf: move renderer to top-level module #2598

Conversation

nolanlawson commented Dec 7, 2021 • edited Loading

Details

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

nolanlawson Dec 7, 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 Jan 3, 2022

nolanlawson commented Jan 5, 2022

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

caridy left a comment

Choose a reason for hiding this comment

nolanlawson commented Dec 7, 2021 •

edited

Loading

nolanlawson Dec 7, 2021 •

edited

Loading