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

process: improve nextTick() performance #13446

Merged
merged 1 commit into from
Jun 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/process/next-tick-depth-args.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

var common = require('../common.js');
var bench = common.createBenchmark(main, {
millions: [2]
millions: [12]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this increase? You are getting ridiculous statistical confidence. By some quick calculation, this should only improve the confidence by a factor of 2.5. Compensating for that and the previous version should also give you confidence well beyond ***.

Copy link
Contributor Author

@mscdex mscdex Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always choose something large enough to allow for any re-opts to occur and for results to stabilize. With 2, I was getting results that varied quite a bit in comparison because it was finishing so quickly (at least on my machine).

});

process.maxTickDepth = Infinity;
Expand Down
2 changes: 1 addition & 1 deletion benchmark/process/next-tick-depth.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';
var common = require('../common.js');
var bench = common.createBenchmark(main, {
millions: [2]
millions: [12]
});

process.maxTickDepth = Infinity;
Expand Down
138 changes: 95 additions & 43 deletions lib/internal/process/next_tick.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,42 @@ exports.setup = setupNextTick;
// Will be overwritten when setupNextTick() is called.
exports.nextTick = null;

class NextTickQueue {
constructor() {
this.head = null;
this.tail = null;
this.length = 0;
}

push(v) {
const entry = { data: v, next: null };
if (this.length > 0)
this.tail.next = entry;
else
this.head = entry;
this.tail = entry;
++this.length;
}

shift() {
if (this.length === 0)
return;
const ret = this.head.data;
if (this.length === 1)
this.head = this.tail = null;
else
this.head = this.head.next;
--this.length;
return ret;
}

clear() {
this.head = null;
this.tail = null;
this.length = 0;
}
}

function setupNextTick() {
const async_wrap = process.binding('async_wrap');
const async_hooks = require('async_hooks');
Expand All @@ -27,7 +63,7 @@ function setupNextTick() {
const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr, kInitTriggerId } =
async_wrap.constants;
const { async_id_symbol, trigger_id_symbol } = async_wrap;
var nextTickQueue = [];
var nextTickQueue = new NextTickQueue();
var microtasksScheduled = false;

// Used to run V8's micro task queue.
Expand Down Expand Up @@ -55,27 +91,29 @@ function setupNextTick() {
function tickDone() {
if (tickInfo[kLength] !== 0) {
if (tickInfo[kLength] <= tickInfo[kIndex]) {
nextTickQueue = [];
nextTickQueue.clear();
tickInfo[kLength] = 0;
} else {
nextTickQueue.splice(0, tickInfo[kIndex]);
tickInfo[kLength] = nextTickQueue.length;
}
}
tickInfo[kIndex] = 0;
}

const microTasksTickObject = {
callback: runMicrotasksCallback,
args: undefined,
domain: null,
[async_id_symbol]: 0,
[trigger_id_symbol]: 0
};
function scheduleMicrotasks() {
if (microtasksScheduled)
return;

const tickObject =
new TickObject(runMicrotasksCallback, undefined, null);
// For the moment all microtasks come from the void until the PromiseHook
// API is implemented.
tickObject[async_id_symbol] = 0;
tickObject[trigger_id_symbol] = 0;
nextTickQueue.push(tickObject);
nextTickQueue.push(microTasksTickObject);

tickInfo[kLength]++;
microtasksScheduled = true;
Expand All @@ -86,8 +124,9 @@ function setupNextTick() {
_runMicrotasks();

if (tickInfo[kIndex] < tickInfo[kLength] ||
emitPendingUnhandledRejections())
emitPendingUnhandledRejections()) {
Copy link
Contributor

@refack refack Jun 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering... It this a style choice or a DEOPT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style choice, I think we tend to use braces for multi-line conditionals from what I've generally seen, this is just one "offender" I ran across while working in this file.

scheduleMicrotasks();
}
}

function _combinedTickCallback(args, callback) {
Expand Down Expand Up @@ -133,7 +172,8 @@ function setupNextTick() {
function _tickCallback() {
do {
while (tickInfo[kIndex] < tickInfo[kLength]) {
const tock = nextTickQueue[tickInfo[kIndex]++];
++tickInfo[kIndex];
const tock = nextTickQueue.shift();
const callback = tock.callback;
const args = tock.args;

Expand Down Expand Up @@ -174,7 +214,8 @@ function setupNextTick() {
function _tickDomainCallback() {
do {
while (tickInfo[kIndex] < tickInfo[kLength]) {
const tock = nextTickQueue[tickInfo[kIndex]++];
++tickInfo[kIndex];
const tock = nextTickQueue.shift();
const callback = tock.callback;
const domain = tock.domain;
const args = tock.args;
Expand Down Expand Up @@ -210,45 +251,48 @@ function setupNextTick() {
} while (tickInfo[kLength] !== 0);
}

function TickObject(callback, args, domain) {
this.callback = callback;
this.domain = domain;
this.args = args;
this[async_id_symbol] = -1;
this[trigger_id_symbol] = -1;
}

function setupInit(tickObject, triggerAsyncId) {
tickObject[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr];
tickObject[trigger_id_symbol] = triggerAsyncId || initTriggerId();
if (async_hook_fields[kInit] > 0) {
emitInit(tickObject[async_id_symbol],
'TickObject',
tickObject[trigger_id_symbol],
tickObject);
class TickObject {
constructor(callback, args, asyncId, triggerAsyncId) {
this.callback = callback;
this.args = args;
this.domain = process.domain || null;
this[async_id_symbol] = asyncId;
this[trigger_id_symbol] = triggerAsyncId;
}
}

// `nextTick()` will not enqueue any callback when the process is about to
// exit since the callback would not have a chance to be executed.
function nextTick(callback) {
if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
// on the way out, don't bother. it won't get fired anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clarify the comment rather than removing it?

Copy link
Contributor Author

@mscdex mscdex Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it matches internalNextTick() and it doesn't seem like a particularly helpful comment as process._exiting is pretty self-explanatory to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code says what but I'm wondering if it may be helpful to say why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW Crankshaft can decide not to inline a function because it has a comment in it 😞 so if we come up with a better comment it probably should go in line 231.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, it makes little difference as the comment was already there and that will soon be an obsolete optimization as I understand.

Copy link
Member

@gibfahn gibfahn Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean by a 'why comment.'

AIUI it's a comment explaining why we're doing this (as opposed to explaining what this code does).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn I understand that part. What I meant was I don't understand what this is in reference to. Is it the process._exiting line? The changes I'm making? Something else? What is the suggested 'why comment' ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the suggested 'why comment' ?

(AIUI) This is the comment, it's a "why comment", it's helpful to preserve it.

    // on the way out, don't bother. it won't get fired anyway.

FWIW I'm sure putting it above the function is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 @gibfahn I've added something above both functions now, let me know if that is what you had in mind or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 @gibfahn I've added something above both functions now, let me know if that is what you had in mind or not.

Clearer than the original, thanks.

if (process._exiting)
return;

var args;
if (arguments.length > 1) {
args = new Array(arguments.length - 1);
for (var i = 1; i < arguments.length; i++)
args[i - 1] = arguments[i];
switch (arguments.length) {
case 1: break;
case 2: args = [arguments[1]]; break;
case 3: args = [arguments[1], arguments[2]]; break;
case 4: args = [arguments[1], arguments[2], arguments[3]]; break;
default:
args = new Array(arguments.length - 1);
for (var i = 1; i < arguments.length; i++)
args[i - 1] = arguments[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is faster than spread parameters even with V8 5.9?

Copy link
Contributor Author

@mscdex mscdex Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how spread could be used here. Do you mean rest parameters? If so, I tried that and while it's a little faster in one benchmark, it's slower to varying degrees in all others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I meant. Sorry, could never get all the terminologies right.

}

var obj = new TickObject(callback, args, process.domain || null);
setupInit(obj, null);
const asyncId = ++async_uid_fields[kAsyncUidCntr];
const triggerAsyncId = initTriggerId();
const obj = new TickObject(callback, args, asyncId, triggerAsyncId);
nextTickQueue.push(obj);
tickInfo[kLength]++;
++tickInfo[kLength];
if (async_hook_fields[kInit] > 0)
emitInit(asyncId, 'TickObject', triggerAsyncId, obj);
}

// `internalNextTick()` will not enqueue any callback when the process is
// about to exit since the callback would not have a chance to be executed.
function internalNextTick(triggerAsyncId, callback) {
if (typeof callback !== 'function')
throw new TypeError('callback is not a function');
Expand All @@ -259,17 +303,25 @@ function setupNextTick() {
return;

var args;
if (arguments.length > 2) {
args = new Array(arguments.length - 2);
for (var i = 2; i < arguments.length; i++)
args[i - 2] = arguments[i];
switch (arguments.length) {
case 2: break;
case 3: args = [arguments[2]]; break;
case 4: args = [arguments[2], arguments[3]]; break;
case 5: args = [arguments[2], arguments[3], arguments[4]]; break;
default:
args = new Array(arguments.length - 2);
for (var i = 2; i < arguments.length; i++)
args[i - 2] = arguments[i];
}

var obj = new TickObject(callback, args, process.domain || null);
setupInit(obj, triggerAsyncId);
const asyncId = ++async_uid_fields[kAsyncUidCntr];
const obj = new TickObject(callback, args, asyncId, triggerAsyncId);
nextTickQueue.push(obj);
++tickInfo[kLength];
if (async_hook_fields[kInit] > 0)
emitInit(asyncId, 'TickObject', triggerAsyncId, obj);

// The call to initTriggerId() was skipped, so clear kInitTriggerId.
async_uid_fields[kInitTriggerId] = 0;
nextTickQueue.push(obj);
tickInfo[kLength]++;
}
}