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

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

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

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

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

@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

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

@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?

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

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

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

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

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

CI is green, landed in 472a665.