lib: use class fields in classes that revert to symbol properties due to missing snapshot support by joyeecheung · Pull Request #42361 · nodejs/node · GitHub

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already
fixed, so switch back to class fields instead of using symbol
properties.

Side note: class field initialization performance has been optimized since v8 9.7 so this shouldn't cause performance regressions either.

Refs: b1c3909

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, but since AbortController currently throws ERR_INVALID_THIS we'll revert to class fields in a subsequent patch. For now just update the comments.

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties.

events benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1113/
perf_hooks benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1112/

Benchmark results:

(**)   (***)
17:44:13                                                                  confidence improvement accuracy (*)   (**)   (***)
17:44:14 events/ee-add-remove.js n=1000000 removeListener=0 newListener=0                 0.32 %       ±3.75% ±4.99%  ±6.49%
17:44:14 events/ee-add-remove.js n=1000000 removeListener=0 newListener=1                -0.13 %       ±2.93% ±3.91%  ±5.11%
17:44:14 events/ee-add-remove.js n=1000000 removeListener=1 newListener=0                -0.05 %       ±2.80% ±3.73%  ±4.87%
17:44:14 events/ee-add-remove.js n=1000000 removeListener=1 newListener=1                 0.47 %       ±2.52% ±3.35%  ±4.37%
17:44:14 events/ee-emit.js listeners=10 argc=0 n=2000000                                  0.06 %       ±1.19% ±1.58%  ±2.06%
17:44:14 events/ee-emit.js listeners=10 argc=10 n=2000000                                 0.79 %       ±1.12% ±1.49%  ±1.94%
17:44:14 events/ee-emit.js listeners=10 argc=2 n=2000000                                  0.24 %       ±3.36% ±4.51%  ±5.94%
17:44:14 events/ee-emit.js listeners=10 argc=4 n=2000000                                  0.75 %       ±0.96% ±1.28%  ±1.67%
17:44:14 events/ee-emit.js listeners=1 argc=0 n=2000000                                  -2.49 %       ±4.42% ±5.88%  ±7.66%
17:44:14 events/ee-emit.js listeners=1 argc=10 n=2000000                                 -2.14 %       ±5.22% ±6.95%  ±9.04%
17:44:14 events/ee-emit.js listeners=1 argc=2 n=2000000                                  -0.49 %       ±5.22% ±6.94%  ±9.04%
17:44:14 events/ee-emit.js listeners=1 argc=4 n=2000000                                   0.88 %       ±4.58% ±6.10%  ±7.95%
17:44:14 events/ee-emit.js listeners=5 argc=0 n=2000000                                   1.63 %       ±1.92% ±2.56%  ±3.34%
17:44:14 events/ee-emit.js listeners=5 argc=10 n=2000000                                 -2.00 %       ±2.04% ±2.72%  ±3.56%
17:44:14 events/ee-emit.js listeners=5 argc=2 n=2000000                                  -1.71 %       ±2.48% ±3.30%  ±4.30%
17:44:14 events/ee-emit.js listeners=5 argc=4 n=2000000                                   1.16 %       ±2.26% ±3.01%  ±3.93%
17:44:14 events/ee-listener-count-on-prototype.js n=50000000                              1.47 %       ±6.42% ±8.56% ±11.20%
17:44:14 events/ee-listeners.js raw='false' listeners=50 n=5000000                       -1.83 %       ±2.15% ±2.87%  ±3.73%
17:44:14 events/ee-listeners.js raw='false' listeners=5 n=5000000                         1.47 %       ±3.16% ±4.24%  ±5.58%
17:44:14 events/ee-listeners.js raw='true' listeners=50 n=5000000                         0.12 %       ±1.45% ±1.92%  ±2.50%
17:44:14 events/ee-listeners.js raw='true' listeners=5 n=5000000                          0.14 %       ±4.28% ±5.70%  ±7.42%
17:44:14 events/ee-once.js argc=0 n=20000000                                             -0.03 %       ±1.08% ±1.44%  ±1.88%
17:44:14 events/ee-once.js argc=1 n=20000000                                             -1.32 %       ±2.35% ±3.14%  ±4.10%
17:44:14 events/ee-once.js argc=4 n=20000000                                              0.52 %       ±3.17% ±4.22%  ±5.51%
17:44:14 events/ee-once.js argc=5 n=20000000                                             -0.65 %       ±1.21% ±1.62%  ±2.12%
17:44:14 events/eventtarget.js listeners=10 n=1000000                                     2.88 %       ±4.68% ±6.27%  ±8.25%
17:44:14 events/eventtarget.js listeners=1 n=1000000                                      0.02 %       ±1.24% ±1.66%  ±2.18%
17:44:14 events/eventtarget.js listeners=5 n=1000000                                     -2.28 %       ±4.90% ±6.55%  ±8.60%

16:48:38                                                                 confidence improvement accuracy (*)   (**)  (***)
16:48:39 perf_hooks/bench-eventlooputil.js method='ELU_passed' n=1000000                -1.10 %       ±2.38% ±3.16% ±4.12%
16:48:39 perf_hooks/bench-eventlooputil.js method='ELU_simple' n=1000000                -0.33 %       ±2.89% ±3.85% ±5.01%
16:48:39 perf_hooks/bench-eventlooputil.js method='idleTime' n=1000000                   1.77 %       ±5.49% ±7.31% ±9.52%
16:48:39 perf_hooks/usertiming.js observe='all' n=100000                                 1.34 %       ±4.05% ±5.41% ±7.08%
16:48:39 perf_hooks/usertiming.js observe='measure' n=100000                             2.00 %       ±2.93% ±3.91% ±5.08%

CI: https://ci.nodejs.org/job/node-test-pull-request/43113/

CI: https://ci.nodejs.org/job/node-test-pull-request/43127/

CI: https://ci.nodejs.org/job/node-test-pull-request/43165/

Last CI: https://ci.nodejs.org/job/node-test-pull-request/43171/

Landed in 9680d7d...c37fdac

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, but since AbortController currently throws ERR_INVALID_THIS we'll revert to class fields in a subsequent patch. For now just update the comments. PR-URL: #42361 Refs: b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties. PR-URL: #42361 Refs: b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, but since AbortController currently throws ERR_INVALID_THIS we'll revert to class fields in a subsequent patch. For now just update the comments. PR-URL: #42361 Refs: b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties. PR-URL: #42361 Refs: b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

The V8 version in v17.x does not support private members in "Snapshot"

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, but since AbortController currently throws ERR_INVALID_THIS we'll revert to class fields in a subsequent patch. For now just update the comments. PR-URL: nodejs#42361 Refs: nodejs@b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties. PR-URL: nodejs#42361 Refs: nodejs@b1c3909 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

@joyeecheung should this be pulled into v16.x? If so, do you mind creating a backport PR into v16.x-staging as it breaks the test suite. If it does not, then we can mark it as dont-land-in-v16.x.

joyeecheung added 3 commits March 16, 2022 16:12

lib: update class fields TODO in abort_controller.js

c8c6f0c

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, but since AbortController currently throws ERR_INVALID_THIS we'll revert to class fields in a subsequent patch. For now just update the comments.

lib: use class fields in Event and EventTarget

3e7e7ed

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties.

lib: use class fields in observe.js

ccbb05f

https://bugs.chromium.org/p/v8/issues/detail?id=10704 is already fixed, so switch back to class fields instead of using symbol properties.

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 16, 2022

targos approved these changes Mar 16, 2022

View reviewed changes

aduh95 reviewed Mar 16, 2022

View reviewed changes

Comment thread lib/internal/perf/observe.js

joyeecheung mentioned this pull request Mar 16, 2022

Node.js Technical Steering Committee (TSC) Meeting 2022-03-16 nodejs/TSC#1186

Closed

benjamingr approved these changes Mar 17, 2022

View reviewed changes

lpinca approved these changes Mar 17, 2022

View reviewed changes

aduh95 approved these changes Mar 18, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 18, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2022

Mesteery approved these changes Mar 19, 2022

View reviewed changes

RaisinTen approved these changes Mar 21, 2022

View reviewed changes

github-actions Bot mentioned this pull request Mar 23, 2022

CI Reliability 2022-03-23 nodejs/reliability#229

Open

28 tasks

joyeecheung closed this Mar 23, 2022

This was referenced Mar 24, 2022

CI Reliability 2022-03-24 nodejs/reliability#230

Open

CI Reliability 2022-03-25 nodejs/reliability#231

Open

CI Reliability 2022-03-26 nodejs/reliability#232

Open

juanarbol added the dont-land-on-v17.x label Apr 6, 2022

BethGriggs mentioned this pull request Apr 9, 2022

2022-04-19, Version 18.0.0 (Current) #42262

Merged

danielleadams added the backport-requested-v16.x label Jun 26, 2022

targos added dont-land-on-v16.x and removed backport-requested-v16.x labels Sep 16, 2022

lib: use class fields in classes that revert to symbol properties due to missing snapshot support by joyeecheung · Pull Request #42361 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: use class fields in classes that revert to symbol properties due to missing snapshot support - #42361

lib: use class fields in classes that revert to symbol properties due to missing snapshot support#42361
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:class-fields

joyeecheung commented Mar 16, 2022 •
edited
Loading

Uh oh!

Uh oh!

joyeecheung commented Mar 16, 2022 •
edited
Loading

Uh oh!

Uh oh!

joyeecheung commented Mar 16, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 18, 2022

Uh oh!

nodejs-github-bot commented Mar 19, 2022

Uh oh!

nodejs-github-bot commented Mar 22, 2022

Uh oh!

joyeecheung commented Mar 23, 2022

Uh oh!

joyeecheung commented Mar 23, 2022

Uh oh!

juanarbol commented Apr 6, 2022

Uh oh!

danielleadams commented Jun 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lib: use class fields in classes that revert to symbol properties due to missing snapshot support by joyeecheung · Pull Request #42361 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: use class fields in classes that revert to symbol properties due to missing snapshot support - #42361

lib: use class fields in classes that revert to symbol properties due to missing snapshot support#42361joyeecheung wants to merge 3 commits intonodejs:masternodejs/node:masterfrom joyeecheung:class-fieldsjoyeecheung/node:class-fieldsCopy head branch name to clipboard

Conversation

joyeecheung commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Mar 16, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 18, 2022

Uh oh!

nodejs-github-bot commented Mar 19, 2022

Uh oh!

nodejs-github-bot commented Mar 22, 2022

Uh oh!

joyeecheung commented Mar 23, 2022

Uh oh!

joyeecheung commented Mar 23, 2022

Uh oh!

juanarbol commented Apr 6, 2022

Uh oh!

danielleadams commented Jun 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lib: use class fields in classes that revert to symbol properties due to missing snapshot support#42361
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:class-fields

joyeecheung commented Mar 16, 2022 •
edited
Loading

joyeecheung commented Mar 16, 2022 •
edited
Loading