Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Conversation

@vkurchatkin
Copy link

Rework of #7158. Differences:

  • instead of extending existing _events, just check if they are an own property;
  • no performance impact in general case.

In case _events exists instead of !this.hasOwnProperty('_events') this._events === this.__proto__._events is used, which is slightly faster.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Vladimir Kurchatkin

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@bnoordhuis
Copy link
Member

Calling @trevnorris.

@vkurchatkin
Copy link
Author

ping @trevnorris

@TooTallNate
Copy link

Is Object.getPrototypeOf() a lot slower than this.__proto__? We try to avoid using __proto__ in node core, and I'm pretty sure that currently we're achieving that (sans this patch).

without this check there is chance to have `_events`
shared between instances.

fixes #7157
@vkurchatkin
Copy link
Author

Well, it appears like it's actually even faster. Updated.

btw __proto__ is still used here: https://github.com/joyent/node/blob/master/src/node.js#L33

@trevnorris
Copy link

Either way the patch makes the code path slower. Between 20-40ns on my i7. But since this is for a special case I'm not concerned about it. LGTM.

@trevnorris
Copy link

Merged in 2c6b424.

@trevnorris trevnorris closed this Apr 15, 2014
vkurchatkin added a commit to vkurchatkin/node that referenced this pull request Jun 23, 2015
2c6b424 fixed this problem for the case where
EventEmitter constructor is invoked. If is not, shared
_events were still used. This commit ensures that listeners
are added only to own _events.

See: nodejs/node-v0.x-archive#7405
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants