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.

Sign up for GitHub

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

Jump to bottom

lib: define Event.isTrusted in the prototype #46974

Merged
merged 1 commit into from
Apr 3, 2023
Merged

lib: define Event.isTrusted in the prototype #46974

merged 1 commit into from
Apr 3, 2023

Conversation

santigimeno
Copy link
Member

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!

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 6, 2023
@santigimeno santigimeno mentioned this pull request Mar 6, 2023
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Mar 6, 2023

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

@santigimeno
Copy link
Member Author

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
bnoordhuis approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@santigimeno
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 santigimeno marked this pull request as ready for review March 20, 2023 15:22
@santigimeno santigimeno changed the title RFC: define Event.isTrusted in the prototype lib: define Event.isTrusted in the prototype Mar 20, 2023
@santigimeno santigimeno added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Mar 20, 2023
@nodejs-github-bot
Copy link
Collaborator

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

anonrig
anonrig approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

🚀

@anonrig 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
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Mar 30, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Mar 31, 2023

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

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 74b9cf2 into nodejs:main Apr 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 74b9cf2

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@santigimeno @RafaelGSS
lib: define Event.isTrusted in the prototype
c6b5ee7 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@santigimeno @RafaelGSS
lib: define Event.isTrusted in the prototype
412e615 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
@santigimeno @RafaelGSS
lib: define Event.isTrusted in the prototype
a540dcd 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
@santigimeno @RafaelGSS
lib: define Event.isTrusted in the prototype
a8e3a7c 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
@santigimeno @RafaelGSS
lib: define Event.isTrusted in the prototype
a738164 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@santigimeno @danielleadams
lib: define Event.isTrusted in the prototype
0ed52b4 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@santigimeno @MoLow
lib: define Event.isTrusted in the prototype
33f32dc 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
@marcosc90 marcosc90 mentioned this pull request Aug 16, 2023
Merged
bartlomieju pushed a commit to denoland/deno that referenced this pull request Aug 17, 2023
@marcosc90
perf(ext/event): optimize Event constructor (#20181)
ec63b36 
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
littledivy pushed a commit to littledivy/deno that referenced this pull request Aug 21, 2023
@marcosc90 @littledivy
perf(ext/event): optimize Event constructor (denoland#20181)
3552ba3 
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
littledivy pushed a commit to denoland/deno that referenced this pull request Aug 21, 2023
@marcosc90 @littledivy
perf(ext/event): optimize Event constructor (#20181)
3b4efd3 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@santigimeno @cjihrig @nodejs-github-bot @jasnell @bnoordhuis @anonrig