events: don't keep arrays with a single listener #12043

lpinca · 2017-03-25T21:58:12Z

Use the remaining listener directly if the array of listeners has only one element after running EventEmitter.prototype.removeListener().

Advantages:

Better memory usage and better performance if no new listeners are added for the same event.

Disadvantages:

A new array must be created if new listeners are added for the same event.

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)

events

mscdex · 2017-03-26T01:40:24Z

I fear this could cause userland issues like when I tried to change the internal storage structure when optimizing .once() a long while back. There's quite a few people that access _events externally.

lpinca · 2017-03-26T06:32:59Z

@mscdex care to elaborate?

This doesn't change internal structures, it keeps them consistent (1 listener - no array, 2+ listeners - array). If people use ee._events they have to check if ee._events.whatever is an array or not regardless of this change.

ronkorving · 2017-03-28T08:18:47Z

lib/events.js

-        do {
-          this.removeListener(type, listeners[listeners.length - 1]);
-        } while (listeners[0]);
+        for (i = listeners.length; i-- > 0;) {


I'm not a big fan of this notation tbh. Why not move the -- operation to the 3rd chunk of the for-structure?

I've used that because the same is done few lines above: https://github.com/nodejs/node/pull/12043/files#diff-71dcd327d0ca067b490b22d677f81966R366.

Oh wow. Would love to see some feedback from one or two other @nodejs/collaborators on this. You're re-using this pattern here (where it wasn't here before). Do we really want to embrace that pattern or move away from it?

I agree, this pattern is hard to understand. Let's change it in both places to something simpler.

Looks like this is the only place in core we use this pattern (although it appears a few times in v8 c++ code. It was added in 3e64b56. I think we should remove this and just use the standard for loop style.

➜ rg "i[-|+]+ [<|>]" | grep for lib/events.js: for (i = list.length; i-- > 0;) { deps/v8/src/deoptimizer.cc: for (size_t i = count; i-- > 0;) { deps/v8/src/ast/ast-types.cc: for (size_t i = BoundariesSize() - 1; i-- > 0;) { deps/v8/src/full-codegen/full-codegen.cc: for (int i = last; i-- > 0;) { deps/v8/src/compiler/types.cc: for (size_t i = BoundariesSize() - 1; i-- > 0;) { deps/v8/src/parsing/parser.cc: for (int i = labels->length(); i-- > 0;) { deps/openssl/openssl/crypto/store/str_lib.c: for (i = 0; i++ < STORE_ATTR_TYPE_NUM;)

🔧 in ca87422.

Thanks guys 👍

FWIW the same pattern is also used here

node/benchmark/events/ee-add-remove.js

Line 19 in 678480e

for (k = listeners.length; --k >= 0; /* empty */)

mcollina · 2017-03-28T09:56:51Z

Does this affect performance? Can you please run https://github.com/nodejs/node/tree/master/benchmark/events?

lpinca · 2017-03-28T12:30:24Z

@mcollina yes will do as soon as my internet connection will start working again.

lpinca · 2017-03-28T16:26:06Z

$ node benchmark/compare.js --old ./node-master --new ./node-pr-12043 events > compare-pr-12043.csv
[00:13:25|% 100| 7/7 files | 60/60 runs | 1/1 configs]: Done
$ cat compare-pr-12043.csv | Rscript benchmark/compare.R

                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                         0.42 %            4.469925e-01
 events/ee-emit-multi-args.js n=2000000                   0.13 %            8.361339e-01
 events/ee-emit.js n=2000000                              1.76 %          * 2.385787e-02
 events/ee-listener-count-on-prototype.js n=50000000     -0.75 %          * 1.356947e-02
 events/ee-listeners-many.js n=5000000                    0.21 %            3.401062e-01
 events/ee-listeners.js n=5000000                         2.10 %        *** 4.978391e-08
 events/ee-once.js n=20000000                             4.05 %        *** 1.101861e-41

mcollina

LGTM, can you add that we get some speedup in the commit message?

lpinca · 2017-03-28T19:26:23Z

@mcollina done, I've only mentioned the events/ee-once.js benchmark as I can't really see how this change could make a difference in events/ee-listeners.js or events/ee-emit.js.

mscdex · 2017-03-28T19:48:16Z

It might be better to add some new benchmarks and refer to the results of those since the current benchmarks should not be affected by this PR's changes (except maybe ee-add-remove).

lpinca · 2017-03-28T20:29:17Z

Agreed. ee-add-remove is actually a good benchmark for these changes. I'll remove the misleading info from commit message.

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event.

lpinca · 2017-04-03T19:37:33Z

This ad-hoc benchmark shows no difference in performance between master and this PR.

'use strict';
var common = require('../common.js');
var events = require('events');

var bench = common.createBenchmark(main, {n: [1e6]});

function main(conf) {
  var n = conf.n | 0;

  var ee = new events.EventEmitter();

  function listener1() {}
  function listener2() {}

  ee.on('removeListener', function() {});

  bench.start();
  for (var i = 0; i < n; i++) {
    ee.on('dummy', listener1);
    ee.on('dummy', listener2);
    ee.removeListener('dummy', listener2);
    ee.emit('dummy', 'arg');
    ee.on('dummy', listener2);
    ee.removeAllListeners('dummy');
  }
  bench.end(n);
}

Taking the best result over multiple runs I get

events/pr-12043.js n=1000000: 1,189,260.6881846066

for master and

events/pr-12043.js n=1000000: 1,186,814.6173030164

for this PR.

lpinca · 2017-04-03T19:39:31Z

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

ronkorving · 2017-04-04T02:38:34Z

@lpinca Then I guess one of the advantages mentioned in the description of this PR (performance) is not really valid. I think the consistency argument is valid though.

lpinca · 2017-04-04T05:49:41Z

@ronkorving well if I craft the benchmark to only remove listener2 and emit the event there is indeed a minimal performance gain and better memory usage (I made this PR for this particular case, see #11868 (comment)).

The benchmark also readd listener2 which is the "disadvantage" mentioned in commit message and then remove all listeners to see the overall impact.

ronkorving · 2017-04-04T06:07:19Z

@lpinca Oh wow, interesting 👍

lpinca · 2017-04-04T10:14:53Z

If there are no objections I'll land this tomorrow.

cc: @nodejs/collaborators

ronkorving · 2017-04-04T10:23:32Z

LGTM 👍

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event. PR-URL: #12043 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl>

jasnell · 2017-04-04T17:40:07Z

Landed in 8d386ed

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event. PR-URL: nodejs#12043 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl>

MylesBorins · 2017-04-18T23:37:03Z

looks like something we may want on v6.x, want to give it a bit more time to bake first

Commit 8d386ed stopped the Event Emitter implementation from storing arrays containing a single listener. This change left a section of code in removeListener() as unreachable. This commit removes the unreachable code. Refs: nodejs#12043 PR-URL: nodejs#12501 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn · 2017-06-18T16:50:50Z

@lpinca Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

#12501 should probably be backported in the same PR.

Commit 8d386ed stopped the Event Emitter implementation from storing arrays containing a single listener. This change left a section of code in removeListener() as unreachable. This commit removes the unreachable code. Refs: nodejs#12043 PR-URL: nodejs#12501 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event. PR-URL: #12043 Backport-PR-URL: #13796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl>

Commit 8d386ed stopped the Event Emitter implementation from storing arrays containing a single listener. This change left a section of code in removeListener() as unreachable. This commit removes the unreachable code. Refs: #12043 PR-URL: #12501 Backport-PR-URL: #13796 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event. PR-URL: #12043 Backport-PR-URL: #13796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl>

Commit 8d386ed stopped the Event Emitter implementation from storing arrays containing a single listener. This change left a section of code in removeListener() as unreachable. This commit removes the unreachable code. Refs: #12043 PR-URL: #12501 Backport-PR-URL: #13796 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use the remaining listener directly if the array of listeners has only one element after running `EventEmitter.prototype.removeListener()`. Advantages: - Better memory usage and better performance if no new listeners are added for the same event. Disadvantages: - A new array must be created if new listeners are added for the same event. PR-URL: nodejs/node#12043 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl>

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

jasnell approved these changes Mar 26, 2017

View reviewed changes

ronkorving reviewed Mar 28, 2017

View reviewed changes

mcollina approved these changes Mar 28, 2017

View reviewed changes

lpinca force-pushed the avoid/single-listener-arrays branch from ca87422 to 8979815 Compare March 28, 2017 19:22

lpinca force-pushed the avoid/single-listener-arrays branch from 8979815 to ac61a0a Compare March 28, 2017 20:33

lpinca force-pushed the avoid/single-listener-arrays branch from ac61a0a to 1b027c1 Compare April 3, 2017 19:04

watilde approved these changes Apr 4, 2017

View reviewed changes

jasnell closed this Apr 4, 2017

lpinca deleted the avoid/single-listener-arrays branch April 4, 2017 18:32

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the lts-watch-v6.x label Apr 18, 2017

cjihrig mentioned this pull request Apr 19, 2017

events: remove unreachable code #12501

Merged

2 tasks

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 15, 2017

gibfahn added backport-requested-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Jun 18, 2017

gibfahn mentioned this pull request Jun 18, 2017

test: verify listener leak is only emitted once #12502

Merged

3 tasks

lpinca mentioned this pull request Jun 19, 2017

(v6.x backport) events: do not keep arrays with a single listener #13796

Closed

gibfahn added backported-to-v6.x and removed backport-requested-v6.x labels Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: don't keep arrays with a single listener #12043

events: don't keep arrays with a single listener #12043

lpinca commented Mar 25, 2017 •

edited

Loading

mscdex commented Mar 26, 2017 •

edited

Loading

lpinca commented Mar 26, 2017 •

edited

Loading

ronkorving Mar 28, 2017

lpinca Mar 28, 2017 •

edited

Loading

ronkorving Mar 28, 2017 •

edited

Loading

seishun Mar 28, 2017 •

edited

Loading

gibfahn Mar 28, 2017

lpinca Mar 28, 2017

ronkorving Mar 29, 2017

lpinca Mar 29, 2017

mcollina commented Mar 28, 2017

lpinca commented Mar 28, 2017

lpinca commented Mar 28, 2017

mcollina left a comment

lpinca commented Mar 28, 2017

mscdex commented Mar 28, 2017 •

edited

Loading

lpinca commented Mar 28, 2017 •

edited

Loading

lpinca commented Apr 3, 2017

lpinca commented Apr 3, 2017

ronkorving commented Apr 4, 2017 •

edited

Loading

lpinca commented Apr 4, 2017 •

edited

Loading

ronkorving commented Apr 4, 2017

lpinca commented Apr 4, 2017

ronkorving commented Apr 4, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017 •

edited

Loading

gibfahn commented Jun 18, 2017

events: don't keep arrays with a single listener #12043

events: don't keep arrays with a single listener #12043

Conversation

lpinca commented Mar 25, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Mar 26, 2017 • edited Loading

lpinca commented Mar 26, 2017 • edited Loading

ronkorving Mar 28, 2017

Choose a reason for hiding this comment

lpinca Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

ronkorving Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

seishun Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn Mar 28, 2017

Choose a reason for hiding this comment

lpinca Mar 28, 2017

Choose a reason for hiding this comment

ronkorving Mar 29, 2017

Choose a reason for hiding this comment

lpinca Mar 29, 2017

Choose a reason for hiding this comment

mcollina commented Mar 28, 2017

lpinca commented Mar 28, 2017

lpinca commented Mar 28, 2017

mcollina left a comment

Choose a reason for hiding this comment

lpinca commented Mar 28, 2017

mscdex commented Mar 28, 2017 • edited Loading

lpinca commented Mar 28, 2017 • edited Loading

lpinca commented Apr 3, 2017

lpinca commented Apr 3, 2017

ronkorving commented Apr 4, 2017 • edited Loading

lpinca commented Apr 4, 2017 • edited Loading

ronkorving commented Apr 4, 2017

lpinca commented Apr 4, 2017

ronkorving commented Apr 4, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017 • edited Loading

gibfahn commented Jun 18, 2017

lpinca commented Mar 25, 2017 •

edited

Loading

mscdex commented Mar 26, 2017 •

edited

Loading

lpinca commented Mar 26, 2017 •

edited

Loading

lpinca Mar 28, 2017 •

edited

Loading

ronkorving Mar 28, 2017 •

edited

Loading

seishun Mar 28, 2017 •

edited

Loading

mscdex commented Mar 28, 2017 •

edited

Loading

lpinca commented Mar 28, 2017 •

edited

Loading

ronkorving commented Apr 4, 2017 •

edited

Loading

lpinca commented Apr 4, 2017 •

edited

Loading

MylesBorins commented Apr 18, 2017 •

edited

Loading