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.

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

net: use rest parameters instead of arguments #13472

Closed
wants to merge 1 commit into from
Closed

net: use rest parameters instead of arguments #13472

wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Jun 5, 2017

In v8 5.8, rest parameters are significantly faster than other ways to create an array of the arguments. (And it just looks better.)

Refs: #13430

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

net

@tniessen
net: use rest parameters instead of arguments
10ebc86 
In v8 5.8, rest parameters are significantly faster than other ways
to create an array of the arguments.

Refs: nodejs#13430
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 5, 2017
@tniessen
Copy link
Member Author

tniessen commented Jun 5, 2017

@joyeecheung Just noticed that your comments were probably not supposed to mean rest parameters but actually destructuring of the array args? If so, I am sorry, I will add the comments again.

@mscdex
Copy link
Contributor

mscdex commented Jun 5, 2017

From what I'm seeing, rest params are only faster once 5 arguments are passed to a function. This isn't the case for these functions.

@refack
Copy link
Contributor

refack commented Jun 5, 2017

@tniessen I'm +1 for readability, but we need to prove it does not come with a performance penalty. IMHO you should run a comparative benchmark (https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md)

Re: @mscdex's comment

@refack refack added performance Issues and PRs related to the performance of Node.js. refactor to ES6+ labels Jun 5, 2017
@tniessen
Copy link
Member Author

tniessen commented Jun 5, 2017

Mhhh I guess @mscdex is right, I benchmarked with 1000 and 5 arguments, and in both cases rest parameters were faster. I did not expect it to become slower once we hit less than 5 arguments... I have no idea how v8 implemented rest parameters, but I didn't think it could become slower than creating an array by hand and copying the values. Do rest parameters do more than that?

@simonkcleung
Copy link

Should arrow function even faster?

@mscdex
Copy link
Contributor

mscdex commented Jun 5, 2017

@simonkcleung What do you mean about arrow functions? They can't be used as the prototype functions because you lose the connection to the instance (this).

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Do rest parameters do more than that?

I think ...args has an O(1) cost, and for <5 args it's just that a loop is faster (loops have been around longer probably better optimized).

@refack
Copy link
Contributor

refack commented Jun 5, 2017

I'd be 💯 with this change if we mark it don't land on 6/7/8 or block on #13263

refack
refack approved these changes Jun 5, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM in principle.
Maybe wait with landing until V8 is even better

@simonkcleung
Copy link

@mscdex Right. I am just wondering if arguments is not created in arrow function, it should be faster. Anyway, I tested, if I did it correctly, the difference is very small.

@joyeecheung
Copy link
Member

@tniessen yes I did mean rest params...just picked the wrong word ;)

This is left as a TODO mostly because V8 does not optimize the common case (fewer arguments) enough yet.

@joyeecheung
Copy link
Member

Also for the methods changed here the valid signatures do not take more than 5 args anyway...(you can pass that many args but the ones before the final callback argument will be ignored anyway

@jasnell
Copy link
Member

jasnell commented Jun 13, 2017

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

@cjihrig
Copy link
Contributor

cjihrig commented Jun 14, 2017

@jasnell what is the plan for landing this? I thought the discussion was leaning toward "rest params aren't quite good enough yet."

@jasnell
Copy link
Member

jasnell commented Jun 14, 2017

Not sure to be honest. I'm happy with letting it sit

@tniessen
Copy link
Member Author

I cannot find any discussion about these performance issues apart from this, and that one ends with a commit saying that they "could optimize even further (leading to 8-10x improvements for functions with rest parameters". However, I am not too familiar with v8 internals, so I might just be missing something.

I guess we cannot expect performance improvements anytime soon. Feel free to notify me about any changes :)

@refack
Copy link
Contributor

refack commented Jun 16, 2017

Ref: #13729

@tniessen
Copy link
Member Author

tniessen commented Aug 8, 2017

@Trott Do you think your #13430 (comment) applies here?

@Trott
Copy link
Member

Trott commented Aug 8, 2017

@Trott Do you think your #13430 (comment) applies here?

I don't know. Would have to benchmark the change on master, I suppose. Pinging @davidmarkclements @mcollina in case there are other insights to be added...

@mcollina
Copy link
Member

mcollina commented Aug 8, 2017

@Trott yes, this change should be good to go.

Neither of those are in a typical hot path, so I'd say we should not worry anyway.

mcollina
mcollina approved these changes Aug 8, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott Trott removed the performance Issues and PRs related to the performance of Node.js. label Aug 8, 2017
@tniessen
Copy link
Member Author

tniessen commented Aug 9, 2017

@Trott Quick and dirty benchmark based on 10 million iterations with five arguments:

node v8 spread operator loop gain
8.2.1 5.8.283.41 376427426ms 497823279ms 24%
master 6.0.286.52 259321397ms 293091466ms 12%

The spread operator appears to be faster both in v8 5.8 and 6.0, even though I am a little surprised to see the huge performance gap between master and 8.2.1.

@tniessen tniessen self-assigned this Aug 9, 2017
@tniessen
Copy link
Member Author

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

@Trott
Copy link
Member

Trott commented Aug 10, 2017

Windows test failures seem unrelated but a re-run is probably in order just to be sure....

@tniessen
Copy link
Member Author

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

@refack
Copy link
Contributor

refack commented Aug 10, 2017

@Trott windows failure in job 9579 are caused by common.refreshTmpDir misfiring. I'm worried... Maybe we need to graceful-fs it.

@tniessen
Copy link
Member Author

CI is green, landed in 472a665.

@tniessen tniessen closed this Aug 10, 2017
tniessen added a commit that referenced this pull request Aug 10, 2017
@tniessen
net: use rest parameters instead of arguments
472a665 
In v8 6.0, rest parameters are significantly faster than other ways to
create an array of the arguments, even for small numbers.

PR-URL: #13472
Refs: #13430
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 10, 2017
@tniessen @addaleax
net: use rest parameters instead of arguments
c94f346 
In v8 6.0, rest parameters are significantly faster than other ways to
create an array of the arguments, even for small numbers.

PR-URL: #13472
Refs: #13430
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
@benjamingr benjamingr mentioned this pull request Sep 1, 2017
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

@mcollina mcollina mcollina approved these changes

Assignees

@tniessen tniessen

Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tniessen @mscdex @refack @simonkcleung @joyeecheung @jasnell @cjihrig @Trott @mcollina @nodejs-github-bot