Greatly speed up 'advanced' ipc receiving with big messages by CaramelFur · Pull Request #42931 · nodejs/node

CaramelFur · 2022-04-30T21:25:16Z

Problem

I was running into an issue where I needed to transfer large buffers between a child_process and the main process with 'advanced' serializing, and this was suprisingly very slow.

Solution

After some debugging and profiling I found that the Buffer functions, and mainly the Buffer.concat function were to blame. So I have changed the code to use these functions as little as possible.

Now it will not concat each and every incoming part, it will instead store these in an array and only concat them when the full message has been collected.

Tests

I wrote a small test where I transferred different buffer sizes from a child process to the parent, and timed how long it took before the message was received in the parent.

(The scripts I used to test this: test-ipc.zip)

Before fix:

1kb : 1 ms
8kb : 0 ms
64kb : 0 ms
512kb : 1 ms
1mb : 6 ms
2mb : 17 ms
4mb : 43 ms
8mb : 176 ms
16mb : 717 ms
32mb : 2726 ms
64mb : 11781 ms
128mb : 57075 ms
1gb : 3299888 ms (55 minutes)

After fix:

1kb : 1 ms
8kb : 0 ms
64kb : 0 ms
512kb : 1 ms
1mb : 2 ms
2mb : 3 ms
4mb : 6 ms
8mb : 15 ms
16mb : 31 ms
32mb : 73 ms
64mb : 142 ms
128mb : 217 ms
1gb : 1426 ms

BridgeAR

This looks really good! Just left a suggestion.

BridgeAR · 2022-05-01T13:30:50Z

Suggested change
// We read the uint manually here, because this is faster than first converting
// it to a buffer and using `readUInt32BE` on that.
constsize=
messageBufferHead[0]<<24|
messageBufferHead[1]<<16|
messageBufferHead[2]<<8|
messageBufferHead[3];
constsize=Buffer.prototype.readUInt32BE.call(messageBufferHead,0)+4;
This might work as well? I added the plus 4 as well. That way there's no need to add it later on. This might require a new variable name as well.

I have added these suggestions, since I can't see any noticable performance impact of this after some testing, and it makes the code easier to read.

nodejs-github-bot · 2022-05-01T19:29:41Z

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

CaramelFur · 2022-05-01T21:14:08Z

@lpinca Is it normal for the osx tests to fail, or is that because of this change?
Sorry, I'm not familiar with the node CI system.

himself65 · 2022-05-02T00:36:03Z

const{ readUInt32BE }=require('internal/buffer');// ...constfullMessageSize=ReflectApply(readUInt32BE,messageBufferHead,[0])+4;

Change has been added

himself65 · 2022-05-02T01:08:11Z

@lpinca Is it normal for the osx tests to fail, or is that because of this change? Sorry, I'm not familiar with the node CI system.

It's normal for some randomly crash, and I don't know why

anonrig

I've added some comments regarding the usage of primordials which has a documentation on https://github.com/nodejs/node/blob/master/doc/contributing/primordials.md

anonrig · 2022-05-02T03:23:17Z

Usage of primordials should be preferred for any new code. Full documentation can be found from https://github.com/nodejs/node/blob/master/doc/contributing/primordials.md
ArrayPrototypePush(channel[kMessageBuffer],readData);

ArrayPrototypePush is mentioned in the list of primordials with known performance issues- https://github.com/nodejs/node/blob/master/doc/contributing/primordials.md#primordials-with-known-performance-issues, so I'm not sure if it's worth using it here given that this PR is about speeding things up.

I think it's also a little ambiguous what "new code" means here - new APIs (this one is not a new API) or just new lines of code that are not moved in from a different place.

After some testing it seems that the primordial function is about 12% slower than the normal one. However, this does seem to affect the speed of the ipc in any noticible way, probably because most processing time is spent elsewhere. So I don't see any problem with using the primordial function.

himself65

LGTM

CaramelFur · 2022-05-09T09:10:40Z

@himself65 Is there anything else I need to do to get this merged? Or do I just have to wait until a maintainer merges this?

CaramelFur · 2022-06-06T10:21:30Z

@lpinca Hi there, sorry to bother, but this PR has now been a month without any activity. Is this normal, or do I have to do something else to get this merged?

RaisinTen · 2022-06-06T13:14:21Z

This would need a fully green Jenkins CI run before landing.

nodejs-github-bot · 2022-06-06T13:14:46Z

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

RaisinTen · 2022-06-06T13:15:09Z

cc @nodejs/child_process

nodejs-github-bot · 2022-06-12T09:29:24Z

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

bnoordhuis · 2022-06-12T10:07:29Z

Drive-by suggestion: read the int inline, that's both faster and shorter (and arguably easier to read)
Suggested change
constfullMessageSize=ReflectApply(readUInt32BE,messageBufferHead,[0])+4;
constb=messageBufferHead;
constfullMessageSize=b[0]*0x1000000+b[1]*65536+b[2]*256+b[3];

@himself65

Suggested by @himself65 and @anonrig

nodejs-github-bot · 2022-06-16T15:10:11Z

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

nodejs-github-bot · 2022-06-17T08:12:08Z

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

nodejs-github-bot · 2022-06-18T00:24:42Z

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

nodejs-github-bot · 2022-06-18T09:07:13Z

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

nodejs-github-bot · 2022-06-18T22:31:15Z

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

nodejs-github-bot · 2022-06-19T13:08:27Z

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

CaramelFur · 2022-06-19T14:05:50Z

Ah great, all checks have now finally passed.

PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

lpinca · 2022-06-19T14:11:35Z

Landed in 94020ac...8db79cc.

PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

PR-URL: nodejs/node#42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Apr 30, 2022

BridgeAR approved these changes May 1, 2022
View reviewed changes

lpinca approved these changes May 1, 2022
View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2022

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2022

github-actionsBot mentioned this pull request May 2, 2022
CI Reliability 2022-05-02 nodejs/reliability#269
Open
30 tasks

himself65 requested changes May 2, 2022
View reviewed changes

anonrig reviewed May 2, 2022
View reviewed changes

himself65 approved these changes May 2, 2022
View reviewed changes

This was referenced May 3, 2022
CI Reliability 2022-05-03 nodejs/reliability#270
Open
CI Reliability 2022-05-04 nodejs/reliability#271
Open
CI Reliability 2022-05-05 nodejs/reliability#272
Open

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 6, 2022

bnoordhuis reviewed Jun 12, 2022
View reviewed changes

This was referenced Jun 13, 2022
CI Reliability 2022-06-13 nodejs/reliability#299
Open
CI Reliability 2022-06-14 nodejs/reliability#300
Open

CaramelFur added 3 commits June 15, 2022 15:48

child_process: speed up 'advanced' ipc receiving
cfd9ff9

child_process: add the suggestion from@BridgeAR
ff29aec

child_process: use primordials
abd50a0
Suggested by @himself65 and @anonrig

CaramelFur force-pushed the feature/faster-advanced-ipc branch from 7e80934 to e2247c8Compare June 15, 2022 13:58

This comment was marked as outdated.
Sign in to view

github-actionsBot mentioned this pull request Jun 17, 2022
CI Reliability 2022-06-17 nodejs/reliability#303
Open
31 tasks

github-actionsBot mentioned this pull request Jun 18, 2022
CI Reliability 2022-06-18 nodejs/reliability#304
Open
34 tasks

github-actionsBot mentioned this pull request Jun 19, 2022
CI Reliability 2022-06-19 nodejs/reliability#305
Open
30 tasks

lpinca pushed a commit that referenced this pull request Jun 19, 2022
child_process: improve ipc write performance
8db79cc
PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

lpinca closed this Jun 19, 2022

This was referenced Jun 20, 2022
CI Reliability 2022-06-20 nodejs/reliability#306
Open
CI Reliability 2022-06-21 nodejs/reliability#307
Open
CI Reliability 2022-06-22 nodejs/reliability#308
Open

targos pushed a commit that referenced this pull request Jul 12, 2022
child_process: improve ipc write performance
3ca9e65
PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged

targos pushed a commit that referenced this pull request Jul 18, 2022
child_process: improve ipc write performance
567af41
PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

targos pushed a commit that referenced this pull request Jul 31, 2022
child_process: improve ipc write performance
2b6e98b
PR-URL: #42931 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged

zbjornson mentioned this pull request May 31, 2024
perf: fix for cluster aggregation performance prometheus/client_js#631
Closed

MGorkov mentioned this pull request Nov 13, 2024
Hang up with 'advanced' IPC deserialization #55834
Closed

-// We read the uint manually here, because this is faster than first converting
-// it to a buffer and using `readUInt32BE` on that.
-constsize=
-messageBufferHead[0]<<24|
-messageBufferHead[1]<<16|
-messageBufferHead[2]<<8|
-messageBufferHead[3];
+constsize=Buffer.prototype.readUInt32BE.call(messageBufferHead,0)+4;

	constfullMessageSize=ReflectApply(readUInt32BE,messageBufferHead,[0])+4;
	constb=messageBufferHead;
	constfullMessageSize=b[0]0x1000000+b[1]65536+b[2]*256+b[3];

Uh oh!

Uh oh!

Conversation

CaramelFur commented Apr 30, 2022

Problem

Solution

Tests

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaramelFurMay 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 1, 2022

Uh oh!

CaramelFur commented May 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himself65 commented May 2, 2022

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

himself65 left a comment

Choose a reason for hiding this comment

Uh oh!

CaramelFur commented May 9, 2022

Uh oh!

CaramelFur commented Jun 6, 2022

Uh oh!

RaisinTen commented Jun 6, 2022

Uh oh!

nodejs-github-bot commented Jun 6, 2022

Uh oh!

RaisinTen commented Jun 6, 2022

Uh oh!

nodejs-github-bot commented Jun 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jun 16, 2022

Uh oh!

nodejs-github-bot commented Jun 17, 2022

Uh oh!

nodejs-github-bot commented Jun 18, 2022

Uh oh!

nodejs-github-bot commented Jun 18, 2022

Uh oh!

nodejs-github-bot commented Jun 18, 2022

Uh oh!

nodejs-github-bot commented Jun 19, 2022

Uh oh!

CaramelFur commented Jun 19, 2022

Uh oh!

lpinca commented Jun 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

CaramelFurMay 1, 2022•
edited
Loading

CaramelFur commented May 1, 2022•
edited
Loading