Ditch Array.prototype methods in favor of fast array helpers

[briancavalier]

Faster, and makes the code nicer, imho. This is a precursor to handling Arguments objects in when/function et. al., as reported by @phated via IRC.

[briancavalier]

Closing this since it's not clear what the impact on real world performance will be. It might be worth more testing, but we should do something simpler to deal with the original issue of allowing Arguments objects that @phated raised.

[briancavalier]

@stefanpenner Could it be because before this change, we're using Array.prototype.slice? Unless v8 treats slice specially, it seems like that would incur the same deopt. So maybe when.js is already being penalized, which is why using these array helpers seems to either make no difference or actually improve perf?

[phated]

I know @jdalton has dealt with this. Pinging him

[stefanpenner]

So maybe when.js is already being penalized, which is why using these array helpers seems to either make no difference or actually improve perf?

yes when is already being penalized, only fn.apply doesn't suffer this problem on v8.

The only way to convert the arguments object to an array without angering the V8 gods is.

var length = arguments.length;
var args = new Array(length);

for (var i = 0; i < length; i++) {
  args[i] = arguments[i];
}

Also, as far as i can tell JSC handles this much better.

[briancavalier]

@stefanpenner ok, thanks, that's what I suspected ... that explains why these array helpers are actually faster than using slice in some cases. So, the only way not to be penalized is to use an inline loop to copy the arguments to an array. I don't think I'm willing to go that route yet ... maybe in 4.0 ... I'll keep my fingers crossed that v8 is able to optimize arguments before then ... or ES6 rest params arrive :D

[stefanpenner]

@briancavalier you and me both!

[jdalton]

Even passing arguments to apply may cause v8 to disqualify a method for optimizations or at least that's what this simple test is showing:

function printStatus(fn) {
  switch (%GetOptimizationStatus(fn)) {
    case 1: console.log('Function is optimized'); break;
    case 2: console.log('Function is not optimized'); break;
    case 3: console.log('Function is always optimized'); break;
    case 4: console.log('Function is never optimized'); break;
    case 6: console.log('Function is maybe deoptimized'); break;
  }
}

function foo(a,b,c) {
  return [a, b, c];
}

function bar() {
  return foo.apply(null, arguments);
}

bar(1, 2, 3);

%OptimizeFunctionOnNextCall(bar);

bar(1, 2, 3);

printStatus(bar);

> node --trace_opt --trace_deopt --allow-natives-syntax test.js
> [deoptimize context: ab31af14679]
> [disabled optimization for bar, reason: bad value context for arguments value]
> Function is not optimized

That said engines in general, not just v8, have narrow happy paths with arguments objects. I tend to let the public API handle things that disqualify a method from optimizations and pass it off to internal helpers that other internals use to avoid de-opting them. Using vanilla JS equivs of array methods (esp. ES5/ES6 methods) will produce better performance in general (esp. for small/medium arrays).

[briancavalier]

I tend to let the public API handle things that disqualify a method from optimizations and pass it off to internal helpers that other internals use to avoid de-opting them

Thanks @jdalton! This sounds like the best bet to me ... and much more palatable than littering inline array copy loops everywhere.

When I have time, I'm gonna take another pass on this branch and see how it goes. We're already being penalized and [].slice is slow ... sounds like I have a decent change of not making things any worse :)