events: use nullish coalencing operator

[VoltrexKeyva]

Could use this here instead of checking for all falsy values, a little bit more readable as well.

[mscdex]

I think a benchmark that utilizes 'newListener' events should be added to this PR so we can ensure that this change doesn't result in any performance regressions like we've recently seen with other new javascript operators (e.g. a0261d2).

[VoltrexKeyva]

I think a benchmark that utilizes 'newListener' events should be added to this PR so we can ensure that this change doesn't result in any performance regressions like we've recently seen with other new javascript operators (e.g. a0261d2).

How may I add that? Sorry I've never worked with Node.js benchmarks before.

[mscdex]

Just create a new file in benchmarks/events. You can use one of the existing benchmark files in there as a guide/starting point.

[VoltrexKeyva]

Just create a new file in benchmarks/events. You can use one of the existing benchmark files in there as a guide/starting point.

Done, I'm not sure if that's how I'm supposed to do it though.

[benjamingr]

@VoltrexMaster https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md :)

[VoltrexKeyva]

@VoltrexMaster https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md :)

What I wrote seems to be working perfectly, no idea what else am I supposed to do.

[jasnell]

The basic change LGTM if there's not a significant perf regression. I did not look at the benchmark code.

[mscdex]

@VoltrexMaster There is no need to emit 'newListener' manually in the benchmark as node core is already doing that and that's what we're wanting to benchmark. At this point, I think it's probably easiest to just modify the "ee-add-remove" benchmark to include a (new) boolean parameter (0 or 1) that indicates whether a 'newListener' handler will be added to the emitter before the timed loop. That should be all we need to be able to benchmark this code path.

[VoltrexKeyva]

@VoltrexMaster There is no need to emit 'newListener' manually in the benchmark as node core is already doing that and that's what we're wanting to benchmark. At this point, I think it's probably easiest to just modify the "ee-add-remove" benchmark to include a (new) boolean parameter (0 or 1) that indicates whether a 'newListener' will be added to the emitter before the timed loop. That should be all we need to be able to benchmark this code path.

By "before the timed loop" do you mean before where the benchmark actually starts? (bench.start()).

[benjamingr]

Thank you for this! Only putting 'request changes' so this doesn't land without benchmarks showing no performance regression. Code looks good otherwise 🙏

[mscdex]

By "before the timed loop" do you mean before where the benchmark actually starts? (bench.start()).

Yes.

[VoltrexKeyva]

@mscdex the "(new) boolean parameter (0 or 1) that indicates whether a 'newListener' handler will be added to the emitter" kinda confused me, parameter where exactly or what kind of handler? Can you please elaborate on that?

[mscdex]

@VoltrexMaster ee-add-remove would look like this, using the current copy from the master branch (I've also added a parameter for testing 'removeListener' handlers):

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

const bench = common.createBenchmark(main, {
  newlistener: [0, 1],
  removelistener: [0, 1],
  n: [1e6],
});

function main({ newlistener, removelistener, n }) {
  const ee = new events.EventEmitter();
  const listeners = [];

  for (let k = 0; k < 10; k += 1)
    listeners.push(() => {});

  if (newlistener === 1)
    ee.on('newListener', (event, listener) => {});

  if (removelistener === 1)
    ee.on('removeListener', (event, listener) => {});

  bench.start();
  for (let i = 0; i < n; i += 1) {
    const dummy = (i % 2 === 0) ? 'dummy0' : 'dummy1';
    for (let k = listeners.length; --k >= 0; /* empty */) {
      ee.on(dummy, listeners[k]);
    }
    for (let k = listeners.length; --k >= 0; /* empty */) {
      ee.removeListener(dummy, listeners[k]);
    }
  }
  bench.end(n);
}

[VoltrexKeyva]

@VoltrexMaster ee-add-remove would look like this, using the current copy from the master branch (I've also added a parameter for testing 'removeListener' handlers):

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

const bench = common.createBenchmark(main, {
  newlistener: [0, 1],
  removelistener: [0, 1],
  n: [1e6],
});

function main({ newlistener, removelistener, n }) {
  const ee = new events.EventEmitter();
  const listeners = [];

  for (let k = 0; k < 10; k += 1)
    listeners.push(() => {});

  if (newlistener === 1)
    ee.on('newListener', (event, listener) => {});

  if (removelistener === 1)
    ee.on('removeListener', (event, listener) => {});

  bench.start();
  for (let i = 0; i < n; i += 1) {
    const dummy = (i % 2 === 0) ? 'dummy0' : 'dummy1';
    for (let k = listeners.length; --k >= 0; /* empty */) {
      ee.on(dummy, listeners[k]);
    }
    for (let k = listeners.length; --k >= 0; /* empty */) {
      ee.removeListener(dummy, listeners[k]);
    }
  }
  bench.end(n);
}

Oh thanks, I didn't know what those properties on the createBenchmark() method does but now I do, adding that rn.

[mscdex]

Sorry, I meant you can just update the ee-add-remove.js benchmark file instead of creating a new one.

[VoltrexKeyva]

Sorry, I meant you can just update the ee-add-remove.js benchmark file instead of creating a new one.

Oh, how you said it made me think you meant to create a new one with copying and modifying it, Imma delete this one and edit the existing one then 😁

[mscdex]

Benchmark CI results seem to look ok, although I'm not sure why the newListener=0 and removeListener=0 case for the ee-add-remove benchmark shows an improvement.

[VoltrexKeyva]

Benchmark CI results seem to look ok, although I'm not sure why the newListener=0 and removeListener=0 case for the ee-add-remove benchmark shows an improvement.

¯\_(ツ)_/¯

[nodejs-github-bot]

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

[jasnell]

Landed in 6e6663b