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.

Sign up for GitHub

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

child_process: improve ipc performance #13459

Merged
merged 2 commits into from
Jun 9, 2017
Merged

child_process: improve ipc performance #13459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd83d11
child_process: simplify send() result handling
mscdex Jun 5, 2017
8208fda
child_process: reduce nextTick() usage
mscdex Jun 5, 2017
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
70 changes: 70 additions & 0 deletions benchmark/cluster/echo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict';

const cluster = require('cluster');
if (cluster.isMaster) {
const common = require('../common.js');
const bench = common.createBenchmark(main, {
workers: [1],
payload: ['string', 'object'],
sendsPerBroadcast: [1, 10],
n: [1e5]
});

function main(conf) {
var n = +conf.n;
var workers = +conf.workers;
var sends = +conf.sendsPerBroadcast;
var expectedPerBroadcast = sends * workers;
var payload;
var readies = 0;
var broadcasts = 0;
var msgCount = 0;

switch (conf.payload) {
case 'string':
payload = 'hello world!';
break;
case 'object':
payload = { action: 'pewpewpew', powerLevel: 9001 };
break;
default:
throw new Error('Unsupported payload type');
}

for (var i = 0; i < workers; ++i)
cluster.fork().on('online', onOnline).on('message', onMessage);

function onOnline(msg) {
if (++readies === workers) {
bench.start();
broadcast();
}
}

function broadcast() {
var id;
if (broadcasts++ === n) {
bench.end(n);
for (id in cluster.workers)
cluster.workers[id].disconnect();
return;
}
for (id in cluster.workers) {
const worker = cluster.workers[id];
Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong with this but you could ES6-ify it with for (const worker of Object.values(cluster.workers)). Whatever your preference.

I suppose it could even be a little faster because Object.values() doesn't walk the prototype chain but that's probably only a marginal benefit (and I didn't test so my intuition might very well be wrong.)

Copy link
Contributor Author

@mscdex mscdex Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW I literally copied it from the cluster docs. I know we were trying to be more on the conservative side when it comes to introducing ES* features to benchmarks because of possible incompatibilities with older branches. For example, Object.values() does not exist in node v4.x or v6.x (although it's apparently behind a flag in recent v6.x releases).

for (var i = 0; i < sends; ++i)
worker.send(payload);
}
}

function onMessage(msg) {
if (++msgCount === expectedPerBroadcast) {
msgCount = 0;
broadcast();
}
}
}
} else {
process.on('message', function(msg) {
process.send(msg);
});
}
68 changes: 39 additions & 29 deletions lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,16 +456,22 @@ function setupChannel(target, channel) {
}
chunks[0] = jsonBuffer + chunks[0];

var nextTick = false;
for (var i = 0; i < numCompleteChunks; i++) {
var message = JSON.parse(chunks[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check for isInternal on the raw chunk?

Copy link
Contributor Author

@mscdex mscdex Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It has to be parsed first. chunks here is just an array of (complete) JSON strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant implement a string based check. Can't we assume "innerMessages" will have a {cmd: NODE_ prefix? or at least include a , cmd: NODE_ substring? Might also be able to distinguish between three cases: user/inner/inner_with_handle, and turn the following compound if into a switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not make assumptions about JSON.stringify() output.


// There will be at most one NODE_HANDLE message in every chunk we
// read because SCM_RIGHTS messages don't get coalesced. Make sure
// that we deliver the handle with the right message however.
if (message && message.cmd === 'NODE_HANDLE')
handleMessage(target, message, recvHandle);
else
handleMessage(target, message, undefined);
if (isInternal(message)) {
if (message.cmd === 'NODE_HANDLE')
handleMessage(message, recvHandle, true, false);
else
handleMessage(message, undefined, true, false);
} else {
handleMessage(message, undefined, false, nextTick);
nextTick = true;
}
}
jsonBuffer = incompleteChunk;
this.buffering = jsonBuffer.length !== 0;
Expand Down Expand Up @@ -526,7 +532,7 @@ function setupChannel(target, channel) {

// Convert handle object
obj.got.call(this, message, handle, function(handle) {
handleMessage(target, message.msg, handle);
handleMessage(message.msg, handle, isInternal(message.msg), false);
});
});

Expand Down Expand Up @@ -645,16 +651,15 @@ function setupChannel(target, channel) {
obj.postSend(handle, options, target);
}

req.oncomplete = function() {
if (this.async === true)
if (req.async) {
req.oncomplete = function() {
control.unref();
if (typeof callback === 'function')
callback(null);
};
if (req.async === true) {
if (typeof callback === 'function')
callback(null);
};
control.ref();
} else {
process.nextTick(function() { req.oncomplete(); });
} else if (typeof callback === 'function') {
process.nextTick(callback, null);
}
} else {
// Cleanup handle on error
Expand Down Expand Up @@ -733,27 +738,32 @@ function setupChannel(target, channel) {
process.nextTick(finish);
};

function emit(event, message, handle) {
target.emit(event, message, handle);
}

function handleMessage(message, handle, internal, nextTick) {
if (!target.channel)
return;

var eventName = (internal ? 'internalMessage' : 'message');
if (nextTick)
process.nextTick(emit, eventName, message, handle);
else
target.emit(eventName, message, handle);
}

channel.readStart();
return control;
}


const INTERNAL_PREFIX = 'NODE_';
function handleMessage(target, message, handle) {
if (!target.channel)
return;

var eventName = 'message';
if (message !== null &&
typeof message === 'object' &&
typeof message.cmd === 'string' &&
message.cmd.length > INTERNAL_PREFIX.length &&
message.cmd.slice(0, INTERNAL_PREFIX.length) === INTERNAL_PREFIX) {
eventName = 'internalMessage';
}
process.nextTick(() => {
target.emit(eventName, message, handle);
});
function isInternal(message) {
return (message !== null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

even if we don't do a string based check, returning 0,1,2 (for user / inner / NODE_HANDLE) will enable using a switch in L466

Copy link
Contributor Author

@mscdex mscdex Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's unlikely it'll matter much, I'll let someone else work on it if they want. My main focus on this PR is nextTick() usage.

typeof message === 'object' &&
typeof message.cmd === 'string' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] since internal messages are rare, will adding a Object.hasOwnProperty(message, 'cmd') && fail faster than typeof message.cmd === 'string'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this could be simplified but I opted to leave it as-is for now.

message.cmd.length > INTERNAL_PREFIX.length &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Shave off a few more millis with const INTERNALPREFIX_LENGTH = INTERNAL_PREFIX.length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I know it can be simplified and probably optimized, but I opted to leave it as-is for now.

message.cmd.slice(0, INTERNAL_PREFIX.length) === INTERNAL_PREFIX);
}

function nop() { }
Expand Down