These sort of changes need benchmarks - have you tried running the benchmarks and getting results?

@apapirovski : Thanks for the feedback. I'll close the PR.

@apapirovski would still love some benchmarks

@benjamingr I'm speaking from experience after trying to make this change on events and having the benchmarks be roughly 20-25% slower in certain situations. Theoretically this is supposed to be optimized (a basic benchmark shows maybe 2-3% perf gap and that might just be noise), practically there seem to be edge cases.

@apapirovski : Thanks for sharing that. Quite helpful 👍

@apapirovski can you send me a link to those benchmarks then? This is the third time we've been talking about whether or not spread is optimized or not and I want something concrete to go with when I bug V8 people about it :)

@benjamingr the benchmarks are just the events ones... I think I saw the issue when working on once or something similar. It's also possible it might've been in process.nextTick or timers. I can try to reproduce again when I have a moment.

I wish I had more info, it's just that I've been working on that stuff on-and-off for the past 2 months so it's a bit hard to recall at what point I encountered it. You could look over recent events, timers & process PRs potentially.

@apapirovski @benjamingr : Just to mention, post closing the PR.. over my conversation with few JS experienced dev folks.. they did acknowledge too that f.apply(undefined, arguments); is much better performant than f(...arguments); in multiple different scenarios and they did confirm they've tested it !

ping @bmeurer any thoughts on fn(...arguments) vs fn.apply(undefined, arguments)? I'm almost certain I've run into edge cases before but I also know the V8 team has done work to optimize this recently?

FWIW I think the fastest combination here might be

function(...args) {
  return cb.apply(undefined, args);
}

When I test that version within once with ee-once benchmark, it's about 5% faster than using arguments. Don't ask me why...

@mithunsasidharan If you would like to try that version of the PR and run the relevant benchmarks in benchmark/fs then that PR could probably be accepted?

Here's the bench results after a short run:

fs/bench-readdir.js n=10000      1.95 %        *** 0.0006178907

Reasonably certain it's faster. Don't think it can land on v8.x or older though.

Also, you could potentially test fn(...args) { return cb(...args); }? It might be the same. Not sure.

@apapirovski : Can you take a look at updated change here ?

@apapirovski Yes I also think fn.apply(undefined, args) is best for now.

CI: https://ci.nodejs.org/job/node-test-pull-request/11915/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/71/

@apapirovski : Thanks much !

LGTM

Landed in 0a0fbd5