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

joyeecheung · 2022-03-16T08:17:34Z

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.

joyeecheung · 2022-03-16T08:23:17Z

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/

joyeecheung · 2022-03-16T10:21:30Z

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%

lib/internal/perf/observe.js

nodejs-github-bot · 2022-03-18T11:12:28Z

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

nodejs-github-bot · 2022-03-19T12:34:41Z

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

nodejs-github-bot · 2022-03-22T14:19:52Z

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

joyeecheung · 2022-03-23T12:04:45Z

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

joyeecheung · 2022-03-23T12:08:40Z

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>

juanarbol · 2022-04-06T16:07:18Z

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>

danielleadams · 2022-06-26T18:39:42Z

@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

lib/internal/perf/observe.js Show resolved Hide resolved

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 commented Mar 16, 2022 •

edited

Loading

joyeecheung commented Mar 16, 2022 •

edited

Loading

joyeecheung commented Mar 16, 2022

nodejs-github-bot commented Mar 18, 2022

nodejs-github-bot commented Mar 19, 2022

nodejs-github-bot commented Mar 22, 2022

joyeecheung commented Mar 23, 2022

joyeecheung commented Mar 23, 2022

juanarbol commented Apr 6, 2022

danielleadams commented Jun 26, 2022

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

Conversation

joyeecheung commented Mar 16, 2022 • edited Loading

joyeecheung commented Mar 16, 2022 • edited Loading

joyeecheung commented Mar 16, 2022

nodejs-github-bot commented Mar 18, 2022

nodejs-github-bot commented Mar 19, 2022

nodejs-github-bot commented Mar 22, 2022

joyeecheung commented Mar 23, 2022

joyeecheung commented Mar 23, 2022

juanarbol commented Apr 6, 2022

danielleadams commented Jun 26, 2022

joyeecheung commented Mar 16, 2022 •

edited

Loading

joyeecheung commented Mar 16, 2022 •

edited

Loading