Improve Performance of EventTarget

[Ethan-Arrowood]

Extracting an action item from the OpenJS World AbortController collaborator summit:

The performance of EventTarget is much slower than EventEmitter at the moment (order of magnitude) and we should investigate and improve the performance.

If its okay with @benjamingr and @jasnell (+ any other maintainers that are involved here) I'd like to give this a shot. I'm not sure if it involves just JS or some C++ as well; either way I'm interested in at least investigating and then hopefully improving!

I didn't find an existing issue for this, so if one exists please let me know!

[addaleax]

Nice! Yeah, it would be great if you could take a look at this in more detail.

Some relevant discussion from yesterday: #34057 (comment)

In particular, moving away from private properties and not using Object.defineProperty() inside the Event class already speeds things up a lot. Whether not using Object.defineProperty() and instead using a prototype method is okay is another question, though.

I don’t think this would necessarily involve C++… However, clever use of V8 FunctionTemplates/ObjectTemplates might enable faster creation of these objects. I haven’t looked into that myself.

[Ethan-Arrowood]

Awesome! Whats the best way I can test performance of my changes?

[jasnell]

There should be a benchmark in the benchmark/events already, but feel free to evolve it to something more effective if you'd like

[addaleax]

@Ethan-Arrowood This is my go-to test command (that produced the results I posted in that thread as well):

./node benchmark/compare.js --new ./node --old ./node-master --runs 20 --filter messageport worker | Rscript benchmark/compare.R

And for profiling, this is what I use (on Linux):

env NODE_RUN_BENCHMARK_FN=1 0x --kernel-tracing -o -- ./node benchmark/worker/messageport.js payload=object

(You can probably adapt that quite easily for the events benchmark rather than the messageport one.)

[benjamingr]

If its okay with @benjamingr and @jasnell (+ any other maintainers that are involved here) I'd like to give this a shot.

Semi-meta: you are always welcome to get involved with that sort of change, we don't "own" that code (all the collaborators and project members do). I'm happy to help you figure out how to improve the performance of that code though:

• Compared to the other AIs in the meeting, I think it is the least important to me with getting EventTarget to the point it's spec compliant most important for making unflagging easier. Mostly because EventTarget is currently exposed for very specific events (like data) and the plan is to expose it for pretty expensive events (like messaging across threads).
• I still think investigating its performance is a good idea.

I think the performance wins are probably:

• Remove the linked list in favor of a simpler array-based implementation. I'm not sure why @jasnell implemented things in a linked list and the comment regarding call performance might be true but I suspect that linked lists are slow in V8 and require multiple memory hops. I suspect memory locality is a lot more important than CPU locality in this case. We found this consistently in bluebird.
• Optimize for the simple case (of having one event listener for one event type) since that's the case used in AbortController 99% of the time. This can make event targets both a lot smaller memory-wise and a lot faster.

[Ethan-Arrowood]

Thank you all for the tips! Definitely excited to dig into this 😁

@benjamingr one of my goals is to get more involved in contributing so feel free to direct me towards other action items / issues related to EventTarget/AbortController

[benjamingr]

@Ethan-Arrowood sure and thanks 🙏 , I think the biggest thing I started doing and then got stuck at is (manually porting) WPTs from wpt/dom/events to our test suite and making EventTarget pass them and going over the differences and figure out if our implementation is correct.

This is relatively simple to do since it's only JS (no C++) and it's a "sink" (adding tests and changing one file event_target.js) (but to be fair the C++ stuff in Node is pretty straightforward once you get used to its quirks). My flow has been:

• Update event-target.js
• Build (probably make on your computer) which is pretty fast on incremental runs
• Run ./out/Release/node --expose-internals test-eventtarget.js or whatever I'm testing

If you are on a slower computer where building takes more than a few seconds, you can also extract the relevant JS parts from event-target elsewhere, work on them and copy them back.

Performance is also interesting but not really a big priority.

There are a ton of action items in https://docs.google.com/document/d/19lEj6h1xDe1I2lEvW_HSoyYG1OfAJO2PLe4dNE7gJ0w/edit but I think the biggest ones that are accessible are:

• Make EventTarget spec compliant.
  • Add WPT.
  • Add other tests.
  • Go over the spec and fix deltas (and add tests)
• Make EventTarget fast (this issue) :]
• Figure out the error semantics for EventTarget and what we should do (or keep doing what we do right now)
• Make AbortController spec complaint
  • Add WPT.
  • Add other tests.
  • Go over the spec and fix deltas (and add tests)
• Add support for AbortController for Node APIs (like James did for timers/promises), it's better to make small incremental PRs if you tackle this one.
• Product and QA work: like writing down common user stories for AbortController and testing the impl (and fixing deltas). Talk to community library authors and gather feedback, etc.

These are all open tasks you can get involved with,

[Ethan-Arrowood]

Incredible, thank you! I think I'll start with some of this perf work first to familiarize myself with the current API then I'll dive in to spec compliant stuff. I can also break these action items into issues too so they can be tracked better later today

[Ethan-Arrowood]

PR above has some of my initial attempts + metrics.

I don't know what kind of performance improvement we are looking for. My initial attempts are only showing marginal improvement (and some other attempts actually decreased perf), so it might be an indicator of @benjamingr comment:

Performance is also interesting but not really a big priority

Maybe I shelf this and focus on making EventTarget spec compliant first.