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.

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

Code question #13430

Closed
fsx950223 opened this issue Jun 3, 2017 · 10 comments
Closed

Code question #13430

fsx950223 opened this issue Jun 3, 2017 · 10 comments
Labels
performance Issues and PRs related to the performance of Node.js. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.

Comments

@fsx950223
Copy link

fsx950223 commented Jun 3, 2017

  • Version: 7.10
  • Platform: windows
  • Subsystem: net.js Server.prototype.listen

Why use this

var args = new Array(arguments.length);
for (var i = 0; i < arguments.length; i++)
  args[i] = arguments[i];

instead of

var args = Array.from(arguments)

Edit by @tniessen: Formatting

@not-an-aardvark
Copy link
Contributor

I think this has to do with allowing the function to be optimized. V8 has some odd optimization behavior regarding arguments -- see Optimization Killers.

@ChALkeR ChALkeR added the question Issues that look for answers. label Jun 3, 2017
@vsemozhetbyt vsemozhetbyt added performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Jun 3, 2017
@gamelaster
Copy link

So spread operator for arguments array is too slower?

@tniessen
Copy link
Member

tniessen commented Jun 3, 2017

Yes, both the spread operator and Array.from(arguments) are currently slower than a good old loop.

Edit: Remove inaccurate benchmark

@ChALkeR
Copy link
Member

ChALkeR commented Jun 3, 2017

@tniessen Are rest arguments still slow on 5.8? I.e. function(...args) {.

@tniessen
Copy link
Member

tniessen commented Jun 3, 2017

@ChALkeR Sorry, the last benchmark was inaccurate thanks to some v8 optimizations.

Edit: Correct benchmark.

Non-strict mode, normal

Rest parameters:   1339159
Loop:              6605579
slice:             7823976
push(...):        95413135
[...]:           245505113
Array.from:      372074063

Non-strict mode, apply

Rest parameters:  25619389
Loop:             54306959
slice:            58377201
push(...):       120685909
[...]:           299723564
Array.from:      422041350

Strict mode, normal

Rest parameters:   1246582
Loop:              6643733
slice:             7899853
push(...):        95045032
[...]:           248078968
Array.from:      369839268

Strict mode, apply

Rest parameters:  25643509
Loop:             52928680
slice:            57370494
push(...):       120584620
[...]:           297672491
Array.from:      423719739

@simonkcleung
Copy link

I tested that function test(...args) {} is fastest.
Code:

function forLoop() {
  var args = new Array(arguments.length);
  for(var i = 0, len = arguments.length; i < len; i++)
    args[i] = arguments[i];
}

function ArrayFrom() {
  var args = Array.from(arguments);
}

function ArraySlice() {
  var args = Array.prototype.slice.call(arguments);
}

function spreadArray() {
  var args = [...arguments];
}

function spreadPush() {
  var args = [];
  args.push(...arguments);
}

function spreadArg(...args) {
}

var tests = [ forLoop, ArrayFrom, ArraySlice, spreadArray, spreadPush, spreadArg ];

for (var i = 0; i < tests.length; i++){
  var test=tests[i], j=0;
  console.log(test.name);
  console.time(test.name + 0);
     for (j = 0; j < 100000; j++){
     test();
  }
  console.timeEnd(test.name + 0);
  console.time(test.name+1);
     for (j = 0; j < 100000; j++){
     test(1);
  }
  console.timeEnd(test.name + 1);
  console.time(test.name+2);
     for (j = 0; j < 100000; j++){
     test(1, 2);
  }
  console.timeEnd(test.name + 2);
  console.time(test.name + 3);
     for (j = 0; j < 100000; j++){
     test(1, 2, 3);
  }
  console.timeEnd(test.name + 3);
  console.time(test.name + 4);
     for (j = 0; j < 100000; j++){
     test(1, 2, 3, 4);
  }
  console.timeEnd(test.name + 4);
  console.time(test.name + 5);
     for (j = 0; j < 100000; j++){
     test(1, 2, 3, 4, 5);
  }
  console.timeEnd(test.name + 5);
  console.time(test.name + 6);
     for (j = 0; j < 100000; j++){
     test(1, 2, 3, 4, 5, 6);
  }
  console.timeEnd(test.name + 6);
  console.time(test.name + 7);
     for (j = 0; j < 100000; j++){
     test(1, 2, 3, 4, 5, 6, 7);
  }
  console.timeEnd(test.name + 7);
}

tniessen added a commit to tniessen/node that referenced this issue Jun 5, 2017
In v8 5.8, rest parameters are significantly faster than other ways
to create an array of the arguments.

Refs: nodejs#13430
@mscdex
Copy link
Contributor

mscdex commented Jun 5, 2017

@simonkcleung You have to be careful when benchmarking. For example, in that benchmarking code you gave, the spreadArg() function will optimized away because it contains no code, so you end up benchmarking how fast V8 can execute an empty for-loop.

If you add something like return args[0] + 1 to each function, that would be more representative of performance for each solution. It's also good to execute each different benchmark in isolation because I've seen the execution of one benchmarked function affect the results of further benchmarked functions (e.g. executing ArrayFrom() benchmarks after forLoop()). When I make the necessary changes, I see that forLoop() is actually the fastest for <= 4 arguments. Anything more than that, rest params do seem to be somewhat faster.

FWIW here are example results of what I'm seeing with spreadArg() and forLoop() on master (with iterations bumped up to 1e8):

spreadArg
spreadArg0: 1625.339ms
spreadArg1: 1945.502ms
spreadArg2: 1975.642ms
spreadArg3: 2065.839ms
spreadArg4: 2151.618ms
spreadArg5: 2208.423ms
spreadArg6: 2267.304ms
spreadArg7: 2377.411ms
forLoop
forLoop0: 1609.836ms
forLoop1: 1467.904ms
forLoop2: 1682.607ms
forLoop3: 1893.179ms
forLoop4: 2076.911ms
forLoop5: 2366.336ms
forLoop6: 2593.911ms
forLoop7: 2699.884ms

@Trott
Copy link
Member

Trott commented Aug 7, 2017

Spread operator should get a significant performance bump when Node.js 8.3.0 (with V8 6.0) comes out (probably this week).

Closing as this is not really a Node.js issue. (The spread operator is implemented by V8.) Feel free to comment or (if GitHub lets you) re-open if you think that's misguided of me.

@Trott Trott closed this as completed Aug 7, 2017
@Trott
Copy link
Member

Trott commented Aug 7, 2017

Oh, reference for the perf boost for spread operator: https://github.com/davidmarkclements/v8-perf

tniessen added a commit that referenced this issue Aug 10, 2017
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>
addaleax pushed a commit that referenced this issue Aug 10, 2017
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>
@fsx950223
Copy link
Author

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

9 participants