Skip to content

events: avoid emit() eager deopt #10568

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

events: avoid emit() eager deopt #10568

wants to merge 2 commits into from

Conversation

@vhf
Copy link
Contributor

@vhf vhf commented Jan 1, 2017 •
edited
Loading

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
mscdex
mscdex reviewed Jan 1, 2017
View reviewed changes
lib/events.js Outdated
Copy link
Contributor

@mscdex mscdex Jan 1, 2017

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

@vhf vhf Jan 1, 2017

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
Trott
Trott reviewed Jan 2, 2017
View reviewed changes
benchmark/run.js Outdated
Copy link
Member

@Trott Trott Jan 2, 2017

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

@vhf vhf Jan 2, 2017

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

@Trott Trott Jan 2, 2017

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

cjihrig
cjihrig approved these changes Jan 2, 2017
View reviewed changes
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
@vhf
events: avoid emit() eager deopt
91cf369 
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
benchmark: fix typo "categoty" -> "category"
8db2546
@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

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

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Jan 3, 2017
mhdawson
mhdawson approved these changes Jan 3, 2017
View reviewed changes
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
@vhf @jasnell
events: avoid emit() eager deopt
e52fee5 
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
@vhf @jasnell
benchmark: fix typo "categoty" -> "category"
da71d48 
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 •
edited by MylesBorins
Loading

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
@vhf
events: avoid emit() eager deopt
81866a9 
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
@vhf
benchmark: fix typo "categoty" -> "category"
d28b6d7 
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
@vhf
events: avoid emit() eager deopt
91e96ab 
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
@vhf
benchmark: fix typo "categoty" -> "category"
712d305 
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
@vhf
events: avoid emit() eager deopt
922318e 
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
@vhf
benchmark: fix typo "categoty" -> "category"
0b7ff76 
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 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@vhf
events: avoid emit() eager deopt
5cca693 
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
@vhf
benchmark: fix typo "categoty" -> "category"
4f96272 
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
Merged
@vsemozhetbyt vsemozhetbyt mentioned this pull request Feb 1, 2017
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 7, 2017

Thoughts re: backport?

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 13, 2017

ping

@benjamingr
Copy link
Member

benjamingr commented Apr 13, 2017

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 13, 2017

@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

Reviewers

@Trott Trott Trott left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

+3 more reviewers

@mscdex mscdex mscdex left review comments

@evanlucas evanlucas evanlucas approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.