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

Lazy wrapper instantiation #158

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pmdartus
Copy link
Member

Changes

This PR is an early draft for lazy wrapper instantiation. Instead of eagerly create a wrapper for each new impl, the wrapper is only created when accessed by userland code.

While the results are REALLY encouraging, there are still a couple of test failures in jsdom (mainly related to events). Running this change against jsdom performance benchmark we can measure a 44% reduction in memory consumption and a 20% to 200% performance improvement across all the tests. More details in the results section.

Fixes #157

Results

Memory

Memory (MB) p50 p75 p90 p95
Baseline 1328.97 1560.35 1669.04 1726.82
Lazy 922.80 1126.09 1315.43 1392.72
Delta -44.01% -38.56% -26.88% -23.99%

Benchmark Performance

Benchmark Baseline (ops/sec) Lazy wrapper (ops/sec) Detla
dom/compare-document-position/compare ancestor 18,986.00 18,440 2.88%
dom/compare-document-position/compare descendant 18,110.00 19,403 -7.14%
dom/compare-document-position/compare siblings 1,107,701.00 1,120,706 -1.17%
dom/construction/createComment 1,276,052.00 2,263,382 -77.37%
dom/construction/createDocumentFragment 1,410,648.00 2,188,373 -55.13%
dom/construction/createElement 192,790.00 646,840 -235.52%
dom/construction/createEvent 215,484.00 209,256 2.89%
dom/construction/createNodeIterator 1,217,542.00 1,747,421 -43.52%
dom/construction/createProcessingInstruction 540,648.00 903,657 -67.14%
dom/construction/createTextNode 991,920.00 1,732,976 -74.71%
dom/inner-html/tables 0.31 0.39 -25.81%
dom/named-properties/setAttribute(): Remove a named property from window 3,049.00 3,591 -17.78%
dom/tree-modification/appendChild: many parents 22.03 22.69 -3.00%
dom/tree-modification/appendChild: many siblings 240,373.00 298,791 -24.30%
dom/tree-modification/appendChild: no siblings 234,210.00 262,268 -11.98%
dom/tree-modification/insertBefore: many siblings 208,446.00 264,970 -27.12%
dom/tree-modification/removeChild: many parents 116.00 130 -12.07%
dom/tree-modification/removeChild: many siblings 116,963.00 133,050 -13.75%
dom/tree-modification/removeChild: no siblings 111,543.00 129,702 -16.28%
html/parsing/text 13.10 13.88 -5.95%
jsdom/api/new JSDOM() defaults 240.00 256 -6.67%
jsdom/api/new JSDOM() with many elements 26.51 33.57 -26.63%

@domenic
Copy link
Member

domenic commented Jan 20, 2020

Amazing!!

Would you be able to remove the impl -> implSymbol renaming for now, or maybe do it as a separate commit? That would make review much easier (including of the generated code).

@pmdartus
Copy link
Member Author

Yes, I will do that soon. I was just too excited to share those numbers.

value: new Impl.implementation(globalObject, constructorArgs, privateData),
configurable: true
});
implObject[wrapperSymbol] = null;
Copy link
Contributor

@ExE-Boss ExE-Boss Feb 10, 2020

Choose a reason for hiding this comment

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

Why not use a getter that turns into a data property?

That will remove the need for createWrapperSymbol and globalObjectSymbol.

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.

Wrapper lazy instantiation
3 participants