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

Optimize child_process IPC for large data #10557

Closed
wants to merge 6 commits into from

Conversation

ypresto
Copy link
Contributor

@ypresto ypresto commented Dec 31, 2016

jsonBuffer.indexOf('\n', start) in channel.onread (internal/child_process.js) dramatically slows down when jsonBuffer is very large.
jsonBuffer does not contains linebreak before jsonBuffer += decoder.write(pool) call, so checking linebreaks in return value of decoder.write(pool) is enough to parse chunks.

This increases total bytes received in 5 secs by 2-4x in benchmark...!

I found this when developing vscode plugin which returns very large result (around 10MB):
rubyide/vscode-ruby#107
Also vscode core has workaround for this bottleneck:
microsoft/vscode#6026 (comment)

Also fixed benchmark script which only shows zero.

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

Benchmark result

Mac OS X 10.12.2
MacBook Pro (Retina, 15-inch, Mid 2014)
2.2 GHz Intel Core i7

Before:
child_process/child-process-read-ipc.js dur=5 len=64: 7,583,464
child_process/child-process-read-ipc.js dur=5 len=256: 10,687,553
child_process/child-process-read-ipc.js dur=5 len=1024: 6,279,313
child_process/child-process-read-ipc.js dur=5 len=4096: 6,319,896
child_process/child-process-read-ipc.js dur=5 len=16384: 6,482,388
child_process/child-process-read-ipc.js dur=5 len=65536: 8,580,203
child_process/child-process-read-ipc.js dur=5 len=1048576: 13,398,807
child_process/child-process-read-ipc.js dur=5 len=16777216: 0

After:
child_process/child-process-read-ipc.js dur=5 len=64: 9,131,734
child_process/child-process-read-ipc.js dur=5 len=256: 10,502,876
child_process/child-process-read-ipc.js dur=5 len=1024: 6,272,325
child_process/child-process-read-ipc.js dur=5 len=4096: 6,195,482
child_process/child-process-read-ipc.js dur=5 len=16384: 6,505,698
child_process/child-process-read-ipc.js dur=5 len=65536: 8,513,730
child_process/child-process-read-ipc.js dur=5 len=1048576: 40,615,305
child_process/child-process-read-ipc.js dur=5 len=16777216: 30,183,113


When setTimeout() in benchmark is 0:
(I have no idea why this produces quite different result.)

Before:
child_process/child-process-read-ipc.js dur=5 len=64: 11,924,365
child_process/child-process-read-ipc.js dur=5 len=256: 34,004,413
child_process/child-process-read-ipc.js dur=5 len=1024: 51,082,451
child_process/child-process-read-ipc.js dur=5 len=4096: 61,968,471
child_process/child-process-read-ipc.js dur=5 len=16384: 59,297,771
child_process/child-process-read-ipc.js dur=5 len=65536: 59,533,374
child_process/child-process-read-ipc.js dur=5 len=1048576: 6,704,920
child_process/child-process-read-ipc.js dur=5 len=16777216: 0

After:
child_process/child-process-read-ipc.js dur=5 len=64: 12,209,873
child_process/child-process-read-ipc.js dur=5 len=256: 36,964,340
child_process/child-process-read-ipc.js dur=5 len=1024: 50,775,987
child_process/child-process-read-ipc.js dur=5 len=4096: 79,817,420
child_process/child-process-read-ipc.js dur=5 len=16384: 92,115,253
child_process/child-process-read-ipc.js dur=5 len=65536: 105,704,985
child_process/child-process-read-ipc.js dur=5 len=1048576: 23,256,095
child_process/child-process-read-ipc.js dur=5 len=16777216: 0

//Linebreak is used as a message end sign
while ((i = jsonBuffer.indexOf('\n', start)) >= 0) {
var json = jsonBuffer.slice(start, i);
chunks.forEach(function(json) {
Copy link
Contributor

Choose a reason for hiding this comment

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

forEach() is slower than a normal for loop.

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Dec 31, 2016
@ypresto
Copy link
Contributor Author

ypresto commented Dec 31, 2016

forEach() removed!
New performance test result:

setTimeout() is 20:
child_process/child-process-read-ipc.js dur=5 len=64: 13,380,196
child_process/child-process-read-ipc.js dur=5 len=256: 10,571,078
child_process/child-process-read-ipc.js dur=5 len=1024: 5,688,590
child_process/child-process-read-ipc.js dur=5 len=4096: 5,898,209
child_process/child-process-read-ipc.js dur=5 len=16384: 6,037,993
child_process/child-process-read-ipc.js dur=5 len=65536: 8,054,859
child_process/child-process-read-ipc.js dur=5 len=1048576: 36,042,245
child_process/child-process-read-ipc.js dur=5 len=16777216: 43,590,387

setTimeout() is 0:
child_process/child-process-read-ipc.js dur=5 len=64: 13,237,553
child_process/child-process-read-ipc.js dur=5 len=256: 38,048,327
child_process/child-process-read-ipc.js dur=5 len=1024: 54,938,211
child_process/child-process-read-ipc.js dur=5 len=4096: 83,703,421
child_process/child-process-read-ipc.js dur=5 len=16384: 89,017,635
child_process/child-process-read-ipc.js dur=5 len=65536: 101,471,791
child_process/child-process-read-ipc.js dur=5 len=1048576: 28,087,643
child_process/child-process-read-ipc.js dur=5 len=16777216: 0

@YurySolovyov
Copy link

I wonder how it performs with setImmediate or possibly other ways of queueing up callbacks

@mscdex
Copy link
Contributor

mscdex commented Jan 1, 2017

I don't think the setTimeout() is a good change to the benchmark. As @YurySolovyov suggested, perhaps setImmediate() is a more suitable replacement?

@ypresto
Copy link
Contributor Author

ypresto commented Jan 1, 2017

58f6cc0 fixes '"${msg}"' is used so actually above benchmarks uses len + 2 size message.

New benchmark result with setImmediate():

child_process/child-process-read-ipc.js dur=5 len=64: 9,758,913
child_process/child-process-read-ipc.js dur=5 len=256: 34,099,862
child_process/child-process-read-ipc.js dur=5 len=1024: 101,673,385
child_process/child-process-read-ipc.js dur=5 len=4096: 143,175,819
child_process/child-process-read-ipc.js dur=5 len=16384: 82,422,291
child_process/child-process-read-ipc.js dur=5 len=65536: 34,661,893
child_process/child-process-read-ipc.js dur=5 len=1048576: 2,096,105
child_process/child-process-read-ipc.js dur=5 len=16777216: 0

@@ -498,9 +498,9 @@ function setupChannel(target, channel) {
var queue = target._handleQueue;
target._handleQueue = null;

queue.forEach(function(args) {
for (var args in queue) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? for (var … in …) iterates over the indices of the array, so it does not do the same thing as for (var i = 0; i < args.length; ++i)

@ypresto
Copy link
Contributor Author

ypresto commented Jan 1, 2017

for (var … in …) iterates over the indices of the array

Oops... test was failing. Fixed in de82d97..!

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

The changes in lib LGTM, can’t really speak up for the benchmark part

CI: https://ci.nodejs.org/job/node-test-commit/7051/

@ypresto ypresto deleted the optimize-ipc branch January 16, 2017 06:04
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: nodejs#3145
PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: nodejs#3145
PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: nodejs#3145
PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: nodejs#3145
PR-URL: nodejs#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 8, 2017

Is this something we will want to backport? Should it bake a bit longer first?

@MylesBorins
Copy link
Contributor

ping

@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed lts-watch-v4.x labels May 8, 2017
addaleax pushed a commit that referenced this pull request May 9, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: #10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request May 9, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: #3145
PR-URL: #10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

addaleax commented May 9, 2017

@MylesBorins This should be fine to backport to v6.x… I’ve cherry-picked the changes here to v6.x-staging

@addaleax addaleax added land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 9, 2017
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Squashed from:
- child_process: fix IPC bench to obey send() ret val
- child_process: fix IPC benchmark message has two more bytes
- child_process: use setImmediate for IPC bench

PR-URL: nodejs/node#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Squashed from:
- child_process: stop indexOf() on whole IPC buffer
- child_process: get rid of forEach() and slice() in IPC
- child_process: get rid of another forEach() in IPC

Fixes: nodejs/node#3145
PR-URL: nodejs/node#10557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants