Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@lpinca
Copy link
Member

commented Mar 21, 2016

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().

events: make eventNames() use Reflect.ownKeys()
Use `Reflect.ownKeys()` instead of `Object.keys()` and
`Object.getOwnPropertySymbols()`.
@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2016

See discussion on #5818.

@lpinca lpinca referenced this pull request Mar 21, 2016

Closed

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

2 of 4 tasks complete

@mscdex mscdex added the events label Mar 21, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

I believe this would be our first use of Reflect in core ;-) I'd actually forgotten that this was available now. LGTM

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

@mscdex using your benchmark, I've tried adding symbol properties to the _events object (I've also reduced the number of runs from 500000 to 50000 because it was taking too much time on my prehistoric machine):

'use strict';

var common = require('../common.js');
var EE = require('events');
var v8 = require('v8');

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

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

  var ee = new EE();

  for (var k = 0; k < 1000; k += 1) {
    ee.on('dummy' + k, function() {});
    ee.on(Symbol(), 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 are interesting:

macbook:node luigi$ ./node -pe "require('events').prototype.eventNames.toString()"
function eventNames() {
  return this._eventsCount > 0 ? Reflect.ownKeys(this._events) : [];
}
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=50000: ./node: 1923.3 ./node-pre: 1543.4 . 24.61%

I think symbols are not widely used as event names but maybe it is still something to consider.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2016

@lpinca I'm not too worried about that as I'd rather take a -24% hit on an edge case like that than a -89% hit in a more common case.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

Yeah ofc.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

Anyway, is having a fixed set of event names the most common use case? Maybe, I don't know, but in that case why would I need to run eventNames() more than once? I would just call it once and reuse the returned array If I'm sure that it will not change.

We should add a benchmark to compare the two solution when events are added or removed between eventNames() calls.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

Does it make sense to add a new event with a given probability in the for loop?

for (var i = 0; i < n; i += 1) {
  if (Math.random() < 0.1)
    ee.on('foo' + i, function() {});

  ee.eventNames().length;
}

If so, I get these results with the above change (no symbols on initialization):

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=50000: ./node: 1033.3 ./node-pre: 866.81 . 19.21%
@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

Well, Object.keys caches the result in an array https://github.com/v8/v8/blob/master/src/builtins.cc#L1768-L1799 .

It might be fair to cache the result in a WeakMap on our side if we want performance parity (and probably better) but honestly I'm not really sure it's worth it since I don't think anyone is assuming it - that is, I don't see any code doing:

for(var i = 0; i < Object.keys(o).length ; i++) {
    doSomethingWith(Object.keys(o)[i])

I see plenty of people doing:

const keys = Object.keys(o);
for(var i = 0; i < keys.length; i++) {
    doSomethingWith(keys[i])

It's worth mentioning that from v8's side this optimization is entirely reasonable for objects that are not used as dictionaries or mutate very rarely - this is not the case here.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

That said - if other collaborators feel like the use case is common enough or don't feel entirely comfortable with this change in terms of performance - I'm also fine with leaving things the way they are.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

@mscdex your opinion would be appreciated here - I want to either land or close this and to do either we need consensus :)

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

Can you guys give a comment on #5822 (comment)?
Does it invalidate the benchmark? If it doesn't, a little chance like 1% of having a new event added will make the Reflect.ownKeys() solution on par with or better than the Object.keys() solution.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

@lpinca in my opinion the benchmark isn't indicative of usage because when people run over the keys they always cache it - I've literally never seen anyone write:

for(var i = 0; i < Object.keys(o).length ; i++) {
    doSomethingWith(Object.keys(o)[i])

And lots of times people write:

const keys = Object.keys(o);
for(var i = 0; i < keys.length; i++) {
    doSomethingWith(keys[i])

Which means that the reason Object.keys is faster (caching of the structure at the v8 level) is irrelevant.

However, I acknowledge I'm not aware of every use case - and @mscdex has objected to the performance regression before so I want to know if the new code (and benchmarks) satisfy him. Stylistically I like the change.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

Stylistically, LGTM. But if the performance hit is really 89%, then -1.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

@cjihrig the performance hit is ~19% in the benchmark above. I think in practice it's probably a lot less than that.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

@cjihrig please take a look at the benchmark where perf is -89%. It compares two implementations where one caches result (internally) and one don't. It's pretty obvious that the former wins.

What @benjamingr is saying, I guess, is that realistically if you know beforehand that keys will not change there is no point in calling eventNames() again as you will cache the result yourself:

const events = ee.eventNames();

If keys will change, then this implementation is faster. With the above benchmark, new keys are added with a probability of 10% and this make the implementation in question 19% faster.
It is still faster if keys are added with a probability of 1%.

To reiterate, I'm not sure if this invalidate the original benchmark @mscdex wrote. Also I'm obviously biased here.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

I'll defer to @mscdex here. I'm all for simplifying the code.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

Out of curiosity, what is the performance like (both with the .eventNames() and other existing benchmarks) with a "mutated" flag that gets set in .on()/.once() and gets checked and unset in .eventNames()?

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

I guess it really depends on how frequently this flag is set/unset.
If it is never set then perf will be on par with or more likely better than the Object.keys() solution.
If it is set/unset every time then perf will be on par with the Reflect.ownKeys() solution.

The question is, how frequently would you set this "mutated" flag to make the benchmark realistic?

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

@mscdex ... any further thoughts on this one?

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2016

@jasnell No.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

@mscdex ... I'm trying to parse the conversation around this... where did this change end up in terms of performance impact? Is it faster or slower than the current code?

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

I'll sum up the discussion:

  • It is slower if the _events object is not mutated, faster if it is.
  • Mutating the _events object with a probability of 1% during the benchmark makes this solution faster.
  • If the _events object does not mutate, the result can be cached const events = ee.eventNames();
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Given the discussion I'm inclined to leave the code as it is currently and revisiting this later if necessary. As a new API, this is not yet in broad use yet, as such, the perf impact is going to be hard to measure. Let's hold off and let this circulate in the ecosystem a bit longer before deciding if/how/when to optimize it.

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

I guess it's hard to say how this new API will be used but I think in the most common use case this method will only be called once making the current and proposed solutions equivalent in terms of perfomance.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

I tend to agree. Anyone wanting to track mutations already have events they can listen to. It's unlikely that this method will be in very many hot code paths.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

@lpinca ... how about we close this PR for now and revisit it again later if necessary?

@lpinca

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

@jasnell I don't mind but in that case why don't we go for this one-liner?

Advantages:

  1. same perf on the most common use case
  2. better perf on mutations
  3. better readability

Disadvanges:

  1. slower when called multiple times on an object that does not mutate (anyone with a bit of intelligence would cache the result in this case and not call the method again)

I would like to have more people weigh in, but again feel free to close it.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Yeah, you're right. There's no harm and it's less code ;-)

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

@cjihrig ... does this still LGTY?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Yea, I suppose so.

jasnell added a commit that referenced this pull request Apr 21, 2016

events: make eventNames() use Reflect.ownKeys()
Use `Reflect.ownKeys()` instead of `Object.keys()` and
`Object.getOwnPropertySymbols()`.

PR-URL: #5822
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Landed in c1cd644

@jasnell jasnell closed this Apr 21, 2016

@lpinca lpinca deleted the lpinca:refactor/ee-event-names branch Apr 21, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

events: make eventNames() use Reflect.ownKeys()
Use `Reflect.ownKeys()` instead of `Object.keys()` and
`Object.getOwnPropertySymbols()`.

PR-URL: nodejs#5822
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

events: make eventNames() use Reflect.ownKeys()
Use `Reflect.ownKeys()` instead of `Object.keys()` and
`Object.getOwnPropertySymbols()`.

PR-URL: #5822
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.