net: avoid use of arguments

[notarseniy]

See #11359 and #11358.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

net

[mscdex]

@jasnell I'm confused as to why we're converting over to rest parameters when they're still slower in master? Here is what I get:

es/restparams-bench.js millions=100 method="copy": 20.298014523646717
es/restparams-bench.js millions=100 method="rest": 14.590843918868641
es/restparams-bench.js millions=100 method="arguments": 39.868138190132065

Rest parameters are still ~28% slower.

When you modify the benchmarks to explicitly specify some of the parameters, the gap closes somewhat, but it's still slower. I think we should revert the recently landed rest parameters usage and give the feature more time for optimization by the V8 team before we start using it in runtime code (tests and such are fine).

EDIT: Interestingly, removing the forced optimization (which I think is probably overused in many benchmarks) produces different results, which shows them being much closer in performance.

[Trott]

@mscdex IIUC, once #10992 lands, rest parameters should be as fast or faster than arguments. (Not arguing your point though: We should probably hold off on rest parameters in lib until that actually lands.)

[mscdex]

@Trott Where did you see that?

[ronkorving]

@mscdex My guess is https://v8project.blogspot.jp/2016/12/v8-release-56.html ?

[mscdex]

@ronkorving Hrmm... it doesn't show rest parameters and the benchmark site referenced in the article tests rest parameters in a much simpler way (it just accesses one argument and does not make an array of the arguments in the ES5 implementation). Also they're comparing 5.4 and 5.6. I wonder if 5.5 vs 5.6 results in the same increase in performance. This is relevant since IIRC 5.5 will land in node v7.x.

[jasnell]

@mscdex ... I've been benchmarking as I go and what I've found is that the results are fairly inconsistent. We should still avoid rest params in code we know is hot or in cases where there are either no arguments or a fixed number of arguments passed typically. In the case where no arguments are typically passed, the rest params can cause up to a 50% slowdown. That said, in some of the cases where deoptimization is caused either by leaking arguments (by passing it off to an apply() or doing an out of bounds read on arguments, using the rest params has shown to provide modest improvement -- at least in the testing I have done. With the optimizations that are continuing to be made, I believe these will continue to improve. So rather than reverting anything, Perhaps we should just mark these as dont-land in 7.x and earlier?

[Trott]

@mscdex asked:

@Trott Where did you see that?

@ronkorving replied:

@mscdex My guess is https://v8project.blogspot.jp/2016/12/v8-release-56.html ?

I thought I saw something more recent and more specific about how rest parameters in V8 5.6 would generally be as performant or more performant than arguments. I'm having trouble finding it right now. Maybe I'm misremembering. Paging @bmeurer and @fhinkel ....

[vsemozhetbyt]

@Trott Something a bit different, but concerned: https://twitter.com/bmeurer/status/824182909317902336

[bmeurer]

Until we finally turn on --future by default, rest parameters vs. arguments is a tricky thing. One of the main problems is that for each function we have decide at parsing time whether to send it to FCG+CS or I+TF. A function with rest parameters has to go to I+TF. But assume that that function now calls other functions that didn't have anything fancy inside that would enforce I+TF. Now you can no longer inline those at the call sites, and performance will already be worse.

Also note that in 5.6 TF lacked quite a few optimizations still for other JavaScript constructs. So while rest parameters are properly optimized, other constructs in the same function might not yet benefit from TF.

So my advice would be to wait until we flip --future switch, at least until 5.8, and only then start removing arguments and other weird constructs. We can offer help in the migration. I noticed that various parts of Node.js are highly tuned for CS, probably because of benchmarking work that was done in the past. A lot of the rules still apply, but some rules also changed significantly with TF.

[mscdex]

@bmeurer Ok, so then you are also suggesting we revert recently added rest parameter additions?

[bmeurer]

I'd suggest to keep those that fix a concrete problem, like Crankshaft disabling optimization because of funny use of arguments, but wait a bit for the remaining cosmetic fixes.

[mscdex]

I think all of the recent rest params changes have been cosmetic (just replacing a loop). I would rather keep it consistent and avoid potential performance issues by rolling back the rest params changes for now...

Thoughts @nodejs/collaborators ?

[lpinca]

I would rather keep it consistent and avoid potential performance issues by rolling back the rest params changes for now...

👍 from me.

[ChALkeR]

@mscdex

I think all of the recent rest params changes have been cosmetic

I don't think that is correct, #11357 and #11242 showed significant performance improvements (though the latter did that in a new benchmark that hasn't landed yet: #11313).

[mscdex]

@ChALkeR The performance is not consistent and the rest params can actually cause regressions as both @jasnell and @bmeurer have pointed out.

When I call it cosmetic changes, what I meant was that the use of rest parameters did not happen because we were trying to avoid an existing deopt because of the use of arguments as @bmeurer described.

[jasnell]

My preference in general would be to simply mark those as dont-land in 7 or below and leave them unless they should a definite perf loss.

[vsemozhetbyt]

@ChALkeR I should notice that #11242 does not use rest parameters, it simply avoids out-of-bounds with arguments.length check and adding a named parameter. As for other PRs (#11359, #11404, and #11357) the perf gain from avoiding *.apply(*, arguments) could be partly caused by fixing #11001 (but I am not sure). So maybe, if these PRs would be reverted/closed, these fragments need to be addressed in other ways.

[notarseniy]

+1 for dont-land in 7 and lower. This change is definitely valuable only in new V8's, plus also see @mscdex comment:

EDIT: Interestingly, removing the forced optimization (which I think is probably overused in many benchmarks) produces different results, which shows them being much closer in performance.

Also, thanks to everyone in this thread for pointing out this issue. Really interesting conversation for me.

[BridgeAR]

Rest params clearly got a lot better and this should probably followed up now.

es/restparams-bench.js millions=500 method="copy": 32.08098823613811
es/restparams-bench.js millions=500 method="rest": 52.6404804858652
es/restparams-bench.js millions=500 method="arguments": 52.54962030819939

@notarseniy would you be so kind and rebase this?

[benjamingr]

Looks like these changes have already happened in 472a665 #13472

Thank you for your contribution @notarseniy !