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

net: misc changes #8112

Merged
merged 2 commits into from
Aug 23, 2016
Merged

net: misc changes #8112

merged 2 commits into from
Aug 23, 2016

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 15, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • net
Description of change
  • Removes unnecessary variables
  • Fixes potential deopts when accessing nonexistent indices of an array

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Aug 15, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Aug 15, 2016

@@ -60,9 +60,8 @@ exports.createServer = function(options, connectionListener) {
// connect(path, [cb]);
//
exports.connect = exports.createConnection = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps switch to function(...args) { here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't ran the relevant benchmarks recently (at least with v8 5.1), but I wouldn't be surprised if rest args are still slower.

Copy link
Member

Choose a reason for hiding this comment

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

They are still a bit slower.

@benjamingr
Copy link
Member

LGTM

2 similar comments
@evanlucas
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 16, 2016

LGTM

@mscdex
Copy link
Contributor Author

mscdex commented Aug 23, 2016

CI again before landing: https://ci.nodejs.org/job/node-test-pull-request/3800/

V8 is smart enough to optimize the length property checking when
iterating over an array with a for loop.

PR-URL: nodejs#8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This helps to prevent possible deoptimizations that arise when trying
to access nonexistent indices.

PR-URL: nodejs#8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex mscdex merged commit a206afe into nodejs:master Aug 23, 2016
@mscdex mscdex deleted the net-connect-args-add-check branch August 23, 2016 19:25
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
V8 is smart enough to optimize the length property checking when
iterating over an array with a for loop.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
This helps to prevent possible deoptimizations that arise when trying
to access nonexistent indices.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

should this be backported?

@mscdex
Copy link
Contributor Author

mscdex commented Sep 30, 2016

I would say it's safe to backport.

MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
V8 is smart enough to optimize the length property checking when
iterating over an array with a for loop.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
This helps to prevent possible deoptimizations that arise when trying
to access nonexistent indices.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
V8 is smart enough to optimize the length property checking when
iterating over an array with a for loop.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
This helps to prevent possible deoptimizations that arise when trying
to access nonexistent indices.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
V8 is smart enough to optimize the length property checking when
iterating over an array with a for loop.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
This helps to prevent possible deoptimizations that arise when trying
to access nonexistent indices.

PR-URL: #8112
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants