lib: faster event listening

[Farenheith]

The current EventEmitter uses arrays to control multiple listeners,
but that means that prependListener and removeListeners operations
have an O(N) time complexity. With this change, instead of an array
we use a map + a linked list (to preserve execution order), which
gives an O(1) time complexity for those operations. This will be
especially helpful in scenarios where we have many listeners to
the same event.

BREAKING CHANGE: I don't know how much this is a breaking change,
because if someone is using the _events property, I think it is bad
usage that has no guarantees to keep working. Regardless, the _events
property will no longer be a function or an array, but a function
or a "MappedLinkedList". Some of the operations from the array are
present, though, like [Symbol.iterator], unshift and push, making
it possible to adapt some code that is arbritary using it

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[BridgeAR]

Thanks, this really looks good! You might want to add a few benchmarks or run existing ones in case they already indicate improvements and post the results here.

About the breaking change: I do not think we are able to break the property usage. We could however add a private/symbol property, use that internally and use a proxy on the _events property that maps the entries to prevent breaking existing usage.

[benjamingr]

Linked lists are almost never ever better than arrays because often code is dominated by memory access and cache locality is a lot bigger of a deal than doing O(n) operations.

Additionally, prependListener is not the most common use case to optimize for.

I am open to being wrong - this definitely needs a benchmark :)

(Aside, good job figuring out all the bits of working with the code for your first contribution!)

[Farenheith]

Thank you, guys. I'll work on the benchmarks after Christmas and post them here. I think it's expected that a better performance is achieved when we have many listeners at the same event.

@benjamingr in this particular case of use, this linked list is only there to preserve execution order, what will really make a difference in performance is the Map, I think. But I'm not sure if this change can bring a better performance overall, there's really a memory trade-off here and in most cases the average program doesn't add too many listeners to the same event, so maybe this change would bring benefits only in edge cases. But I'll benchmark this ASAP

[benjamingr]

@Farenheith that sounds very reasonable to me. Feel free to ping me if you have any issue running the benchmarks. I agree it's probably a trade-off.

These PRs are good by the way, even if the code doesn't eventually get merged they are a great idea to build an intuition for the code and figure out what's slow/fast/needs work :)

(As a side note, we all happen to be guys in this conversation but this project has a lot of non male (and non binary) contributors that make up a significant portion of the project, the code written and the leadership - so it is best to avoid "guys" and gendered pronouns in PRs for "folks" "people" etc)

[Farenheith]

@benjamingr thank you for the tip about "guys". I actually thought it was a gender-neutral term because of some literal translation from my mother language lol. But it's good to know better about it.

[Farenheith]

Folks, I want to ask about the file I added: mapped_linkedlist.
I'm not used to use WeakMap, but I think I could use it there instead of Map, as all of my keys will be functions, I don't need to iterate over the map, and I'll have other memory references to them. Do you know if there are any performance benefits from using WeakMap instead of Map in this case?

[Farenheith]

About the breaking change: I do not think we are able to break the property usage. We could however add a private/symbol property, use that internally and use a proxy on the _events property that maps the entries to prevent breaking existing usage.

@BridgeAR that's a really good idea. I'll work on that

[jasnell]

Something else to consider here is that while the _events property is technically "internal", there are a non-trivial number of userland modules that access it. This change could break a lot of people. We've tried to make performance improvements here before and kept running into issues breaking nearly everyone. We need to be careful.

[Farenheith]

@jasnell @BridgeAR @benjamingr I ran the benchmarks and the results aren't good.
Maybe is something wrong with my implementation, but for now, I think it's better to close this PR
@benjamingr thank you for showing me how to run the benchmarks, if I open a PR like that in the future I'll make sure to not make a breaking change and I'll check the benchmark first :)

"old","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",1089374.5258551845,0.917957944
"old","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",1086476.0229941525,0.920406874
"new","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",180489.97320270602,5.540473979
"new","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",185656.86911819418,5.386280641
"new","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",184314.32196097678,5.425514357
"new","events/ee-add-remove.js","n=1000000 removeListener=0 newListener=0",214791.9037945568,4.655668963

[apapirovski]

@Farenheith this has a loooooong history of being attempted, without success, see #21856 and #17074 for prior takes.

[Farenheith]

@apapirovski I was thinking about it and I suppose the catch here is that not only prepend is a very uncommon operation, but also the removeListener operation tends to remove the last listener of the array in most of the uses, so there's not much room for optimization.