child_process: improve ipc performance #13459

mscdex · 2017-06-05T00:23:20Z

These commits reduce/improve nextTick() usage in the child_process IPC implementation.

I should point out that I wasn't sure if we should ever need nextTick() when receiving/emitting messages, so for now I just made sure that both all internal messages and only the first non-internal message in a group are emitted within the same tick.

There is a substantial performance improvement when removing nextTick() altogether for emitted messages. My thought was that these messages should always happen on at least the next tick anyway, so perhaps it would be safe to remove it completely? All tests pass with nextTick() completely removed from handleMessage().

Results with the current changes with the included benchmark:

                                                                           improvement confidence      p.value
 cluster/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1      15.62 %        *** 1.440552e-08
 cluster/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1      18.59 %        *** 3.478095e-10
 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1     25.71 %        *** 6.436977e-26
 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1     33.88 %        *** 1.812799e-22

CI: https://ci.nodejs.org/job/node-test-pull-request/8473/

Checklist

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

Affected core subsystem(s)

child_process

refack · 2017-06-05T02:55:56Z

lib/internal/child_process.js

+  return (message !== null &&
+          typeof message === 'object' &&
+          typeof message.cmd === 'string' &&
+          message.cmd.length > INTERNAL_PREFIX.length &&


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

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

refack · 2017-06-05T03:03:06Z

lib/internal/child_process.js

+function isInternal(message) {
+  return (message !== null &&
+          typeof message === 'object' &&
+          typeof message.cmd === 'string' &&


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

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

refack · 2017-06-05T03:08:45Z

lib/internal/child_process.js

@@ -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]);


Why not check for isInternal on the raw chunk?

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

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

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

refack · 2017-06-05T04:35:10Z

lib/internal/child_process.js

-    target.emit(eventName, message, handle);
-  });
+function isInternal(message) {
+  return (message !== null &&


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

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.

refack · 2017-06-05T04:52:22Z

@mscdex Thanks for the answers 🎩

mscdex · 2017-06-08T06:40:52Z

Results with V8 5.9 now in master:

                                                                           improvement confidence      p.value
 cluster/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1      11.22 %        *** 4.196927e-10
 cluster/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1      14.63 %        *** 4.344752e-10
 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1     17.93 %        *** 1.171912e-15
 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1     26.25 %        *** 6.978421e-19

bnoordhuis

Nice optimization, LGTM.

bnoordhuis · 2017-06-08T08:07:26Z

benchmark/cluster/echo.js

+        return;
+      }
+      for (id in cluster.workers) {
+        const worker = cluster.workers[id];


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.)

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).

PR-URL: nodejs#13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

PR-URL: #13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: nodejs#6909 Refs: nodejs#13459 Refs: nodejs#13648 PR-URL: nodejs#13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

MylesBorins · 2017-07-17T18:02:45Z

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate

This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

mscdex added child_process Issues and PRs related to the child_process subsystem. performance Issues and PRs related to the performance of Node.js. labels Jun 5, 2017

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 5, 2017

mscdex force-pushed the child_process-ipc-perf branch from 8dfec59 to a1bd681 Compare June 5, 2017 00:27

refack approved these changes Jun 5, 2017

View reviewed changes

refack reviewed Jun 5, 2017

View reviewed changes

bnoordhuis approved these changes Jun 8, 2017

View reviewed changes

mscdex added 2 commits June 9, 2017 01:38

child_process: simplify send() result handling

dd83d11

PR-URL: nodejs#13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

child_process: reduce nextTick() usage

8208fda

PR-URL: nodejs#13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

mscdex force-pushed the child_process-ipc-perf branch from a1bd681 to 8208fda Compare June 9, 2017 05:39

mscdex merged commit 8208fda into nodejs:master Jun 9, 2017

mscdex deleted the child_process-ipc-perf branch June 9, 2017 05:41

addaleax pushed a commit that referenced this pull request Jun 10, 2017

child_process: simplify send() result handling

d1fa59f

PR-URL: #13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax pushed a commit that referenced this pull request Jun 10, 2017

child_process: reduce nextTick() usage

0ca4bd1

PR-URL: #13459 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

mscdex mentioned this pull request Jun 13, 2017

child_process: remove nextTick on IPC message #13648

Closed

2 tasks

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

cjihrig mentioned this pull request Jun 21, 2017

child_process: fix IPC 'message' event regression #13856

Merged

3 tasks

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Jul 17, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: improve ipc performance #13459

child_process: improve ipc performance #13459

mscdex commented Jun 5, 2017 •

edited

Loading

refack Jun 5, 2017

mscdex Jun 5, 2017

refack Jun 5, 2017

mscdex Jun 5, 2017

refack Jun 5, 2017

mscdex Jun 5, 2017 •

edited

Loading

refack Jun 5, 2017

mscdex Jun 5, 2017

refack Jun 5, 2017

mscdex Jun 5, 2017 •

edited

Loading

refack commented Jun 5, 2017

mscdex commented Jun 8, 2017

bnoordhuis left a comment

bnoordhuis Jun 8, 2017

mscdex Jun 8, 2017 •

edited

Loading

MylesBorins commented Jul 17, 2017

child_process: improve ipc performance #13459

child_process: improve ipc performance #13459

Conversation

mscdex commented Jun 5, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Jun 5, 2017

mscdex commented Jun 8, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Jul 17, 2017

mscdex commented Jun 5, 2017 •

edited

Loading

mscdex Jun 5, 2017 •

edited

Loading

mscdex Jun 5, 2017 •

edited

Loading

mscdex Jun 8, 2017 •

edited

Loading