events: support abortsignal in ee.on

[benjamingr]

An attempt to support passing a signal to EventEmitter's once in order to unsubscribe.

This is useful for ergonomically unsubscribing from multiple events at once.

This is very much WIP and should not be merged. It currently introduces a ±12% performance regression in the add/remove listeners benchmarks.

I am interested mostly in:

• Discussion about the API - is this even a good idea? is there ecosystem breakage potential? Is there a better API for this?
• People to possibly play around with it so we can figure out if this (and the EventTarget counterpart at Support AbortSignals in addEventListeners options to unsubscribe from events? whatwg/dom#911 ) is a good idea.
• Ideas on how to avoid the performance regression (swap out removeListener and addListener on the emitter when a signal is passed for the first time maybe?)

cc @nodejs/events @jasnell

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

[benjamingr]

Last benchmark run on my computer. I believe this regression indicates we should not merge this before fixing it even if we decide this API is good and we like it:

 confidence improvement accuracy (*) (**) (***) events/ee-add-remove.js n=1000000 *** -13.98 % ±2.81% ±3.73% ±4.86% events/ee-emit.js listeners=1 argc=0 n=2000000 -0.10 % ±3.50% ±4.67% ±6.11% events/ee-emit.js listeners=1 argc=10 n=2000000 2.65 % ±3.28% ±4.38% ±5.73% events/ee-emit.js listeners=1 argc=2 n=2000000 -1.87 % ±3.87% ±5.15% ±6.70% events/ee-emit.js listeners=1 argc=4 n=2000000 2.30 % ±3.35% ±4.46% ±5.80% events/ee-emit.js listeners=10 argc=0 n=2000000 -0.25 % ±4.00% ±5.32% ±6.93% events/ee-emit.js listeners=10 argc=10 n=2000000 -1.21 % ±3.00% ±3.99% ±5.20% events/ee-emit.js listeners=10 argc=2 n=2000000 0.15 % ±3.25% ±4.33% ±5.63% events/ee-emit.js listeners=10 argc=4 n=2000000 * 4.06 % ±3.40% ±4.53% ±5.91% events/ee-emit.js listeners=5 argc=0 n=2000000 -0.99 % ±4.27% ±5.68% ±7.40% events/ee-emit.js listeners=5 argc=10 n=2000000 -0.67 % ±2.77% ±3.68% ±4.80% events/ee-emit.js listeners=5 argc=2 n=2000000 -0.42 % ±3.76% ±5.01% ±6.52% events/ee-emit.js listeners=5 argc=4 n=2000000 1.49 % ±3.39% ±4.51% ±5.88% events/ee-listener-count-on-prototype.js n=50000000 * -2.01 % ±2.00% ±2.67% ±3.47% events/ee-listeners.js raw='false' listeners=5 n=5000000 0.34 % ±4.16% ±5.54% ±7.21% events/ee-listeners.js raw='false' listeners=50 n=5000000 0.32 % ±2.61% ±3.48% ±4.53% events/ee-listeners.js raw='true' listeners=5 n=5000000 -1.29 % ±3.03% ±4.04% ±5.26% events/ee-listeners.js raw='true' listeners=50 n=5000000 1.87 % ±3.83% ±5.09% ±6.63% events/ee-once.js argc=0 n=20000000 *** -4.14 % ±1.73% ±2.30% ±3.00% events/ee-once.js argc=1 n=20000000 *** -4.16 % ±2.06% ±2.75% ±3.59% events/ee-once.js argc=4 n=20000000 ** -3.48 % ±2.23% ±2.97% ±3.87% events/ee-once.js argc=5 n=20000000 -1.25 % ±1.77% ±2.35% ±3.06% events/eventtarget.js listeners=1 n=1000000 -1.45 % ±3.61% ±4.80% ±6.25% events/eventtarget.js listeners=10 n=1000000 -0.00 % ±3.12% ±4.15% ±5.41% 

[benjamingr]

The performance regressions still looks pretty bad @addaleax@jasnell :

12:28:36 confidence improvement accuracy (*) (**) (***) 12:28:36 events/ee-add-remove.js n=1000000 *** -11.46 % ±3.47% ±4.66% ±6.16% 12:28:36 events/ee-emit.js listeners=10 argc=0 n=2000000 1.40 % ±1.90% ±2.54% ±3.34% 12:28:36 events/ee-emit.js listeners=10 argc=10 n=2000000 1.77 % ±2.41% ±3.21% ±4.20% 12:28:36 events/ee-emit.js listeners=10 argc=2 n=2000000 0.61 % ±1.41% ±1.89% ±2.48% 12:28:36 events/ee-emit.js listeners=10 argc=4 n=2000000 -0.17 % ±1.11% ±1.49% ±1.95% 12:28:36 events/ee-emit.js listeners=1 argc=0 n=2000000 1.61 % ±8.51% ±11.44% ±15.11% 12:28:36 events/ee-emit.js listeners=1 argc=10 n=2000000 2.31 % ±4.36% ±5.82% ±7.62% 12:28:36 events/ee-emit.js listeners=1 argc=2 n=2000000 2.29 % ±4.45% ±5.93% ±7.71% 12:28:36 events/ee-emit.js listeners=1 argc=4 n=2000000 * 3.39 % ±3.31% ±4.41% ±5.74% 12:28:36 events/ee-emit.js listeners=5 argc=0 n=2000000 0.04 % ±2.06% ±2.74% ±3.57% 12:28:36 events/ee-emit.js listeners=5 argc=10 n=2000000 1.48 % ±1.86% ±2.47% ±3.23% 12:28:36 events/ee-emit.js listeners=5 argc=2 n=2000000 0.37 % ±1.69% ±2.25% ±2.93% 12:28:36 events/ee-emit.js listeners=5 argc=4 n=2000000 -1.20 % ±2.32% ±3.09% ±4.02% 12:28:36 events/ee-listener-count-on-prototype.js n=50000000 -2.08 % ±4.10% ±5.45% ±7.10% 12:28:36 events/ee-listeners.js raw='false' listeners=50 n=5000000 ** -1.68 % ±1.25% ±1.66% ±2.16% 12:28:36 events/ee-listeners.js raw='false' listeners=5 n=5000000 0.85 % ±2.11% ±2.80% ±3.65% 12:28:36 events/ee-listeners.js raw='true' listeners=50 n=5000000 -0.59 % ±2.19% ±2.91% ±3.79% 12:28:36 events/ee-listeners.js raw='true' listeners=5 n=5000000 0.67 % ±2.49% ±3.31% ±4.31% 12:28:36 events/ee-once.js argc=0 n=20000000 *** -8.62 % ±2.62% ±3.51% ±4.62% 12:28:36 events/ee-once.js argc=1 n=20000000 *** -6.88 % ±2.60% ±3.48% ±4.59% 12:28:36 events/ee-once.js argc=4 n=20000000 * -3.97 % ±3.06% ±4.07% ±5.30% 12:28:36 events/ee-once.js argc=5 n=20000000 *** -6.48 % ±1.97% ±2.62% ±3.42% 12:28:36 events/eventtarget.js listeners=10 n=1000000 -0.56 % ±1.88% ±2.50% ±3.26% 12:28:36 events/eventtarget.js listeners=1 n=1000000 1.02 % ±1.59% ±2.11% ±2.75% 12:28:36 events/eventtarget.js listeners=5 n=1000000 -0.04 % ±1.74% ±2.32% ±3.02% 12:28:36 

(from our CI https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/674/console )

I am kind of confused by this and will investigate but any help/idea welcome

[addaleax]

It’s not really obvious to me what’s going on performance-wise here either, sorry :/

[addaleax]

This would throw if signal === undefined, btw, and it doesn’t look like that would be intentional

[mcollina]

The reason of the slowdown is that you are adding an additional parameter... this adds an additional frame if there is an arguments length mismatch. An API with fixed arguments is around 10% faster than one with fixed arguments.: normally this would not matter much but on and once are called everywhere, so it matters for the overall performance of Node.js.

The good news is that the V8 is already working on the problem: https://bugs.chromium.org/p/v8/issues/detail?id=10201#c94.

---

On the API side, I would focus more on supporting this in other places: fs.readFile and fs.writeFile (and their pomise-based APIs), streams, http.request. I think adding it to EE is a too low level at this point.

[benjamingr]

@mcollina thanks that makes perfect sense.

[benjamingr]

I will close this in the meantime and suggest we revisit this once V8 has made optimizing this possible.

I don't think we can justify the perf regression at this point and the promise returning variants already support AbortSignal. That said if others feel strongly feel free to take the code here (with or without attribution) and try to gain consensus for it (I'd be -0).

Thanks for the help Anna/Matteo!

[benjamingr]

Also, I'll see about fs.readFile, I thought there was an effort to add AbortSignal support to stuff by @ptomato and I didn't want to do any toe-stepping.

I'll try to make a small change (like only readFile) and iterate from there. readFile should be easy since it's probably fine to just cancel between chunks rather than abort the actual OS operation.

[ptomato]

I do have that in a branch but I haven't worked on it in a few months so it will need rebasing. I'd appreciate if you could take a look at the approach taken in #34080 and let me know if that seems reasonable enough to make a real pull request out of. If so, then I will rebase it and remove the draft status, and follow up with fs.readFile.

(I don't think I'd be able to get around to it this week or next week, but hopefully the week after.)

[ptomato]

Sadly I did not get around to it last week and it looks like I won't be able to until January. Sorry for mis-estimating! If someone else would like to pick it up sooner, I'll be happy to do whatever is necessary to hand it off.

[erichocean]

v8 has now fixed the "adding an undeclared argument introduces a 10% overhead" problem, so this can be revisited.

[benjamingr]

@erichocean waiting for Node to update its v8 version first :)

[benjamingr]

Going to see what performance looks like now.

Edit: Ignore the merge commit this is just here to make it easier to run benchmarks, https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1088/

[mcollina]

I'm -1 on this. I don't think there is a good way to do it without decreasing perf. I don't see how this would benefit our APIs.

[benjamingr]

I'm -1 on this. I don't think there is a good way to do it without decreasing perf.

I think we can agree that if there is a significant perf regression this shouldn't land.

[benjamingr]

Benchmarks locally still show a 4% (significant) regression - let's try again in a year :)