add/remove handlers are slow on EventTarget

[ronag]

To the point that e.g. RxJS choose not to implement the abort signal pattern.

ReactiveX/rxjs#6675 (comment)

[anonrig]

Benchmark file: https://github.com/nodejs/node/blob/main/benchmark/events/eventtarget.js

[debadree25]

During adding a event listener we traverse the entire linkedlist to find duplicates, i tried to avoid that here nodejs/node@f6c2580 but it seems not much of a perf improvement, almost same performance, any other methods in mind?

[benjamingr]

Given maps support iterating in insertion order iirc - what about using maps instead of linked lists and not just for duplicate checking?

[metcoder95]

Unless I'm missing something, using Maps and calling Map#has for checking if a value exists or not, incurs iterating twice over the Map in the worst-case scenario.
If SafeMap behaves similarly, just calling SafeMap#get remove the first iteration over the Map, and can be replaced with a nullish (== null) for checking the duplicateListener (also the same for listenerType).

[debadree25]

Given maps support iterating in insertion order iirc - what about using maps instead of linked lists and not just for duplicate checking?

So the implementation of Listener in hashmaps?

[debadree25]

@metcoder95 makes sense let me see making the change

[debadree25]

Tried this not really any improvement 😅

                                             confidence improvement accuracy (*)   (**)  (***)
events/eventtarget.js listeners=1 n=1000000                  0.74 %       ±4.06% ±5.41% ±7.04%
events/eventtarget.js listeners=10 n=1000000                 1.59 %       ±4.39% ±5.84% ±7.61%
events/eventtarget.js listeners=5 n=1000000                 -1.41 %       ±4.27% ±5.70% ±7.47%
events/eventtarget.js listeners=50 n=1000000                 1.34 %       ±4.03% ±5.38% ±7.05%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

[debadree25]

Also I think this would need a fresh benchmark? this given benchmark is primarily measuring speed of event dispatch

it could look something like

for (let i = 0; i < n; i++) {
  const list = [];
  for (let j = 0; j < listeners; j++) {
     const fn = () => {}
     target.addEventListener('foo', fn);
     list.push(fn);
  }
 // remove the listeners from list....
}

[metcoder95]

Tried this not really any improvement 😅

That's sad 🙁

Also I think this would need a fresh benchmark? this given benchmark is primarily measuring speed of event dispatch

Maybe splitting it into two biggest chunks? add/remove, and event dispatch?
Seeing the benchmark once again it seems is overlapping the two topics

[debadree25]

Trying to open a new pr with this add and remove benchmark

[debadree25]

Opened and PR and as (probably expected) my code shows significant regression 😕 with this benchmark