events: remove code duplication

[gwer]

Code duplications are removed from emit* functions.

EventEmitter.prototype.emit becomes shorter.

The logic of selecting an emit* function is in a separate function now.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

events

[mscdex]

Have you benchmarked these changes? The reason they were the way they were was for performance reasons.

[jasnell]

I'm -1 on these changes as is. As @mscdex mentioned, these are there intentionally for performance reasons and should remain. Once we move to enable the TurboFan-Ignition toolchain under V8 5.8 or higher, we will need to revisit this code with an approach that will be optimized for that toolchain.

[Trott]

@gwer Thanks for the PR! To run the benchmarks and check for statistically significant changes, look at the instructions at https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-nodejs-versions. There's a JS runner (compare.js) and a R data cruncher that might or might not reveal something surprising. (I have no idea myself.)

[jasnell]

(fyi... I'm running a comparison of this PR to 7.9 and master to 7.9 now... :-) ...)

[jasnell]

This PR compared to 7.9.0 :

$ ./node benchmark/compare.js --old node --new ./node events > ~/eventsbench
[00:11:16|% 100| 7/7 files | 60/60 runs | 1/1 configs]: Done
 james@ubuntu:~/node/node$ cat ~/eventsbench | Rscript benchmark/compare.R
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -47.52 %        *** 1.217871e-32
 events/ee-emit-multi-args.js n=2000000                 -10.27 %        *** 1.185612e-09
 events/ee-emit.js n=2000000                            -50.98 %        *** 4.618004e-36
 events/ee-listener-count-on-prototype.js n=50000000    -95.13 %        *** 8.151102e-27
 events/ee-listeners-many.js n=5000000                  -21.36 %        *** 9.206574e-25
 events/ee-listeners.js n=5000000                       -48.40 %        *** 1.409208e-45
 events/ee-once.js n=20000000                           -60.39 %        *** 3.561297e-37

Master compared to 7.9.0

$ ./node benchmark/compare.js --old node --new ../main/node events > ~/eventsbench
[00:10:21|% 100| 7/7 files | 60/60 runs | 1/1 configs]: Done
 james@ubuntu:~/node/node$cat ~/eventsbench | Rscript benchmark/compare.Rh
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -45.73 %        *** 8.380381e-18
 events/ee-emit-multi-args.js n=2000000                   0.70 %            2.285963e-01
 events/ee-emit.js n=2000000                              2.12 %        *** 3.829377e-05
 events/ee-listener-count-on-prototype.js n=50000000    -95.07 %        *** 1.717885e-35
 events/ee-listeners-many.js n=5000000                  -20.60 %        *** 8.616063e-14
 events/ee-listeners.js n=5000000                       -48.69 %        *** 2.759171e-60
 events/ee-once.js n=20000000                           -58.16 %        *** 3.019260e-56

Ok, so there are some definite performance regressions that I'm seeing in the event benchmarks independent of this PR, but if we pay attention specifically to the ee-emit-* benchmarks, you'll see that this PR introduces a definite performance regression relative to current master.

[jasnell]

@mscdex ... the results of the benchmark run above ^^ are concerning independent of this PR as there are some results there that simply should not be impacted by this change. Can you please take a moment to see what numbers you're getting for the events benchmarks between master and 7.9?

[TimothyGu]

@jasnell it's very probable that the regression you are seeing comes from #11930, but IMO it is more of a problem of unrealistic benchmark (see #11930 (comment) for my reasoning).

[BridgeAR]

There was no update here for a long time, so I am going to close this. Please feel free to reopen if you would like to follow up on this!