FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Optimize child_process IPC for large data by ypresto · Pull Request #10557 · nodejs/node · GitHub

/ node Public

Optimize child_process IPC for large data - #10557

Closed
ypresto wants to merge 6 commits into
nodejs:masterfrom
ypresto:optimize-ipc
ConversationCommits (6)ChecksFiles changed
Closed

Optimize child_process IPC for large data#10557
ypresto wants to merge 6 commits into
nodejs:masterfrom
ypresto:optimize-ipc

Conversation

ypresto commented Dec 31, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. lts-watch-v6.x labels Dec 31, 2016
ypresto mentioned this pull request Dec 31, 2016
Closed
mscdex reviewed Dec 31, 2016
View reviewed changes
Comment thread lib/internal/child_process.js Outdated
//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) {

mscdex Dec 31, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

forEach() is slower than a normal for loop.

mscdex added the performance Issues and PRs related to the performance of Node.js. label Dec 31, 2016

ypresto commented Dec 31, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Jan 1, 2017

Copy link
Copy Markdown

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

mscdex commented Jan 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

ypresto commented Jan 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

addaleax reviewed Jan 1, 2017
View reviewed changes
Comment thread lib/internal/child_process.js Outdated

addaleax Jan 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jan 1, 2017

Copy link
Copy Markdown
Contributor Author

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

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

addaleax mentioned this pull request Jan 6, 2017
Closed
addaleax approved these changes Jan 6, 2017
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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
benchmark: cleanup child_process IPC benchmark
f4e1fe3 
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
child_process: optimize IPC for large data
004f2e4 
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
benchmark: cleanup child_process IPC benchmark
fea95d7 
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
child_process: optimize IPC for large data
b831d9a 
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
benchmark: cleanup child_process IPC benchmark
1436555 
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
child_process: optimize IPC for large data
cdfa056 
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 mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
benchmark: cleanup child_process IPC benchmark
2f339e7 
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
child_process: optimize IPC for large data
ddcd1a2 
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 mentioned this pull request Jan 29, 2017
Merged

MylesBorins commented Mar 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

kuychaco mentioned this pull request Apr 26, 2017
Merged

MylesBorins commented May 8, 2017

Copy link
Copy Markdown
Contributor

ping

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
benchmark: cleanup child_process IPC benchmark
32fa37f 
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
child_process: optimize IPC for large data
c13429a 
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 commented May 9, 2017

Copy link
Copy Markdown
Member

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

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 mentioned this pull request May 23, 2017
Merged
BinaryMuse mentioned this pull request Jul 10, 2017
Closed
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
benchmark: cleanup child_process IPC benchmark
1b0e927 
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
child_process: optimize IPC for large data
72d0200 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
addaleax addaleax approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL