worker: make MessagePort inherit from EventTarget

[addaleax]

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]

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]

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

[addaleax]

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]

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.

[addaleax]

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]

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]

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]

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?

[GeorgeSapkin]

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

[addaleax]

@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

[GeorgeSapkin]

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

[addaleax]

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.