events: make eventNames() use Reflect.ownKeys()

[lpinca]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

Affected core subsystem(s)

events

Description of change

Use Reflect.ownKeys() instead of Object.keys() and Object.getOwnPropertySymbols().

[lpinca]

Consistency really triggers my OCD.

Object.getOwnPropertyNames() is also a little faster:

'use strict';

const benchmark = require('benchmark');

const o = {};

for (var i = 0; i < 10000; i++) o[i] = i;

const suite = new benchmark.Suite;

suite.add('Object.getOwnPropertyNames()', function () {
  Object.getOwnPropertyNames(o);
});
suite.add('Object.keys()', function () {
  Object.keys(o);
});
suite.on('cycle', function (event) {
  console.log(event.target.toString());
});
suite.on('complete', function () {
  console.log('Fastest is '+ this.filter('fastest').map('name'));
});

suite.run();

Object.getOwnPropertyNames() x 3,016 ops/sec ±1.03% (85 runs sampled)
Object.keys() x 2,721 ops/sec ±1.10% (86 runs sampled)
Fastest is Object.getOwnPropertyNames()

[lpinca]

Previous discussion in #5817.

[benjamingr]

I'm +1 on this since it guarantees that the order of the properties returned is preserved between versions of v8. Object.keys has no order guarantee in the ES2015 spec where Object.getOwnPropertyNames is guaranteed to return properties in a specific order which coincides with what we currently have.

LGTM

[benjamingr]

Also note that your benchmark isn't great because it measures only numeric properties. Not that it's a big issue IMO.

[lpinca]

Updated benchmark with:

for (let i = 0; i < 10000; i++) {
  o[i % 2 === 0 ? i : `foo${i}`] = i;
}

Object.getOwnPropertyNames() x 1,685 ops/sec ±1.34% (84 runs sampled)
Object.keys() x 7.39 ops/sec ±1.11% (22 runs sampled)
Fastest is Object.getOwnPropertyNames()

[benjamingr]

Wouldn't it make sense to just call Reflect.ownKeys instead of calling both Object.getOwnPropertyNames and then Object.getOwnPropertySymbols and concat them?

Reflect.ownKeys returns both symbol and non-symbol keys - it's already with guaranteed iteration order in OwnPropertyKeys and both keys and getOwnPropertyNames uses the same JSReceiver::GetKeys(receiver, OWN_KEYS only one gets the strings and the other only symbols.

Your new benchmark is also really strange since it checks an object with 10K properties and Object.keys looks like it uses some caching internally. To reiterate I do not think performance is a concern here anyway.

(code in https://github.com/v8/v8/blob/master/src/builtins.cc )

[lpinca]

Sounds good, I didn't know about Reflect.ownKeys().

[mscdex]

I'm getting quite the opposite results when testing with this benchmark on master:

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

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

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

  var ee = new EventEmitter();

  for (var k = 0; k < 1000; k += 1) {
    ee.on('dummy' + k, function() {});
  }

  // Force optimization before starting the benchmark
  ee.eventNames().length;
  v8.setFlagsFromString('--allow_natives_syntax');
  eval('%OptimizeFunctionOnNextCall(ee.eventNames)');
  ee.eventNames().length;

  bench.start();
  for (var i = 0; i < n; i += 1) {
    ee.eventNames().length;
  }
  bench.end(n);
}

Results:

mscdex@mscdexPC:~/git/node$ node benchmark/compare.js -r -g ./node ./node-pre-eventnames -- events ee-eventnames
running ./node
events/ee-eventnames.js
running ./node-pre-eventnames
events/ee-eventnames.js

events/ee-eventnames.js n=500000: ./node: 14515 ./node-pre-eventnames: 111260 . -86.95%

mscdex@mscdexPC:~/git/node$ ./node-pre-eventnames -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  if (this._eventsCount > 0) {
    const events = this._events;
    return Object.keys(events).concat(
      Object.getOwnPropertySymbols(events));
  }
  return [];
}
mscdex@mscdexPC:~/git/node$ ./node -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  if (this._eventsCount > 0) {
    const events = this._events;
    return Object.getOwnPropertyNames(events).concat(
      Object.getOwnPropertySymbols(events));
  }
  return [];
}

and with Reflect.ownKeys():

mscdex@mscdexPC:~/git/node$ node benchmark/compare.js -r -g ./node ./node-pre-eventnames -- events ee-eventnames
running ./node
events/ee-eventnames.js
running ./node-pre-eventnames
events/ee-eventnames.js

events/ee-eventnames.js n=500000: ./node: 16227 ./node-pre-eventnames: 111850 . -85.49%

mscdex@mscdexPC:~/git/node$ ./node -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  if (this._eventsCount > 0)
    return Reflect.ownKeys(this._events);
  return [];
}

[benjamingr]

Yeah those benchmarks look a lot more in line with what the code looks like, to be fair that benchmark isn't very indicative of real usage - where the results would mutate between calls and the optimization would not always work.

[lpinca]

Just to be on the same boat, I ran the bechmark on node 5.9.0.

[mscdex]

@lpinca node v5.x uses an older branch of v8 (4.6) whereas master uses 4.9 (and will use 5.0 when node v6 is released). So there could very well be changes that affected performance.

[lpinca]

Yes my bad for running it on 5.9.0, making it irrelevant.

[lpinca]

I get this using master:

Object.getOwnPropertyNames() x 596 ops/sec ±1.02% (82 runs sampled)
Object.keys() x 565 ops/sec ±1.05% (83 runs sampled)
Fastest is Object.getOwnPropertyNames()

[mscdex]

@lpinca But did you try running the benchmark I posted? It's always best to verify changes like this end-to-end because it's not always easy to guess what v8 will do in certain circumstances or if the change will matter in the grand scheme of things.

[lpinca]

@mscdex yes I totally understand and with your benchmark I get your same results:

macbook:node luigi$ ./node -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  if (this._eventsCount > 0) {
    const events = this._events;
    return Object.getOwnPropertyNames(events).concat(
      Object.getOwnPropertySymbols(events));
  }
  return [];
}
macbook:node luigi$ ./node-pre -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  if (this._eventsCount > 0) {
    const events = this._events;
    return Object.keys(events).concat(
      Object.getOwnPropertySymbols(events));
  }
  return [];
}
macbook:node luigi$ node benchmark/compare.js -r -g ./node ./node-pre -- events ee-eventnames
running ./node
events/ee-eventnames.js
running ./node-pre
events/ee-eventnames.js

events/ee-eventnames.js n=500000: ./node: 7583.8 ./node-pre: 80453 . -90.57%

[lpinca]

I've updated the pr to use Reflect.ownKeys(). Feel free to close it if performance is too bad.

[mscdex]

@lpinca FWIW I get about the same amount of performance degradation using just Reflect.ownKeys().

[lpinca]

@mscdex yeah, same here and ~100% is significant haha. Do whatever you think it's better with this.

[lpinca]

It doesn't make sense, it's too slow compared to the current implementation, so I close this.
Everything is good with the current implementation, but my brain is still unhappy for having own & enumerable properties and own only symbol properties haha.

[benjamingr]

Why did you close the PR? The benchmark is skewed since .keys uses caching but in practice objects are mutated between calls..

[lpinca]

@benjamingr reopened as #5822, I didn't see #5818 (comment).

[mscdex]

@benjamingr It's still a valid use case that I don't think is exactly uncommon and as such it's pretty hard to ignore that kind of regression. It might be a different story if the existing solution performs worse for a "mutated" _events than Reflect.ownKeys() or whatever other solution.

[jasnell]

The Relect.ownKeys() approach seems to work well. One possible optimization we could perform is to cache the result in EventEmitter and only refresh if the object is mutated but personally I think that's a bit premature.

[benjamingr]

@mscdex to be clear - this is an entirely valid use case and I am not suggesting we ignore that kind of regression.

I just didn't realize why it was closed before we talked about it and made sure the benchmark is good, and there is no way to fix it that's simple - but that has been clarified.

Apologies if it sounded like I was presenting it as a non-issue.