| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
|
The benchmark results don't really mean anything as spliceOne is not called by it. I'll try to propose a better benchmark. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@TimothyGu doesn't that invert the order? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
Oh, got it. |
Sorry, something went wrong.
Also the original's p-value is way too high (which makes sense if the code paths are the same) |
Sorry, something went wrong.
if (position === 0)
list.shift();
else
spliceOne(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. |
Sorry, something went wrong.
|
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." |
Sorry, something went wrong.
@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... |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
It probably makes sense to update the benchmark as per @TimothyGu's diff. |
Sorry, something went wrong.
Yes, and maybe even better, add it as a benchmark in addition to the existing one rather than updating the existing one. |
Sorry, something went wrong.
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.
For posterity, that's commit d3f8db1. At least I was honest about what I was doing. :-) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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():
Checklist
Affected core subsystem(s)
events