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: avoid emit() eager deopt #10568

Closed
wants to merge 2 commits into from

Conversation

vhf
Copy link
Contributor

@vhf vhf commented Jan 1, 2017

This commit makes sure EventEmitter.emit() doesn't get deoptimized by V8.
The deopt happens when accessing out of bound indexes of the arguments
object.

This issue has been raised here: #10323
and this specific case might become a more serious performance issue in
upcoming V8 releases.

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
  • @nodejs/v8

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. lts-watch-v6.x labels Jan 1, 2017
@@ -148,7 +148,7 @@ EventEmitter.prototype.emit = function emit(type) {

// If there is no 'error' event listener then throw.
if (doError) {
er = arguments[1];
er = arguments.length <= 1 ? undefined : arguments[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather just see an if like:

if (arguments.length > 1)
  er = arguments[1];

since er is undefined by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks!

@vhf vhf force-pushed the fix-eventemitter-emit-deopt branch from 843b3d3 to 2c64dd8 Compare January 1, 2017 22:49
@vhf vhf force-pushed the fix-eventemitter-emit-deopt branch from 2c64dd8 to 3d39b08 Compare January 1, 2017 23:17
@@ -6,7 +6,7 @@ const CLI = require('./_cli.js');

const cli = CLI(`usage: ./node run.js [options] [--] <category> ...
Run each benchmark in the <category> directory a single time, more than one
<categoty> directory can be specified.
<category> directory can be specified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spelling correction seems valid, but unrelated to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, unrelated. Would you prefer this PR not to include this correction?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it's in an entirely different file, I'd prefer that it get its own PR, yeah. (But I wouldn't stop this from being approved over it.)

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But the typo fix should be a separate commit (same PR is probably OK though).

vhf added 2 commits January 2, 2017 22:28
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: nodejs#10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.
@vhf vhf force-pushed the fix-eventemitter-emit-deopt branch from 3d39b08 to 8db2546 Compare January 2, 2017 21:33
@vhf
Copy link
Contributor Author

vhf commented Jan 2, 2017

Thanks for your comments. I split the two modifications in two commits.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 2, 2017

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Jan 3, 2017
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell pushed a commit that referenced this pull request Jan 5, 2017
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: #10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.

PR-URL: #10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell pushed a commit that referenced this pull request Jan 5, 2017
PR-URL: #10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member

jasnell commented Jan 5, 2017

Landed in e52fee5...da71d48

@jasnell jasnell closed this Jan 5, 2017
@vhf vhf deleted the fix-eventemitter-emit-deopt branch January 8, 2017 23:29
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: nodejs#10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.

PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: nodejs#10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.

PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: nodejs#10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.

PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
This commit makes sure EventEmitter.emit() doesn't get deoptimized by
V8. The deopt happens when accessing out of bound indexes of the
`arguments` object.

This issue has been raised here: nodejs#10323 and this specific case might
become a more serious performance issue in upcoming V8 releases.

PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
PR-URL: nodejs#10568
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@MylesBorins
Copy link
Contributor

Thoughts re: backport?

@MylesBorins
Copy link
Contributor

ping

@benjamingr
Copy link
Member

@MylesBorins this looks pretty simple to backport although not particularly important. Any reason not to?

@MylesBorins
Copy link
Contributor

@benjamingr want to confirm that there are not potential weird edges... been seeing failures on master due to emit changes

@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed lts-watch-v4.x labels May 8, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.