Skip to content
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

Benchmark + optimize #1096

Merged
merged 12 commits into from
Sep 28, 2021
Merged

Benchmark + optimize #1096

merged 12 commits into from
Sep 28, 2021

Conversation

samouri
Copy link
Member

@samouri samouri commented Sep 23, 2021

summary

Creates a super basic benchmark.mjs file for measuring createElement performance.
Then makes 4 small changes that each remove memory leaks and/or improve ctor performance

initial performance

createElement x 197,202 ops/sec ±5.21% (72 runs sampled)

opt: skip HTMLElement ctor store

createElement x 449,453 ops/sec ±2.38% (83 runs sampled)

opt: getter for CSSStyleDeclaration (lazy instantiation)

createElement x 626,866 ops/sec ±2.26% (85 runs sampled)

opt: class field loose transform

createElement x 643,255 ops/sec ±2.16% (86 runs sampled)

opt: remove minification step (note: I don't know why this has faster output)

createElement x 726,293 ops/sec ±0.77% (91 runs sampled)

cc @rcebulko

@samouri samouri changed the title Bench Benchmark + optimize Sep 23, 2021
@samouri samouri self-assigned this Sep 23, 2021
@@ -62,3 +62,4 @@ type RenderableElement = (HTMLElement | SVGElement | Text | Comment) & {

declare const WORKER_DOM_DEBUG: boolean;
declare const IS_AMP: boolean;
declare const IS_SERVER: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚲 🏚️ :

Suggested change
declare const IS_SERVER: boolean;
declare const DISABLE_TRANSFERS: boolean;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats true for now. But the higher level notion is that it is meant for usage on server instead of client

Copy link
Member Author

@samouri samouri Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of the new thing I did, which is kind of nice IMO. Attached to process.env.SERVER.
Edit: actually..i'm not sure if this works in client at all. testing now
Edit Edit: it works when I always provide the subsitutions

@samouri samouri marked this pull request as ready for review September 27, 2021 22:22
},
preventAssignment: true,
}),
replacePlugin({ debug: true }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the debug: true supposed to be included here? Maybe based on a flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is configuration for the output file: dist/debug/main.mjs (scroll up a few lines to see that).
For this binary we want all of the debug code. Very similar to amphtml's mode

@samouri samouri merged commit b9ffedd into ampproject:main Sep 28, 2021
@samouri samouri deleted the bench branch September 28, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants