Repository navigation
Improve Performance of EventTarget #34074
Description
Activity
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 theEventclass already speeds things up a lot. Whether not usingObject.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.Reacted by Ethan Arrowood and Martin HeideggerAwesome! Whats the best way I can test performance of my changes?
There should be a benchmark in the
benchmark/eventsalready, but feel free to evolve it to something more effective if you'd likeReacted by Benjamin Gruenbaum and Martin Heidegger@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.RAnd 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
eventsbenchmark rather than themessageportone.)Reacted by James M Snell, Ethan Arrowood and Martin HeideggerIf 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.
Reacted by Ethan Arrowood, Anna Henningsen and Martin HeideggerThank 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
Reacted by Benjamin Gruenbaum@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
makeon your computer) which is pretty fast on incremental runs - Run
./out/Release/node --expose-internals test-eventtarget.jsor 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,
Reacted by Ethan Arrowood- Update
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
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.
Extracting an action item from the OpenJS World AbortController collaborator summit:
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!