events: avoid emit() eager deopt by vhf · Pull Request #10568 · nodejs/node · GitHub

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

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

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

since er is undefined by default.

Right, thanks!

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

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

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

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

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.

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

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

LGTM

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>

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>

Landed in e52fee5...da71d48

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>

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>

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>

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>

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>

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>

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>

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>

Thoughts re: backport?

ping

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

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

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. lts-watch-v6.x labels Jan 1, 2017

mscdex reviewed Jan 1, 2017

View reviewed changes

vhf force-pushed the fix-eventemitter-emit-deopt branch from 843b3d3 to 2c64dd8 Compare January 1, 2017 22:49

nodejs-github-bot added the lts-watch-v6.x label Jan 1, 2017

vhf force-pushed the fix-eventemitter-emit-deopt branch from 2c64dd8 to 3d39b08 Compare January 1, 2017 23:17

Trott reviewed Jan 2, 2017

View reviewed changes

benjamingr approved these changes Jan 2, 2017

View reviewed changes

cjihrig approved these changes Jan 2, 2017

View reviewed changes

vhf added 2 commits January 2, 2017 22:28

benchmark: fix typo "categoty" -> "category"

8db2546

vhf force-pushed the fix-eventemitter-emit-deopt branch from 3d39b08 to 8db2546 Compare January 2, 2017 21:33

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

evanlucas approved these changes Jan 3, 2017

View reviewed changes

Fishrock123 approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 3, 2017

View reviewed changes

mhdawson approved these changes Jan 3, 2017

View reviewed changes

jasnell closed this Jan 5, 2017

vhf deleted the fix-eventemitter-emit-deopt branch January 8, 2017 23:29

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

vsemozhetbyt mentioned this pull request Feb 1, 2017

Possibly deoptimizing use of arguments in libs #10323

Closed

MylesBorins added the lts-watch-v4.x label Mar 7, 2017

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 removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

events: avoid emit() eager deopt by vhf · Pull Request #10568 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: avoid emit() eager deopt - #10568

events: avoid emit() eager deopt#10568vhf wants to merge 2 commits intonodejs:masternodejs/node:masterfrom vhf:fix-eventemitter-emit-deoptvhf/node:fix-eventemitter-emit-deoptCopy head branch name to clipboard

Conversation

vhf commented Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex Jan 1, 2017

Choose a reason for hiding this comment

Uh oh!

vhf Jan 1, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

vhf Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

vhf commented Jan 2, 2017

Uh oh!

cjihrig commented Jan 2, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 5, 2017 • edited by MylesBorins Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Mar 7, 2017

Uh oh!

MylesBorins commented Apr 13, 2017

Uh oh!

benjamingr commented Apr 13, 2017

Uh oh!

MylesBorins commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

events: avoid emit() eager deopt#10568
vhf wants to merge 2 commits into
nodejs:masterfrom
vhf:fix-eventemitter-emit-deopt

vhf commented Jan 1, 2017 •
edited
Loading

jasnell commented Jan 5, 2017 •
edited by MylesBorins
Loading