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.

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

worker: make MessagePort inherit from EventTarget #34057

Closed
wants to merge 8 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 25, 2020

events: fix add-remove-add case in EventTarget

Make sure that listeners are added properly if there has previously
been one but currently are none for a given event type.

(→ #34056)

test: delete invalid test

This test has not been working correctly since at least a555be2.

Since it tests internals, any replacement test might become invalid
in a similar way.

events: expand NodeEventTarget functionality

Enable NodeEventTarget as a base class for MessagePort,
by enabling special processing of events for Node.js listeners,
and removing implicit constructors/private properties so that
classes can be made subclasses of NodeEventTarget after they
are created.

test: fixup worker + source map test

The messaging code uses Object.defineProperty(), which accesses
value on Object.prototype by default, so some calls to the
getter here would actually be expected. Instead, make the list
of accessed properties more specific to the tested source map
code to avoid flakiness.

Refs: https://twitter.com/addaleax/status/1276289101671608320

worker: make MessagePort inherit from EventTarget

Use NodeEventTarget to provide a mixed EventEmitter/EventTarget
API interface.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. worker Issues and PRs related to Worker support. eventtarget Issues and PRs related to the EventTarget implementation. labels Jun 25, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 25, 2020
@addaleax
Copy link
Member Author

And here comes the big yikes…

                                                  confidence improvement accuracy (*)    (**)   (***)
worker/messageport.js n=1000000 payload='object'        ***    -56.64 %      ±11.53% ±16.43% ±23.86%
worker/messageport.js n=1000000 payload='string'        ***    -74.24 %      ±10.89% ±15.52% ±22.52%

I assume this will require some more work to get into an acceptable range. 😕

@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Sigh... yeah, unfortunately I think that's expected. EventTarget is quite slow at the moment

@addaleax
Copy link
Member Author

So … moving Event from private properties to symbols and making isTrusted a prototype property removes a lot of the overhead here – I’m not sure how we feel about the latter, since it’s not spec-compliant, and would probably present some kind of security issue in browsers (judging from the name) but Node.js might be in an entirely different position there?

Here’s the benchmark results after those modifications:

                                                   confidence improvement accuracy (*)    (**)   (***)
 worker/messageport.js n=1000000 payload='object'        ***    -18.18 %       ±9.43% ±12.93% ±17.65%
 worker/messageport.js n=1000000 payload='string'        ***    -18.40 %       ±7.62% ±10.49% ±14.42%

still not great, but probably this would already be in the acceptable range for me.

@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

hmm... that's definitely better but still really slow. Let's do some more profiling and see if we can it down further before moving forward.

This test has not been working correctly since at least a555be2.

Since it tests internals, any replacement test might become invalid
in a similar way.
Enable `NodeEventTarget` as a base class for `MessagePort`,
by enabling special processing of events for Node.js listeners,
and removing implicit constructors/private properties so that
classes can be made subclasses of `NodeEventTarget` after they
are created.
The messaging code uses `Object.defineProperty()`, which accesses
`value` on `Object.prototype` by default, so some calls to the
getter here would actually be expected. Instead, make the list
of accessed properties more specific to the tested source map
code to avoid flakiness.

Refs: https://twitter.com/addaleax/status/1276289101671608320
Use `NodeEventTarget` to provide a mixed `EventEmitter`/`EventTarget`
API interface.
addaleax added a commit to addaleax/node that referenced this pull request Jul 20, 2020
This test has not been working correctly since at least a555be2.

Since it tests internals, any replacement test might become invalid
in a similar way.

Refs: nodejs#34057
@addaleax addaleax mentioned this pull request Jul 20, 2020
3 tasks
addaleax added a commit to addaleax/node that referenced this pull request Jul 20, 2020
The messaging code uses `Object.defineProperty()`, which accesses
`value` on `Object.prototype` by default, so some calls to the
getter here would actually be expected. Instead, make the list
of accessed properties more specific to the tested source map
code to avoid flakiness.

Refs: https://twitter.com/addaleax/status/1276289101671608320
Refs: nodejs#34057
@addaleax
Copy link
Member Author

The more I try to work on lessening the performance impact here, the more I become convinced that the current approach behind NodeEventTarget just doesn’t give us performance parity with EventEmitter, and even breaking spec compliance only gives us so much.

So, I think it makes sense to take a different approach to NodeEventTarget, where the Event object is never created if there are only Node.js-style listeners and they don’t receive the Event object itself, as is the case here for MessagePorts. I’ll look into that tomorrow.

@jasnell
Copy link
Member

jasnell commented Jul 20, 2020

The EventTarget approach is so much slower than EventEmitter that it is unsurprising that you're running into issues. Stepping back and rethinking NodeEventTarget is 💯 but I don't think it's ever going to be fully comparable with EventEmitter in performance.

@addaleax
Copy link
Member Author

Fwiw, I don’t see any strict technical limitation that prevents us from achieving performance parity in the case that only Node.js-EventEmitter-style usage is taking place.

@addaleax addaleax marked this pull request as ready for review July 21, 2020 18:59
@addaleax
Copy link
Member Author

Okay, I’ve switched to another approach – instead of lazily generating the Node.js EventEmitter argument from the Event, we now lazily generate the Event object from the Node.js-style argument. That moves performance concerns regarding Event creation out of the way in the common case.

./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter messageport worker | Rscript benchmark/compare.R
[00:03:29|% 100| 1/1 files | 20/20 runs | 4/4 configs]: Done
                                                                        confidence improvement accuracy (*)    (**)   (***)
 worker/messageport.js n=1000000 style='eventemitter' payload='object'          *     -9.63 %       ±9.24% ±12.69% ±17.37%
 worker/messageport.js n=1000000 style='eventemitter' payload='string'                -6.84 %      ±13.35% ±18.54% ±25.81%
 worker/messageport.js n=1000000 style='eventtarget' payload='object'         ***    -50.96 %       ±8.04% ±11.32% ±16.12%
 worker/messageport.js n=1000000 style='eventtarget' payload='string'         ***    -64.79 %       ±7.13%  ±9.95% ±13.97%

The bottom two are using EventTarget-style listeners, the top two are using EventEmitter-style listeners. I’m personally okay with this, particularly since we’re considering this semver-major anyway. This does not include the optimization from #34459 yet, so numbers will likely be better than this in practice (and will improve along with our EventTarget implementation improving in the future).

@jasnell @devnexen @benjamingr @Ethan-Arrowood Could you take another look?

ruyadorno pushed a commit that referenced this pull request Jul 28, 2020
Use `NodeEventTarget` to provide a mixed `EventEmitter`/`EventTarget`
API interface.

PR-URL: #34057
Refs: https://twitter.com/addaleax/status/1276289101671608320
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno added a commit that referenced this pull request Jul 28, 2020
Notable changes:

dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
ruyadorno pushed a commit that referenced this pull request Jul 29, 2020
Use `NodeEventTarget` to provide a mixed `EventEmitter`/`EventTarget`
API interface.

PR-URL: #34057
Refs: https://twitter.com/addaleax/status/1276289101671608320
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno added a commit that referenced this pull request Jul 29, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
MylesBorins pushed a commit that referenced this pull request Jul 29, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
addaleax added a commit that referenced this pull request Sep 22, 2020
The messaging code uses `Object.defineProperty()`, which accesses
`value` on `Object.prototype` by default, so some calls to the
getter here would actually be expected. Instead, make the list
of accessed properties more specific to the tested source map
code to avoid flakiness.

Refs: https://twitter.com/addaleax/status/1276289101671608320
Refs: #34057

PR-URL: #34446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this pull request Sep 22, 2020
This test has not been working correctly since at least a555be2.

Since it tests internals, any replacement test might become invalid
in a similar way.

Refs: #34057

PR-URL: #34445
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this pull request Sep 22, 2020
The messaging code uses `Object.defineProperty()`, which accesses
`value` on `Object.prototype` by default, so some calls to the
getter here would actually be expected. Instead, make the list
of accessed properties more specific to the tested source map
code to avoid flakiness.

Refs: https://twitter.com/addaleax/status/1276289101671608320
Refs: #34057

PR-URL: #34446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this pull request Sep 22, 2020
This test has not been working correctly since at least a555be2.

Since it tests internals, any replacement test might become invalid
in a similar way.

Refs: #34057

PR-URL: #34445
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
stopMessagePort(this);
}
class MessageEvent extends Event {
constructor(data, target, type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@addaleax any reason target is passed here but not stored as in lib/internal/per_context/messageport.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

@GeorgeSapkin That’s because, for Events, the target property is set when they are being emitted. It’s a slight incompatibility between the main-context and non-main-context behaviors, but I doubt that it matters so long as you can access event.target inside of a message event handler

Copy link
Contributor

Choose a reason for hiding this comment

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

So if it's not being used there's no point passing it, i.e. the target argument can be removed, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, the only reason for the parameter being here is for alignment with the class in lib/internal/per_context/messageport.js, but I think it could be removed here.

addaleax added a commit to addaleax/node that referenced this pull request Oct 27, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 28, 2020
Refs: #34057
Refs: #35835

PR-URL: #35839
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2020
Refs: #34057
Refs: #35835

PR-URL: #35839
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Refs: #34057
Refs: #35835

PR-URL: #35839
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Refs: #34057
Refs: #35835

PR-URL: #35839
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Refs: #34057
Refs: #35835

PR-URL: #35839
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. eventtarget Issues and PRs related to the EventTarget implementation. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants