net: use rest parameters instead of arguments#13472
Conversation
In v8 5.8, rest parameters are significantly faster than other ways to create an array of the arguments. Refs: nodejs#13430
|
@joyeecheung Just noticed that your comments were probably not supposed to mean rest parameters but actually destructuring of the array |
|
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. |
|
@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 |
|
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? |
|
Should arrow function even faster? |
|
@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 ( |
I think |
|
I'd be 💯 with this change if we mark it |
refack
left a comment
There was a problem hiding this comment.
LGTM in principle.
Maybe wait with landing until V8 is even better
|
@mscdex Right. I am just wondering if |
|
@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. |
|
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 what is the plan for landing this? I thought the discussion was leaning toward "rest params aren't quite good enough yet." |
|
Not sure to be honest. I'm happy with letting it sit |
|
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 :) |
|
Ref: #13729 |
|
@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... |
|
@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. |
|
@Trott Quick and dirty benchmark based on 10 million iterations with five arguments:
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. |
|
Windows test failures seem unrelated but a re-run is probably in order just to be sure.... |
|
@Trott windows failure in job 9579 are caused by |
|
CI is green, landed in 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 <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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), orvcbuild test(Windows) passesAffected core subsystem(s)
net