events: add type check for event name

[Flarna]

According to docs only string/symbol are allowed but the checks are more relaxed to allow also number/bigint as it seems they are used in user land modules.

There were already checks in place in the past and they have been removed have been removed.
Another try to add them again was done but never finished (#21007).
Therefore I'm not sure if this should be added or not. As I had most of the changes already in place once I noticed this I think it's better to discuss this in a PR.

Fixes: #31331
Refs: 0468861
Refs: #21007

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Edit: Removed null as it was removed during review phase.

[BridgeAR]

LGTM.

[legendecas]

Will this be semver-minor or semver-major for the possible breaking changes on existing codes?

[BridgeAR]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2238/ (queued)

[Trott]

/ping @nodejs/tsc This is a straightforward change, but it is semver-major so will need TSC reviews.

[jasnell]

This needs to be thoroughly benchmarked before it can land. This is one of the single most performance sensitive apis in core.

[mcollina]

I’m +1 if it does not cause a perf regression to the eventemitter, http, and streams benchmarks.

[Flarna]

rebased/squashed and updated commit message

[tniessen]

Is there any risk of breaking existing code? If so, would a deprecation cycle be better?

[jasnell]

Benchmark run (may take a while for the link to be good while jenkins spins up the job): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/514/

[jasnell]

Results of the benchmark run:

02:31:24 confidence improvement accuracy (*) (**) (***)
02:31:24 events/ee-add-remove.js n=1000000 -2.88 % ±3.99% ±5.31% ±6.91%
02:31:24 events/ee-emit.js listeners=10 argc=0 n=2000000 -0.70 % ±1.33% ±1.77% ±2.31%
02:31:24 events/ee-emit.js listeners=10 argc=10 n=2000000 0.83 % ±2.83% ±3.77% ±4.90%
02:31:24 events/ee-emit.js listeners=10 argc=2 n=2000000 0.69 % ±1.76% ±2.35% ±3.08%
02:31:24 events/ee-emit.js listeners=10 argc=4 n=2000000 0.70 % ±0.90% ±1.19% ±1.55%
02:31:24 events/ee-emit.js listeners=1 argc=0 n=2000000 -2.60 % ±4.33% ±5.77% ±7.51%
02:31:24 events/ee-emit.js listeners=1 argc=10 n=2000000 4.04 % ±6.38% ±8.51% ±11.10%
02:31:24 events/ee-emit.js listeners=1 argc=2 n=2000000 -0.38 % ±5.35% ±7.12% ±9.29%
02:31:24 events/ee-emit.js listeners=1 argc=4 n=2000000 3.58 % ±4.66% ±6.20% ±8.07%
02:31:24 events/ee-emit.js listeners=5 argc=0 n=2000000 * 2.08 % ±1.86% ±2.48% ±3.24%
02:31:24 events/ee-emit.js listeners=5 argc=10 n=2000000 0.01 % ±2.64% ±3.52% ±4.58%
02:31:24 events/ee-emit.js listeners=5 argc=2 n=2000000 1.51 % ±1.73% ±2.30% ±3.00%
02:31:24 events/ee-emit.js listeners=5 argc=4 n=2000000 0.22 % ±2.45% ±3.27% ±4.26%
02:31:24 events/ee-listener-count-on-prototype.js n=50000000 -0.03 % ±6.26% ±8.33% ±10.85%
02:31:24 events/ee-listeners.js n=5000000 -1.02 % ±4.32% ±5.78% ±7.58%
02:31:24 events/ee-listeners-many.js n=5000000 -2.79 % ±4.63% ±6.22% ±8.21%
02:31:24 events/ee-once.js argc=0 n=20000000 *** -5.41 % ±2.33% ±3.10% ±4.03%
02:31:24 events/ee-once.js argc=1 n=20000000 *** -3.85 % ±1.88% ±2.51% ±3.26%
02:31:24 events/ee-once.js argc=4 n=20000000 *** -8.29 % ±2.98% ±3.97% ±5.18%
02:31:24 events/ee-once.js argc=5 n=20000000 -1.77 % ±3.76% ±5.04% ±6.62%

The significant slow down in once handling should be looked at before this lands.

[jasnell]

Making -1 explicit until benchmark results can be looked at.

[Flarna]

Seems using an if instead a switch is significant faster.
Hope my local benchmark runs show the truth, never ran then before.

[jasnell]

I'll kick off another CI benchmark run to double check :-)

[jasnell]

New Benchmark Run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/515/

[Flarna]

@jasnell Seems you accidentally started benchmarks for category assert instead event

[jasnell]

Sigh lol ok. Will restart soon

[jasnell]

Trying again: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/516/

[Flarna]

new benchmark results:

16:01:21 confidence improvement accuracy (*) (**) (***)
16:01:21 events/ee-add-remove.js n=1000000 *** -6.84 % ±3.58% ±4.78% ±6.23%
16:01:21 events/ee-emit.js listeners=10 argc=0 n=2000000 0.56 % ±1.86% ±2.48% ±3.23%
16:01:21 events/ee-emit.js listeners=10 argc=10 n=2000000 -0.32 % ±3.00% ±4.02% ±5.28%
16:01:21 events/ee-emit.js listeners=10 argc=2 n=2000000 0.73 % ±1.28% ±1.70% ±2.22%
16:01:21 events/ee-emit.js listeners=10 argc=4 n=2000000 -0.98 % ±2.04% ±2.74% ±3.61%
16:01:21 events/ee-emit.js listeners=1 argc=0 n=2000000 * 4.94 % ±4.92% ±6.55% ±8.55%
16:01:21 events/ee-emit.js listeners=1 argc=10 n=2000000 -2.95 % ±4.71% ±6.27% ±8.17%
16:01:21 events/ee-emit.js listeners=1 argc=2 n=2000000 -0.46 % ±4.30% ±5.72% ±7.45%
16:01:21 events/ee-emit.js listeners=1 argc=4 n=2000000 0.57 % ±4.27% ±5.69% ±7.40%
16:01:21 events/ee-emit.js listeners=5 argc=0 n=2000000 -2.09 % ±4.70% ±6.29% ±8.26%
16:01:21 events/ee-emit.js listeners=5 argc=10 n=2000000 -0.07 % ±1.86% ±2.48% ±3.23%
16:01:21 events/ee-emit.js listeners=5 argc=2 n=2000000 -1.49 % ±1.59% ±2.12% ±2.76%
16:01:21 events/ee-emit.js listeners=5 argc=4 n=2000000 1.34 % ±4.91% ±6.59% ±8.69%
16:01:21 events/ee-listener-count-on-prototype.js n=50000000 0.03 % ±6.54% ±8.71% ±11.35%
16:01:21 events/ee-listeners.js n=5000000 -0.85 % ±3.47% ±4.62% ±6.01%
16:01:21 events/ee-listeners-many.js n=5000000 2.01 % ±4.12% ±5.48% ±7.13%
16:01:21 events/ee-once.js argc=0 n=20000000 -0.95 % ±1.88% ±2.51% ±3.26%
16:01:21 events/ee-once.js argc=1 n=20000000 ** -3.58 % ±2.08% ±2.78% ±3.62%
16:01:21 events/ee-once.js argc=4 n=20000000 -2.84 % ±3.50% ±4.66% ±6.06%
16:01:21 events/ee-once.js argc=5 n=20000000 * -3.09 % ±2.80% ±3.73% ±4.86%

[jasnell]

Still a bit of a concern. It's interesting that the ee-add-remove shows a significant regression in the second run but not the first. I think that's just really showing the performance sensitivity in this part.

I guess I can't really come up with a great argument not to add this but I'm also not really seeing much of a benefit to adding this check. @addaleax and @mcollina, what do you think?

[addaleax]

I’m personally good with not adding typechecking if it affects performance – for events, it wouldn’t be a need-to-have anyway (and I’ve never seen non-string/symbol values being passed as event names in the wild, too).