Skip to content

feat: Use WeakMap for wrapper to impl conversion#155

Closed
ExE-Boss wants to merge 2 commits intojsdom:masterfrom
ExE-Boss:feat/use-weakmap-for-wrapper-to-impl-mapping
Closed

feat: Use WeakMap for wrapper to impl conversion#155
ExE-Boss wants to merge 2 commits intojsdom:masterfrom
ExE-Boss:feat/use-weakmap-for-wrapper-to-impl-mapping

Conversation

@ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 15, 2020 •
edited
Loading

Currently, wrapper → impl mapping is done using a symbol.

The issue with this approach is that it causes several observable [[Get]]s on the object, which causes some WPT failures, and can lead to implementation leakage, as described in the class fields proposal.

@ExE-Boss ExE-Boss requested review from TimothyGu, domenic and pmdartus and removed request for TimothyGu and pmdartus January 15, 2020 08:58
@ExE-Boss ExE-Boss mentioned this pull request Jan 15, 2020
Merged
@domenic
Copy link
Member

domenic commented Jan 15, 2020

Thanks; I've been interested in doing this for some time. But, I'm concerned about the performance hit. Can you post the results of the jsdom benchmark suite before and after?

@ExE-Boss ExE-Boss force-pushed the feat/use-weakmap-for-wrapper-to-impl-mapping branch from eb83b86 to 28823db Compare January 16, 2020 05:31
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 16, 2020

Well, here’s the benchmark result for Node.js 13.5.0 on my PC:

benchmark symbol WeakMap
dom/compare-document-position/compare ancestor jsdom × 33,877 ops/sec ±2.88% (14 runs sampled) jsdom × 30,475 ops/sec ±3.29% (11 runs sampled)
dom/compare-document-position/compare descendant jsdom × 34,320 ops/sec ±2.22% (14 runs sampled) jsdom × 24,739 ops/sec ±41.56% (11 runs sampled)
dom/compare-document-position/compare siblings jsdom × 1,316,711 ops/sec ±3.36% (9 runs sampled) jsdom × 152,480 ops/sec ±196.14% (6 runs sampled)
dom/construction/createComment jsdom × 1,235,394 ops/sec ±4.44% (92 runs sampled) jsdom × 45,629 ops/sec ±60.45% (60 runs sampled)
dom/construction/createDocumentFragment jsdom × 1,270,944 ops/sec ±0.41% (97 runs sampled) jsdom × 295,794 ops/sec ±29.02% (49 runs sampled)
dom/construction/createElement jsdom × 221,507 ops/sec ±2.86% (87 runs sampled) jsdom × 74,368 ops/sec ±31.28% (73 runs sampled)
dom/construction/createEvent jsdom × 191,764 ops/sec ±1.94% (92 runs sampled) jsdom × 150,126 ops/sec ±1.37% (93 runs sampled)
dom/construction/createNodeIterator jsdom × 1,247,649 ops/sec ±0.28% (94 runs sampled) jsdom × 101,389 ops/sec ±52.98% (17 runs sampled)
dom/construction/createProcessingInstruction jsdom × 627,401 ops/sec ±0.32% (95 runs sampled) jsdom × 110,947 ops/sec ±41.93% (74 runs sampled)
dom/construction/createTextNode jsdom × 973,704 ops/sec ±0.49% (99 runs sampled) jsdom × 320,028 ops/sec ±22.86% (79 runs sampled)
dom/inner-html/tables jsdom × 0.33 ops/sec ±1.98% (5 runs sampled) jsdom × 0.28 ops/sec ±1.72% (5 runs sampled)
dom/named-properties/setAttribute(): Remove a named property from window jsdom × 2,904 ops/sec ±4.53% (56 runs sampled) jsdom × 1,596 ops/sec ±42.04% (29 runs sampled)
dom/tree-modification/appendChild: many parents jsdom × 28.77 ops/sec ±0.84% (49 runs sampled) jsdom × 27.18 ops/sec ±0.85% (45 runs sampled)
dom/tree-modification/appendChild: many siblings jsdom × 245,236 ops/sec ±13.16% (16 runs sampled) jsdom × 83,316 ops/sec ±80.33% (8 runs sampled)
dom/tree-modification/appendChild: no siblings jsdom × 217,962 ops/sec ±11.74% (21 runs sampled) jsdom × 45,244 ops/sec ±69.13% (8 runs sampled)
dom/tree-modification/insertBefore: many siblings jsdom × 236,425 ops/sec ±12.12% (15 runs sampled) jsdom × 42,729 ops/sec ±146.72% (5 runs sampled)
dom/tree-modification/removeChild: many parents jsdom × 122 ops/sec ±1.07% (64 runs sampled) jsdom × 121 ops/sec ±0.99% (61 runs sampled)
dom/tree-modification/removeChild: many siblings jsdom × 133,626 ops/sec ±7.42% (35 runs sampled) jsdom × 36,757 ops/sec ±33.78% (9 runs sampled)
dom/tree-modification/removeChild: no siblings jsdom × 118,199 ops/sec ±11.44% (23 runs sampled) jsdom × 20,775 ops/sec ±4.47% (15 runs sampled)
html/parsing/text jsdom × 14.23 ops/sec ±1.51% (40 runs sampled) jsdom × 13.11 ops/sec ±28.91% (43 runs sampled)
jsdom/api/new JSDOM() defaults <Test #⁠10> × 261 ops/sec ±4.49% (73 runs sampled) <Test #⁠10> × 255 ops/sec ±15.80% (74 runs sampled)
jsdom/api/new JSDOM() with many elements <Test #⁠11> × 27.18 ops/sec ±14.82% (51 runs sampled) <Test #⁠11> × 18.54 ops/sec ±21.99% (38 runs sampled)

@TimothyGu
Copy link
Member

TimothyGu commented Jan 16, 2020

Could you also benchmark running esmarkup over the ECMA-262 source with/without this patch? (See command for doing so.)

@Sebmaster
Copy link
Member

Sebmaster commented Jan 16, 2020

The large variability combined with the giant slowdown in the create* tests makes me kinda wary of this. Are we doing something weird in createNode that taxes on WeakMaps so much?

@pmdartus
Copy link
Member

pmdartus commented Jan 18, 2020 •
edited by domenic
Loading

I ran the benchmark locally and I was able to measure a noticeable slowdown locally. All the interface creation tests are 3x slower using WeakMap instead of a Symbol. You can see below the benchmark results I gathered locally on Node 12.13.1. I also ran the same benchmark against Node 13.6.0, and the results are comparable.

The variability of the benchmark results might be explained by 2 things:

  • with the overall slowdown, benchmark.js uses fewer samples for each benchmark.
  • looking at a couple of CPU profiles, we can see that each GC takes way longer in the WeakMap version compared to the Symbol one.

Based on the benchmark data, I feel uncomfortable introducing such a large regression compared to the value-added.

Benchmark result on Macbook Pro 2015 / Node 12.13.1
Test name Symbol mapping WeakMap mapping
dom/compare-document-position/compare ancestor 18,624 ops/sec ±12.65% 18,319 ops/sec ±2.45%
dom/compare-document-position/compare descendant 21,323 ops/sec ±6.25% 16,507 ops/sec ±6.04%
dom/compare-document-position/compare siblings 1,002,885 ops/sec ±26.70% 503,312 ops/sec ±8.50%
dom/construction/createComment 1,290,169 ops/sec ±1.27% 263,351 ops/sec ±26.75%
dom/construction/createDocumentFragment 1,377,951 ops/sec ±0.63% 307,647 ops/sec ±20.64%
dom/construction/createElement 193,831 ops/sec ±5.56% 43,275 ops/sec ±38.74%
dom/construction/createEvent 212,986 ops/sec ±4.40% 25,691 ops/sec ±21.27%
dom/construction/createNodeIterator 1,156,065 ops/sec ±0.44% 317,913 ops/sec ±26.28%
dom/construction/createProcessingInstruction 540,759 ops/sec ±0.93% 33,108 ops/sec ±3.30%
dom/construction/createTextNode 962,896 ops/sec ±0.66% 32,521 ops/sec ±3.05%
dom/inner-html/tables 0.31 ops/sec ±3.88% 0.11 ops/sec ±96.57%
dom/named-properties/setAttribute 3,124 ops/sec ±12.53% 2,523 ops/sec ±1.85%
dom/tree-modification/appendChild: many parents 21.95 ops/sec ±0.93% 20.36 ops/sec ±1.89%
dom/tree-modification/appendChild: many siblings 254,109 ops/sec ±17.94% 35,003 ops/sec ±140.39%
dom/tree-modification/appendChild: no siblings 238,119 ops/sec ±19.03% 36,006 ops/sec ±105.56%
dom/tree-modification/insertBefore: many siblings 194,787 ops/sec ±22.13% 29,815 ops/sec ±89.69%
dom/tree-modification/removeChild: many parents 113 ops/sec ±16.32% 74.89 ops/sec ±1.28%
dom/tree-modification/removeChild: many siblings 96,538 ops/sec ±12.43% 21,796 ops/sec ±23.13%
dom/tree-modification/removeChild: no siblings 79,519 ops/sec ±24.92% 26,069 ops/sec ±43.54%
html/parsing/text 13.26 ops/sec ±1.86% 0.76 ops/sec ±261.72%
jsdom/api/new JSDOM() defaults 212 ops/sec ±19.43% 206 ops/sec ±27.32%
jsdom/api/new JSDOM() with many elements 18.81 ops/sec ±22.14% 15.18 ops/sec ±26.72%

@pmdartus pmdartus mentioned this pull request Jan 18, 2020
Open
@domenic
Copy link
Member

domenic commented Jan 18, 2020

I agree; this is not worth the cost. So, let's close.

@domenic domenic closed this Jan 18, 2020
@TimothyGu TimothyGu deleted the feat/use-weakmap-for-wrapper-to-impl-mapping branch January 19, 2020 04:33
@TimothyGu TimothyGu restored the feat/use-weakmap-for-wrapper-to-impl-mapping branch January 19, 2020 04:33
@domenic domenic mentioned this pull request Feb 16, 2020
Closed
@domenic domenic mentioned this pull request Nov 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@domenic domenic Awaiting requested review from domenic

@pmdartus pmdartus Awaiting requested review from pmdartus

@TimothyGu TimothyGu Awaiting requested review from TimothyGu

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ExE-Boss @domenic @TimothyGu @Sebmaster @pmdartus