-
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: move renderer to top-level module #2598
Conversation
// to inject at runtime. | ||
export const HTMLElementConstructor: typeof HTMLElement = | ||
typeof HTMLElement !== 'undefined' ? HTMLElement : (function () {} as any); | ||
export const HTMLElementPrototype = HTMLElementConstructor.prototype; |
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.
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
.
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.
HTMLElement is already exposed by the renderer. It might be something we can refactor in a follow-up PR.
} | ||
return null; | ||
}, | ||
}, |
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.
I finally figured out how to replace packages in Rollup; I should apply this technique to perf-benchmarks
instead of using string replacement...
typecheckedRenderer = abstractRenderer; | ||
|
||
// eslint-disable-next-line no-console | ||
console.log(typecheckedRenderer); // use the variable so TypeScript doesn't complain |
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.
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.
export declare function getCustomElement(name: string): CustomElementConstructor | undefined; | ||
|
||
declare const HTMLElementInner: typeof HTMLElement; | ||
export { HTMLElementInner as HTMLElement }; |
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.
I could have used actual TypeScript here, but all I need is the .d.ts
file, so it seemed like overkill.
"publishConfig": { | ||
"access": "public" | ||
} | ||
} |
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.
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.
} else { | ||
element.removeAttributeNS(namespace, name); | ||
} | ||
} |
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.
The setAttribute
/removeAttribute
/getAttribute
APIs were inconsistent before:
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 { |
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.
options?: EventListenerOptions | boolean | ||
): void { | ||
target.removeEventListener(type, callback, options); | ||
} |
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.
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 |
38b0c1c
to
faa3a31
Compare
faa3a31
to
2d548bd
Compare
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. |
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 Bundle size difference (including the previous version of this PR):
Brotli-compressed:
The perf tests don't use the minified format, but they still show an improvement: |
// | ||
|
||
export let ssr: boolean; | ||
export function setSsr(ssrImpl: boolean) { |
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.
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.
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.
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.
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 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
Outdated
Show resolved
Hide resolved
// to inject at runtime. | ||
export const HTMLElementConstructor: typeof HTMLElement = | ||
typeof HTMLElement !== 'undefined' ? HTMLElement : (function () {} as any); | ||
export const HTMLElementPrototype = HTMLElementConstructor.prototype; |
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.
HTMLElement is already exposed by the renderer. It might be something we can refactor in a follow-up PR.
// This is a temporary workaround to get the @lwc/engine-server to evaluate in node without having | ||
// to inject at runtime. |
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 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.
@@ -51,3 +50,46 @@ export type { | |||
WireAdapterConstructor, | |||
WireAdapterSchemaValue, | |||
} from './wiring'; | |||
|
|||
export { |
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.
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?
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.
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.).
defineCustomElement = defineCustomElementImpl; | ||
} | ||
|
||
type getCustomElementFunc = (name: string) => CustomElementConstructor | undefined; |
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.
not sure why it is | undefined
, some weird stuff related to SSR?
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 was the pre-existing type:
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.
} | ||
|
||
type assertInstanceOfHTMLElementFunc = (elm: any, msg: string) => void; | ||
export let assertInstanceOfHTMLElement: assertInstanceOfHTMLElementFunc; |
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 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!
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.
LGTM, added few notes...
* 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
* 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
Details
Fixes #2569
Hoists
renderer.ts
into a top-level module so that it's no longer its own object. Avoids the need to passrenderer
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?
Does this pull request introduce 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
I doubt anyone is doing this though.