events: remove internal spliceOne()

[Trott]

spliceOne() was added as a faster alternative to Array#splice in the
particular use case in events.js. However, current master shows no
difference in performance with or without spliceOne():

 improvement confidence p.value events/ee-add-remove.js n=250000 0.14 % 0.7573913

Checklist

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

Affected core subsystem(s)

events

[lpinca]

LGTM if results from #14081 are confirmed.

[TimothyGu]

The benchmark results don't really mean anything as spliceOne is not called by it. I'll try to propose a better benchmark.

[TimothyGu]

With

diff --git a/benchmark/events/ee-add-remove.js b/benchmark/events/ee-add-remove.js
index 99d85367cb..1f560b34cd 100644
--- a/benchmark/events/ee-add-remove.js+++ b/benchmark/events/ee-add-remove.js@@ -16,7 +16,7 @@ function main(conf) {
bench.start();
for (var i = 0; i < n; i += 1) {
- for (k = listeners.length; --k >= 0; /* empty */)+ for (k = 0; k < listeners.length; k++)
ee.on('dummy', listeners[k]);
for (k = listeners.length; --k >= 0; /* empty */)
ee.removeListener('dummy', listeners[k]);

which is about as synthetic as the original benchmark, I got

 improvement confidence p.value
events/ee-add-remove.js n=250000 -52.88 % *** 2.145577e-46

[lpinca]

@TimothyGu doesn't that invert the order?

[TimothyGu]

@lpinca Yes. The spliceOne() branch is only called when the function to be deleted is not the first to be added. In the original benchmark, by adding and removing the listeners in order, the spliceOne() branch is never taken. I inverted the listeners so that the splice branch is always called, except on the last removal.

[lpinca]

Oh, got it.

[refack]

The benchmark results don't really mean anything as spliceOne is not called by it. I'll try to propose a better benchmark.

Also the original's p-value is way too high (which makes sense if the code paths are the same)

[Sunqinying]

@refack

if(position===0)list.shift();elsespliceOne(list,position);

the benchmark script will never call spliceOne。 But I'm still curious about why Array.splice is slower than spliceOne when run benchmark(script change to the same with @TimothyGu ), but quicker when I use console.time.

[Sunqinying]

I find the commit with comment is that "lib: micro-optimize EventEmitter#removeListener() Replace the call to Array#splice() with a faster open-coded version that creates less garbage. Add a new benchmark to prove it. With the change applied, it scores a whopping 40% higher."
Maybe It's nothing about speed , for memory optimize.

[refack]

the benchmark script will never call spliceOne() But I'm still curious about why Array.splice is slower than spliceOne when run benchmark(script change to the same with @TimothyGu ), but quicker when I use console.time.

@Sunqinying it is interesting... probably comes from the fact your code does a 1000 splices on a single array, and node's benchmark probably does less. Might also be because your array if filled with numbers, and here the array has function pointers...

[Trott]

Ooof, I assumed the benchmark exercised the spliceOne() code because it was originally introduced to measure exactly that. But I guess the algorithm/code paths have changed since then. Closing, sorry/thanks.

[lpinca]

It probably makes sense to update the benchmark as per @TimothyGu's diff.

[Trott]

It probably makes sense to update the benchmark as per @TimothyGu's diff.

Yes, and maybe even better, add it as a benchmark in addition to the existing one rather than updating the existing one.

[bnoordhuis]

Maybe It's nothing about speed , for memory optimize.

That's correct. array.splice(n, 1) creates (and returns) a new array for the spliced element, spliceOne() doesn't.

V8 wasn't (and presumably isn't) smart enough to figure out the return value is unused and can be omitted.

lib: micro-optimize EventEmitter#removeListener()

For posterity, that's commit d3f8db1. At least I was honest about what I was doing. :-)