events: remove emit micro-optimizations by apapirovski · Pull Request #16869 · nodejs/node

apapirovski · 2017-11-07T18:40:44Z

With improvements in V8, using separate emit functions is no longer necessary. This is not applicable to v8.x or earlier (the earlier version that's baking is though).

improvement confidence p.value
events/ee-emit.js n=2000000 2.98 % 0.09852489
events/ee-emit-2-args.js n=2000000 4.19 % *** 0.0001914216
events/ee-emit-6-args.js n=2000000 61.69 % *** 6.611964e-35
events/ee-emit-diff-args.js n=2000000 -0.36 % 0.305069
events/ee-once.js n=20000000 6.42 % *** 1.27831e-06

ee-emit-diff-args.js was a custom benchmark to confirm whether there was any deoptimization happening when the function pattern — number of arguments passed in — would change unpredictably (between 0 - 3 arguments, so what used to be the fast path). I also had a version of that benchmark with 0-4 arguments and that one was +20% or so, due to the improvements in emit performance for more than 3 arguments.

Checklist

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

Affected core subsystem(s)

benchmark, events

With improvements in V8, using separate emit functions is no longer necessary and can instead be replaced by the spread operator. improvement confidence p.value events/ee-emit.js n=2000000 2.98 % 0.09852489 events/ee-emit-2-args.js n=2000000 4.19 % *** 0.0001914216 events/ee-emit-6-args.js n=2000000 61.69 % *** 6.611964e-35 events/ee-emit-diff-args.js n=2000000 -0.36 % 0.305069 events/ee-once.js n=20000000 6.42 % *** 1.27831e-06

apapirovski · 2017-11-07T18:41:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11277/

addaleax

nice! 👏

apapirovski · 2017-11-07T18:43:15Z

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/23/

Results

 events/ee-add-remove.js n=250000 -1.25 % 5.003391e-02
events/ee-emit-2-args.js n=2000000 3.25 % *** 3.664056e-13
events/ee-emit-4-args.js n=2000000 45.91 % *** 1.096930e-59
events/ee-emit.js n=2000000 1.66 % *** 8.587999e-04
events/ee-listener-count-on-prototype.js n=50000000 -0.14 % 9.623456e-01
events/ee-listeners.js n=5000000 -0.93 % 1.751751e-01
events/ee-listeners-many.js n=5000000 0.10 % 9.296697e-01
events/ee-once.js n=20000000 4.57 % ** 2.418052e-03

mscdex · 2017-11-07T19:42:59Z

We should just have one ee-emit.js benchmark which has a parameter for number of args, something like 0-6 maybe.

mscdex · 2017-11-07T19:45:33Z

Also, we should probably make the number of listeners configurable as well, so that we can make sure there are no regressions for smaller numbers of listeners (e.g. 2-9).

apapirovski · 2017-11-07T20:19:32Z

Is the AIX failure related? Anyone know? Doesn't seem like it but...

lpinca · 2017-11-07T20:22:00Z

It is failing since yesterday, not related.

mathiasbynens · 2017-11-07T23:25:41Z

+
+ const ee = new EventEmitter();
+
+ for (var k = 0; k < 10; k += 1)


Coding style question: why not let? (Not just here but throughout the patch)

its much slower because it creates a closure for the variable in each loop iteration

@mathiasbynens We have a lint rule re: this so that's the main reason. I think @TimothyGu tried to change it recently and feedback from @bmeurer was that we shouldn't quite yet.

Here's the relevant PR with more conversation: #15648

bmeurer · 2017-11-08T07:42:57Z

+ handler.apply(this, args);
+ } else {
+ const len = handler.length;
+ const listeners = arrayClone(handler, len);


Can you leave a TODO here to consider switching to Array.prototype.slice once V8 6.4 lands in Node? Or even to evaluate the idea of avoiding the defensive copy on emit and rather making sure that the handler itself is never mutated?

bmeurer

LGTM modulo comment.

Seeing this happening makes me happy!

mscdex

Making my concerns more explicit ...

We should just have one ee-emit.js benchmark which has a parameter for number of args, something like 0-6 maybe.

Also, we should probably make the number of listeners configurable as well, so that we can make sure there are no regressions for smaller numbers of listeners (e.g. 2-9).

apapirovski · 2017-11-08T17:40:42Z

New benchmark CI for the changes: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/28/

events/ee-add-remove.js n=250000 -0.51 % 3.833557e-01
events/ee-emit-2-args.js n=2000000 3.40 % ** 3.686601e-03
events/ee-emit-4-args.js n=2000000 45.03 % *** 3.944833e-67
events/ee-emit.js listeners=10 argc=0 n=2000000 -4.79 % *** 1.212118e-18
events/ee-emit.js listeners=10 argc=10 n=2000000 13.82 % *** 3.714492e-15
events/ee-emit.js listeners=10 argc=2 n=2000000 -15.31 % *** 8.885326e-36
events/ee-emit.js listeners=10 argc=4 n=2000000 14.30 % *** 6.484010e-11
events/ee-emit.js listeners=1 argc=0 n=2000000 -0.04 % 9.825977e-01
events/ee-emit.js listeners=1 argc=10 n=2000000 21.72 % *** 5.891698e-07
events/ee-emit.js listeners=1 argc=2 n=2000000 -0.75 % 4.022986e-01
events/ee-emit.js listeners=1 argc=4 n=2000000 26.56 % *** 8.406902e-29
events/ee-emit.js listeners=5 argc=0 n=2000000 -3.05 % *** 2.006354e-06
events/ee-emit.js listeners=5 argc=10 n=2000000 32.30 % *** 1.157809e-12
events/ee-emit.js listeners=5 argc=2 n=2000000 -5.86 % *** 2.034290e-22
events/ee-emit.js listeners=5 argc=4 n=2000000 22.25 % *** 5.349469e-39
events/ee-listener-count-on-prototype.js n=50000000 -0.27 % 8.455537e-01
events/ee-listeners.js n=5000000 0.81 % 1.344333e-01
events/ee-listeners-many.js n=5000000 3.61 % * 4.947461e-02
events/ee-once.js n=20000000 6.30 % *** 4.165866e-05

I have absolutely no clue why there's exactly one benchmark that tanked on the new version:

 events/ee-emit.js listeners=10 argc=2 n=2000000 -15.31 % *** 8.885326e-36

As far as I can tell this is almost equivalent to the old ee-emit-multi-args.js.

apapirovski · 2017-11-08T18:25:12Z

Ok, that benchmark needs to be tweaked. There's an optimization happening when using apply or bind that doesn't when one just makes a regular emit call. Working on v2.

apapirovski · 2017-11-08T18:35:16Z

Ok, created a second version of the benchmark that doesn't use apply or bind (I assume that using those let V8 optimize for the exact number of arguments being passed which favoured the previous version of the code). The new benchmark just plainly calls emit like before.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/31/

(The reason I changed it is that using apply or bind with emit is not something we do in our code. It doesn't seem worth it to test for a scenario that doesn't exist.)

apapirovski · 2017-11-08T19:03:08Z

New benchmark results:

 events/ee-emit.js listeners=10 argc=0 n=2000000 -3.54 % *** 6.874300e-06
events/ee-emit.js listeners=10 argc=10 n=2000000 57.01 % *** 5.922333e-68
events/ee-emit.js listeners=10 argc=2 n=2000000 1.52 % 8.892891e-02
events/ee-emit.js listeners=10 argc=4 n=2000000 45.02 % *** 1.893663e-72
events/ee-emit.js listeners=1 argc=0 n=2000000 -0.18 % 9.229945e-01
events/ee-emit.js listeners=1 argc=10 n=2000000 51.86 % *** 4.830343e-21
events/ee-emit.js listeners=1 argc=2 n=2000000 4.25 % *** 2.762911e-06
events/ee-emit.js listeners=1 argc=4 n=2000000 41.83 % *** 2.613486e-39
events/ee-emit.js listeners=5 argc=0 n=2000000 -0.72 % 2.251744e-01
events/ee-emit.js listeners=5 argc=10 n=2000000 59.05 % *** 6.230044e-24
events/ee-emit.js listeners=5 argc=2 n=2000000 2.09 % ** 1.569820e-03
events/ee-emit.js listeners=5 argc=4 n=2000000 34.62 % *** 1.561699e-50

apapirovski · 2017-11-11T04:32:24Z

@mscdex PTAL

apapirovski · 2017-11-13T12:43:47Z

@mscdex or anyone else that reviewed this, PTAL. The "changes requested" needs to be dismissed before we can land this.

jasnell

Still LGTM

mscdex · 2017-11-13T23:16:50Z

We can make things faster for multiple listeners by adding some fast paths in arrayClone(). For example, this version of arrayClone() gives noticeable speed improvements for all array sizes:

functionarrayClone(arr,n){switch(n){case0: return[];case1: return[arr[0]];case2: return[arr[0],arr[1]];case3: return[arr[0],arr[1],arr[2]];case4: return[arr[0],arr[1],arr[2],arr[3]];case5: return[arr[0],arr[1],arr[2],arr[3],arr[4]];}// Not included in 'default' case because of perf issue with `const` in// a switch caseconstcopy=newArray(n);copy[0]=arr[0];copy[1]=arr[1];copy[2]=arr[2];copy[3]=arr[3];copy[4]=arr[4];copy[5]=arr[5];for(vari=6;i<n;++i)copy[i]=arr[i];returncopy;}

I chose 5 as the max array length for fast paths as that seems like a reasonable limit to me.

Also, 0 and 1-length arrays are supported for backwards compatibility, otherwise we could remove those cases since those kinds of arrays will not be generated internally (I'm thinking about modules that may directly mutate arrays in _events).

TimothyGu · 2017-11-13T23:24:25Z

@mscdex Isn't that a bit outside the scope of this PR specifically, considering this PR does not touch that piece of code?

mscdex · 2017-11-13T23:25:33Z

@TimothyGu No? To me this PR is about improving emit() performance (judging by the previously posted benchmark results). I just thought incorporating my suggestion would make those improvements more substantial since emit() is the sole user of arrayClone()...

Trott · 2017-11-14T00:21:39Z

To me this PR is about improving emit() performance

@mscdex I guess @apapirovski can say for sure. Personally, I thought it was about making the code more maintainable/understandable by removing CrankshaftScript optimizations that no longer provide significant benefit. The fact that it also improves (rather than merely preserves) performance may have been incidental.

I also prefer PRs to remain narrowly scoped on principle. If we put the arrayClone() suggestion into a separate PR, it means we can land it in older versions if it helps there too without having to land the stuff in this PR at the same time. Or if it causes unexpected problems, it can be backed out without having to back out these other changes. (I guess that's an argument for separate commits more than separate PRs. Still, putting it in a separate PR makes the code review more manageable, as well as following the conversation relevant to the code change.)

mscdex · 2017-11-14T00:50:40Z

If we put the arrayClone() suggestion into a separate PR, it means we can land it in older versions if it helps there too without having to land the stuff in this PR at the same time

From what I've seen, benchmarks are often not checked when backporting, which is especially important when going from TurboFan to Crankshaft.

Whatever though, I just thought I'd throw the perf improvement out there for anyone interested.

jasnell · 2017-11-14T01:44:00Z

I see no reason not to add @mscdex's suggestion as a second separate commit in this PR.

bmeurer · 2017-11-14T07:00:59Z

@mscdex These arrayClone optimizations don't look really appealing to me. How about avoiding the defensive copy upon emission completely and instead copy on mutation? Emission seems to be the more common case.

mscdex · 2017-11-14T07:14:16Z

@bmeurer I'm not sure what you're asking, but we've always made a copy up front in case the event handlers change during the execution of event handlers.

bmeurer · 2017-11-14T07:29:46Z

Right, but wouldn't it be possible to treat the handlers array as copy on write? Such that when iterating over them, you sort of take ownership of the handlers array and in case someone adds/removes handlers, you create a new Array instead of mutating the existing one.

TimothyGu · 2017-11-14T07:58:00Z

@bmeurer How would do you get notified when the array changes?

Trott · 2017-11-14T14:04:08Z

I could be wrong, but it sure looks like we may be spiraling off into a conversation about whether and how to implement the arrrayClone() part. So I'm going to repeat my suggestion that it be a separate PR so that conversation can happen separately and not stall the pretty-sure-we-all-agree-it's-a-good-thing-and-should-land changes that @apapirovski has proposed here already.

Trott · 2017-11-14T14:09:31Z

By the way, the only thing preventing this from landing at this point is the objection from @mscdex. So I guess the question is whether the objection has been effectively cleared by the benchmark fixes/changes that @apapirovski did in response? Or is there still an objection to landing this without the arrayClone() stuff as well?

apapirovski · 2017-11-14T18:43:12Z

Sorry, I haven't had time the past couple of days to address much of what's been discussed here. But as mentioned, the intent was mostly to remove "ugly" code that wasn't improving performance any longer. The speed up for emits with many arguments is a just a nice boon. (Further evidenced by the quote in the original: "With improvements in V8, using separate emit functions is no longer necessary. ")

Re: the ensuing discussion, I think @bmeurer has a valid point re: allocating new handler array when an event is attached as opposed to emitted. That's something that could be benchmarked and tested. I might look at it if I have time this week.

In the meantime, I'll be landing this shortly.

apapirovski · 2017-11-14T19:03:57Z

Landed in f44f18a

With improvements in V8, using separate emit functions is no longer necessary and can instead be replaced by the spread operator. improvement confidence p.value events/ee-emit.js n=2000000 2.98 % 0.09852489 events/ee-emit-2-args.js n=2000000 4.19 % *** 0.0001914216 events/ee-emit-6-args.js n=2000000 61.69 % *** 6.611964e-35 events/ee-emit-diff-args.js n=2000000 -0.36 % 0.305069 events/ee-once.js n=20000000 6.42 % *** 1.27831e-06 PR-URL: #16869 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

apapirovski added dont-land-on-v4.x events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js. labels Nov 7, 2017

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Nov 7, 2017

addaleax approved these changes Nov 7, 2017
View reviewed changes

cjihrig approved these changes Nov 7, 2017
View reviewed changes

refack approved these changes Nov 7, 2017
View reviewed changes

refack added the refactor to ES6+ label Nov 7, 2017

evanlucas approved these changes Nov 7, 2017
View reviewed changes

bengl approved these changes Nov 7, 2017
View reviewed changes

lpinca approved these changes Nov 7, 2017
View reviewed changes

TimothyGu approved these changes Nov 7, 2017
View reviewed changes

mathiasbynens reviewed Nov 7, 2017
View reviewed changes

fhinkel approved these changes Nov 8, 2017
View reviewed changes

bmeurer reviewed Nov 8, 2017
View reviewed changes

bmeurer approved these changes Nov 8, 2017
View reviewed changes

jasnell approved these changes Nov 8, 2017
View reviewed changes

apapirovski mentioned this pull request Nov 8, 2017
process: slightly simplify next tick execution #16888
Closed
2 tasks

mscdex suggested changes Nov 8, 2017
View reviewed changes

[fixup] more flexible emit benchmark
8b95f82

[fixup] do not bind or apply args
7ddf389

apapirovski force-pushed the patch-events-emit-perf branch from 210602d to 7ddf389Compare November 8, 2017 18:45

jasnell approved these changes Nov 13, 2017
View reviewed changes

mscdex approved these changes Nov 14, 2017
View reviewed changes

apapirovski closed this Nov 14, 2017

apapirovski deleted the patch-events-emit-perf branch November 14, 2017 19:04


		const ee = new EventEmitter();

		for (var k = 0; k < 10; k += 1)

Uh oh!

Uh oh!

Conversation

apapirovski commented Nov 7, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Nov 7, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Nov 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Nov 7, 2017

Uh oh!

mscdex commented Nov 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Nov 7, 2017

Uh oh!

lpinca commented Nov 7, 2017

Uh oh!

mathiasbynensNov 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnekNov 7, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovskiNov 7, 2017

Choose a reason for hiding this comment

Uh oh!

devsnekNov 7, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovskiNov 7, 2017

Choose a reason for hiding this comment

Uh oh!

bmeurerNov 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmeurer left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Nov 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Nov 8, 2017

Uh oh!

apapirovski commented Nov 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Nov 8, 2017

Uh oh!

apapirovski commented Nov 11, 2017

Uh oh!

apapirovski commented Nov 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Nov 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Nov 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Nov 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

apapirovski commented Nov 7, 2017•
edited
Loading

mscdex commented Nov 7, 2017•
edited
Loading

mathiasbynensNov 7, 2017•
edited
Loading

bmeurerNov 8, 2017•
edited
Loading

mscdex left a comment •
edited
Loading

apapirovski commented Nov 8, 2017•
edited
Loading

apapirovski commented Nov 8, 2017•
edited
Loading

apapirovski commented Nov 13, 2017•
edited
Loading

mscdex commented Nov 13, 2017•
edited
Loading

TimothyGu commented Nov 13, 2017•
edited
Loading

mscdex commented Nov 13, 2017•
edited
Loading

Trott commented Nov 14, 2017•
edited
Loading

mscdex commented Nov 14, 2017•
edited
Loading

apapirovski commented Nov 14, 2017•
edited
Loading