process: use rest parameters in nextTick.

[hashseed]

This increases the performance of stream/writeable-manywrites.js
both with Crankshaft and with Turbofan significantly.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

process

[hashseed]

Individual scores of stream/writeable-manywrites.js on my workstation are interesing:
vee-eight-lkgr with Turbofan: 1.7M
vee-eight-lkgr with Crankshaft: 2.3M
vee-eight-lkgr with this patch, TF or CS: 2.3M
master: 1.7M
master with this patch: 2.0M

[hashseed]

@bmeurer @trevnorris @MylesBorins

[lpinca]

There is a minor difference now when arguments.length === 1 as args is an empty array instead of undefined. I don't know if it's relevant. If not LGTM.

[hashseed]

Good point. Fixed the use site. It doesn't seem to affect performance though.

[mscdex]

I'm assuming this is V8 5.7 only.

[mscdex]

Also, is there still a performance regression when the rest parameters only constitutes one argument (e.g. process.nextTick(fn, 'foo')? I seem to recall someone discovering that or a similar situation recently...

[hashseed]

At least with a micro-benchmark rest parameters do not seem to be slower than arguments object:

function arguments_object(x) {
  if (arguments.length > 0) {
    var a = new Array(arguments.length - 1);
    for (var i = 1; i < arguments.length; i++) {
      a[i - 1] = arguments[i];
    }
    args.push(a);
  }
}

function rest_params(x, ...rest) {
  args.push(rest);
}

var args = [];
var start = Date.now();
for (var i = 0; i < 1000000; i++) {
  arguments_object(1);
}
console.log("arguments-1  ", Date.now() - start);

args = [];
start = Date.now();
for (var i = 0; i < 1000000; i++) {
  rest_params(1);
}
console.log("rest-params-1", Date.now() - start);

args = [];
start = Date.now();
for (var i = 0; i < 1000000; i++) {
  arguments_object(1, 2, 3);
}
console.log("arguments-3  ", Date.now() - start);

args = [];
start = Date.now();
for (var i = 0; i < 1000000; i++) {
  rest_params(1, 2, 3);
}
console.log("rest params-3", Date.now() - start);

I get with the current master:

arguments-1   446
rest-params-1 247
arguments-3   479
rest params-3 305

[mscdex]

FWIW I get much closer results when bypassing the loop in arguments_object() and pushing an empty literal array when arguments.length === 1:

arguments-1 318
rest-params-1 300
arguments-3 464
rest params-3 422

[mscdex]

I think the empty array creation was avoided because it was unnecessary. Although, I'm not sure how that optimization vs. having a consistent value type for args compares.

[jasnell]

From what I've seen in my own benchmarking, V8 5.8 and higher does not seem to have the same performance regression for rest params < 2 arguments. There's still a bit of a hit but it's not as bad. Hopefully it will continue to get better.

[jasnell]

In general I'm happy with the change and it's definitely the right direction, we just need to be careful not to introduce regressions in the most common cases in hot code paths. We also need to be cognizant that these kinds of changes will make backporting to LTS far more difficult.

That all said, this change LGTM

[Fishrock123]

I'm hesitant to make this change unless we are very sure. Experimenting with async_hooks has only brought it even more into the light how much nextTick() is used internally. If an event is to be emitted, expect a nextTick() to be scheduled. (I do not know if there is an overall more hot code path in Node.)

[hashseed]

I created this change because this fixes a performance regression with stream/writeable-manywrites.js with V8 5.9, with the new compilation pipeline.

I'd be happy to wait with this PR until then, or run additional benchmarks, should that be required.

[jasnell]

Let's hold off on landing until after 8.0.0 is released and we update master to 5.8. There will be a large number of these kinds of changes that need to happen to avoid regressions under 5.9, we'll want to be coordinated and deliberate about making them since there will be a definite impact on our ability to easily backport changes to LTS.

[Trott]

@hashseed Would you be willing to re-open this? It's still something we want, right?

[apapirovski]

FWIW I started working on a version of this yesterday, with a few other changes. Still testing performance to find the optimal one.

@hashseed if you prefer to resume this work let me know and I'll stop working on mine. :)

[hashseed]

@apapirovski I do not mind if someone takes this over :)