lib: define Event.isTrusted in the prototype by santigimeno · Pull Request #46974 · nodejs/node

santigimeno · 2023-03-06T14:47:26Z

We have been discussing in nodejs/performance#32 whether deviating from the spec with regards to Event.isTrusted would be acceptable for Node.js in order to avoid a significant performance hit. The spec defines the property as LegacyUnforgeable, but defining the property in the prototype instead of the constructor produces the following numbers:

./node benchmark/compare.js --new ./node --old ./node_old --runs 30 --filter messageport --filter eventtarget worker events  | Rscript benchmark/compare.R
[00:11:52|% 100| 3/3 files | 60/60 runs | 3/3 configs]: Done
                                                                       confidence improvement accuracy (*)   (**)   (***)
events/eventtarget-add-remove.js nListener=10 n=1000000                               0.25 %       ±1.31% ±1.75%  ±2.27%
events/eventtarget-add-remove.js nListener=5 n=1000000                               -1.05 %       ±1.29% ±1.71%  ±2.23%
events/eventtarget.js listeners=1 n=1000000                                  ***    177.72 %       ±3.72% ±5.00%  ±6.59%
events/eventtarget.js listeners=10 n=1000000                                 ***    100.93 %       ±7.30% ±9.83% ±13.04%
events/eventtarget.js listeners=5 n=1000000                                  ***    140.92 %       ±2.50% ±3.35%  ±4.41%
worker/messageport.js n=1000000 style='eventemitter' payload='object'         **     -2.88 %       ±1.93% ±2.57%  ±3.35%
worker/messageport.js n=1000000 style='eventemitter' payload='string'                -0.45 %       ±1.30% ±1.73%  ±2.25%
worker/messageport.js n=1000000 style='eventtarget' payload='object'         ***     25.68 %       ±2.27% ±3.03%  ±3.97%
worker/messageport.js n=1000000 style='eventtarget' payload='string'         ***     43.62 %       ±2.51% ±3.35%  ±4.39%

I'd like to have some feedback whether a change like this would be ok for Node.js (not being in the browser), specially from people more familiar with the standard. If that's the case I'll update the tests that are currently failing. Thanks!

lib/internal/event_target.js

cjihrig · 2023-03-06T16:15:42Z

Do we understand why the spec'ed implementation is so much slower? Is it something that V8 could improve?

santigimeno · 2023-03-06T17:35:10Z

Do we understand why the spec'ed implementation is so much slower? Is it something that V8 could improve?

I think https://bugs.chromium.org/p/v8/issues/detail?id=8447 might be relevant

bnoordhuis

Seems like a good tradeoff to me: obscure and largely irrelevant property vs. big performance jump.

Don't conform to the spec with isTrusted. The spec defines it as `LegacyUnforgeable` but defining it in the constructor has a big performance impact and the property doesn't seem to be useful outside of browsers. Refs: nodejs/performance#32

nodejs-github-bot · 2023-03-20T15:50:13Z

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

nodejs-github-bot · 2023-03-20T21:05:19Z

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

anonrig

🚀

nodejs-github-bot · 2023-03-29T18:05:13Z

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

nodejs-github-bot · 2023-03-31T11:07:23Z

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

nodejs-github-bot · 2023-03-31T16:24:13Z

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

jasnell · 2023-03-31T20:40:18Z

Given that we are intentionally deviating from the spec here, the docs should be updated to reflect that deviation

nodejs-github-bot · 2023-04-03T17:02:28Z

Landed in 74b9cf2

Don't conform to the spec with isTrusted. The spec defines it as `LegacyUnforgeable` but defining it in the constructor has a big performance impact and the property doesn't seem to be useful outside of browsers. Refs: nodejs/performance#32 PR-URL: #46974 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Don't conform to the spec with isTrusted. The spec defines it as `LegacyUnforgeable` but defining it in the constructor has a big performance impact and the property doesn't seem to be useful outside of browsers. Refs: nodejs/performance#32 PR-URL: nodejs#46974 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This PR optimizes `Event` constructor - ~Added a fast path for empty `eventInitDict`~ Removed `EventInit` dictionary converter - Don't make `isTrusted` a [LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable) property. Doing so makes it non-spec compliant but calling `Object/Reflect.defineProperty` on the constructor is a big bottleneck. Node did the same a few months ago nodejs/node#46974. In my opinion, the performance gains are worth deviating from the spec for a browser-related property. **This PR** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 36.69 ns/iter 27,257,504.6 (33.36 ns … 42.45 ns) 37.71 ns 39.61 ns 40.07 ns event constructor 36.7 ns/iter 27,246,776.6 (33.35 ns … 56.03 ns) 37.73 ns 40.14 ns 41.74 ns ``` **main** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 380.48 ns/iter 2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns event constructor 480.33 ns/iter 2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns ``` ```js Deno.bench("event constructor no init", () => { const event = new Event("foo"); }); Deno.bench("event constructor", () => { const event = new Event("foo", { bubbles: true, cancelable: false }); }); ``` towards #20167

This PR optimizes `Event` constructor - ~Added a fast path for empty `eventInitDict`~ Removed `EventInit` dictionary converter - Don't make `isTrusted` a [LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable) property. Doing so makes it non-spec compliant but calling `Object/Reflect.defineProperty` on the constructor is a big bottleneck. Node did the same a few months ago nodejs/node#46974. In my opinion, the performance gains are worth deviating from the spec for a browser-related property. **This PR** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 36.69 ns/iter 27,257,504.6 (33.36 ns … 42.45 ns) 37.71 ns 39.61 ns 40.07 ns event constructor 36.7 ns/iter 27,246,776.6 (33.35 ns … 56.03 ns) 37.73 ns 40.14 ns 41.74 ns ``` **main** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 380.48 ns/iter 2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns event constructor 480.33 ns/iter 2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns ``` ```js Deno.bench("event constructor no init", () => { const event = new Event("foo"); }); Deno.bench("event constructor", () => { const event = new Event("foo", { bubbles: true, cancelable: false }); }); ``` towards denoland#20167

This PR optimizes `Event` constructor - ~Added a fast path for empty `eventInitDict`~ Removed `EventInit` dictionary converter - Don't make `isTrusted` a [LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable) property. Doing so makes it non-spec compliant but calling `Object/Reflect.defineProperty` on the constructor is a big bottleneck. Node did the same a few months ago nodejs/node#46974. In my opinion, the performance gains are worth deviating from the spec for a browser-related property. **This PR** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 36.69 ns/iter 27,257,504.6 (33.36 ns … 42.45 ns) 37.71 ns 39.61 ns 40.07 ns event constructor 36.7 ns/iter 27,246,776.6 (33.35 ns … 56.03 ns) 37.73 ns 40.14 ns 41.74 ns ``` **main** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 380.48 ns/iter 2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns event constructor 480.33 ns/iter 2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns ``` ```js Deno.bench("event constructor no init", () => { const event = new Event("foo"); }); Deno.bench("event constructor", () => { const event = new Event("foo", { bubbles: true, cancelable: false }); }); ``` towards #20167

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

santigimeno mentioned this pull request Mar 6, 2023

EventTarget nodejs/performance#32

Closed

anonrig reviewed Mar 6, 2023

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved

santigimeno force-pushed the santi/is_trusted_proto branch from 9235d8d to 6cf3af6 Compare March 6, 2023 15:31

anonrig approved these changes Mar 6, 2023

View reviewed changes

bnoordhuis approved these changes Mar 7, 2023

View reviewed changes

lib: define Event.isTrusted in the prototype

f7c80d9

Don't conform to the spec with isTrusted. The spec defines it as `LegacyUnforgeable` but defining it in the constructor has a big performance impact and the property doesn't seem to be useful outside of browsers. Refs: nodejs/performance#32

santigimeno force-pushed the santi/is_trusted_proto branch from 6cf3af6 to f7c80d9 Compare March 20, 2023 15:21

santigimeno marked this pull request as ready for review March 20, 2023 15:22

santigimeno changed the title ~~RFC: define Event.isTrusted in the prototype~~ lib: define Event.isTrusted in the prototype Mar 20, 2023

santigimeno added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023

anonrig added the performance Issues and PRs related to the performance of Node.js. label Mar 20, 2023

anonrig approved these changes Mar 20, 2023

View reviewed changes

anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 20, 2023

This was referenced Mar 21, 2023

CI Reliability 2023-03-21 nodejs/reliability#531

Open

CI Reliability 2023-03-22 nodejs/reliability#532

Open

CI Reliability 2023-03-23 nodejs/reliability#533

Open

This was referenced Mar 30, 2023

CI Reliability 2023-03-30 nodejs/reliability#534

Open

CI Reliability 2023-03-31 nodejs/reliability#535

Open

jasnell approved these changes Mar 31, 2023

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2023

nodejs-github-bot merged commit 74b9cf2 into nodejs:main Apr 3, 2023

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

marcosc90 mentioned this pull request Aug 16, 2023

perf(ext/event): optimize Event constructor denoland/deno#20181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lib: define Event.isTrusted in the prototype#46974

lib: define Event.isTrusted in the prototype#46974
nodejs-github-bot merged 1 commit intonodejs:mainfrom
santigimeno:santi/is_trusted_proto

santigimeno commented Mar 6, 2023

Uh oh!

Uh oh!

cjihrig commented Mar 6, 2023

Uh oh!

santigimeno commented Mar 6, 2023

Uh oh!

bnoordhuis left a comment

Uh oh!

nodejs-github-bot commented Mar 20, 2023

Uh oh!

nodejs-github-bot commented Mar 20, 2023

Uh oh!

anonrig left a comment

Uh oh!

nodejs-github-bot commented Mar 29, 2023

Uh oh!

nodejs-github-bot commented Mar 31, 2023

Uh oh!

nodejs-github-bot commented Mar 31, 2023

Uh oh!

jasnell commented Mar 31, 2023

Uh oh!

nodejs-github-bot commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

santigimeno commented Mar 6, 2023

Uh oh!

Uh oh!

cjihrig commented Mar 6, 2023

Uh oh!

santigimeno commented Mar 6, 2023

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 20, 2023

Uh oh!

nodejs-github-bot commented Mar 20, 2023

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 29, 2023

Uh oh!

nodejs-github-bot commented Mar 31, 2023

Uh oh!

nodejs-github-bot commented Mar 31, 2023

Uh oh!

jasnell commented Mar 31, 2023

Uh oh!

nodejs-github-bot commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants