pipe: enable writev for pipe handles on Unix #10677

aqrln · 2017-01-07T15:32:54Z

This PR enables writev for Unix Domain Sockets on supported platforms thus enabling cork/uncork functionality for them and increasing IPC performance.

Only those Unix-like systems that are confirmed to support the feature (Linux, macOS and FreeBSD) are explicitly listed in the define guard. If anyone has an opportunity to test it under other platforms supported by Node, the list should be expanded, inverted into list of platforms that do not support the feature or even changed to a simple Windows/Unix check.

It should fix #5095 and similar issues, but I haven't benchmarked this specific case and haven't prepared any public benchmarks yet. In my usecase it gives about 1.3–1.4x performance increase on Linux (but strangely the same performance on macOS).

UPD: after discussion on IRC and experiment on CI the patch is enabled on all platforms except Windows.

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)

pipe, stream_wrap

jasnell · 2017-01-07T18:15:57Z

/cc @indutny @bnoordhuis

aqrln · 2017-01-08T00:28:56Z

I ran node_redis benchmark and this patch indeed seems to fix #5095.

Full results: https://gist.github.com/aqrln/6fb1af97459e207ec654e75a6fc4cd00

Comparison of benchmarks before and after the patch on a Linux VM:

Comparing before, linux-before.txt ( 37 lines) to after, linux-after.txt ( 37 lines)
clients: 1:        0 ->      0 ops/sec (∆ -      0 -    NaN%)
PING:          27089 ->  30315 ops/sec (∆ +   3226 +  11.91%)
PING:         618513 -> 668113 ops/sec (∆ +  49600 +   8.02%)
SET 4B str:    25407 ->  26432 ops/sec (∆ +   1025 +   4.03%)
SET 4B str:   432767 -> 446921 ops/sec (∆ +  14154 +   3.27%)
SET 4B buf:    12026 ->  12261 ops/sec (∆ +    235 +   1.95%)
SET 4B buf:    17086 -> 171152 ops/sec (∆ + 154066 + 901.71%)
GET 4B str:    28040 ->  28977 ops/sec (∆ +    937 +   3.34%)
GET 4B str:   507737 -> 525030 ops/sec (∆ +  17293 +   3.41%)
GET 4B buf:    23984 ->  27532 ops/sec (∆ +   3548 +  14.79%)
GET 4B buf:   454798 -> 523850 ops/sec (∆ +  69052 +  15.18%)
SET 4KiB str:  20527 ->  23892 ops/sec (∆ +   3365 +  16.39%)
SET 4KiB str: 140304 -> 154618 ops/sec (∆ +  14314 +  10.20%)
SET 4KiB buf:  10802 ->  11967 ops/sec (∆ +   1165 +  10.79%)
SET 4KiB buf:  15961 -> 129128 ops/sec (∆ + 113167 + 709.02%)
GET 4KiB str:  23923 ->  25663 ops/sec (∆ +   1740 +   7.27%)
GET 4KiB str: 171351 -> 187105 ops/sec (∆ +  15754 +   9.19%)
GET 4KiB buf:  23174 ->  26569 ops/sec (∆ +   3395 +  14.65%)
GET 4KiB buf: 144582 -> 163794 ops/sec (∆ +  19212 +  13.29%)
INCR:          25148 ->  27120 ops/sec (∆ +   1972 +   7.84%)
INCR:         471591 -> 534386 ops/sec (∆ +  62795 +  13.32%)
LPUSH:         24301 ->  26746 ops/sec (∆ +   2445 +  10.06%)
LPUSH:        383287 -> 441743 ops/sec (∆ +  58456 +  15.25%)
LRANGE 10:     23138 ->  25018 ops/sec (∆ +   1880 +   8.13%)
LRANGE 10:    204398 -> 236106 ops/sec (∆ +  31708 +  15.51%)
LRANGE 100:    15573 ->  17351 ops/sec (∆ +   1778 +  11.42%)
LRANGE 100:    33627 ->  37870 ops/sec (∆ +   4243 +  12.62%)
SET 4MiB str:    196 ->    215 ops/sec (∆ +     19 +   9.69%)
SET 4MiB str:    193 ->    221 ops/sec (∆ +     28 +  14.51%)
SET 4MiB buf:    398 ->    461 ops/sec (∆ +     63 +  15.83%)
SET 4MiB buf:    415 ->    468 ops/sec (∆ +     53 +  12.77%)
GET 4MiB str:    415 ->    466 ops/sec (∆ +     51 +  12.29%)
GET 4MiB str:    211 ->    227 ops/sec (∆ +     16 +   7.58%)
GET 4MiB buf:    295 ->    321 ops/sec (∆ +     26 +   8.81%)
GET 4MiB buf:    296 ->    318 ops/sec (∆ +     22 +   7.43%)
:              85250 ->  85161 ops/sec (∆ -     89 -   0.10%)
Mean difference in ops/sec:  +18075.4

addaleax · 2017-01-08T22:45:54Z

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

addaleax · 2017-01-08T23:23:46Z

test/parallel/test-pipe-writev.js

+    });
+
+    const writev = connection._writev.bind(connection);
+    connection._writev = common.mustCall(writev);


clever trick!

addaleax · 2017-01-08T23:24:51Z

test/parallel/test-pipe-writev.js

+    throw err;
+  });
+
+  server.listen(common.PIPE, startClient.bind(null, server));


I’d find () => startClient(server) a bit easier to read than .bind()

addaleax · 2017-01-08T23:25:05Z

test/parallel/test-pipe-writev.js

+  });
+
+  client.on('data', common.mustCall((data) => {
+    assert.equal(data.toString(), 'ping');


Can you use assert.strictEqual here?

Agree, it's better to use strictEqual.

addaleax · 2017-01-08T23:25:34Z

test/parallel/test-pipe-writev.js

+
+common.refreshTmpDir();
+
+function startServer() {


This doesn’t really need to be wrapped in a function, does it?

It doesn't. It is only for readability and consistency with startClient.

...well, at least if this startClient function exists. I'll simplify the code so that it resolves this remark and the one related to bind at once.

addaleax

LGTM but I’d prefer it if @bnoordhuis or @indutny had a look at it too

addaleax · 2017-01-08T23:48:32Z

test/parallel/test-pipe-writev.js

+  setTimeout(() => {
+    connection.write('ng');
+    connection.end();
+  }, 200);


Is this timeout value relevant? Could it be smaller, or maybe even a setImmediate?

Hmm... maybe it is even better to remove the setTimeout completely since there's a common.mustCall on connection._writev and even 200 ms timeout doesn't guarantee that a client connection would emit two data events if server used write instead of writev.

So I tested it and yeah, even setTimeout is useless here, not to mention process.nextTick. I'll remove it completely. Though let it be two writes, just in case.

indutny

LGTM

trevnorris · 2017-01-09T18:47:49Z

@aqrln you mentioned in IRC that this fixes an issue I self assigned. If that's the case, mind adding the Fixes: field in the commit message?

aqrln · 2017-01-09T18:56:45Z

@trevnorris sure, my bad. I forgot to add it.

jasnell · 2017-01-10T07:25:00Z

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

jasnell · 2017-01-10T19:27:33Z

@trevnorris ... does this LGTY?

trevnorris · 2017-01-11T20:33:49Z

@aqrln Sorry for being pedantic but the full URL should be used. not just the issue number. other than that, LGTM

This commit enables writev for Unix Domain Sockets on supported platforms thus enabling cork/uncork functionality for them and improving IPC performance. Fixes: nodejs#5095

aqrln · 2017-01-11T22:00:24Z

@trevnorris thanks for pointing that out, I've updated the commit message.

aqrln · 2017-01-12T00:15:48Z

@jasnell I hope this should be ready to land :)

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) #10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) #10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 PR-URL: #11760

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) nodejs/node#10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) nodejs/node#10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. lts-watch-v6.x labels Jan 7, 2017

aqrln force-pushed the pipe-cork branch from a66be94 to f847d5a Compare January 7, 2017 18:58

nodejs-github-bot added the lts-watch-v6.x label Jan 7, 2017

addaleax reviewed Jan 8, 2017

View reviewed changes

aqrln force-pushed the pipe-cork branch from 0a5865b to 0efb3f3 Compare January 8, 2017 23:43

nodejs-github-bot added the lts-watch-v6.x label Jan 8, 2017

addaleax approved these changes Jan 8, 2017

View reviewed changes

addaleax reviewed Jan 8, 2017

View reviewed changes

aqrln force-pushed the pipe-cork branch from 0efb3f3 to 6f9efd5 Compare January 9, 2017 00:02

nodejs-github-bot added the lts-watch-v6.x label Jan 9, 2017

indutny approved these changes Jan 9, 2017

View reviewed changes

aqrln force-pushed the pipe-cork branch from 6f9efd5 to ddb8bb4 Compare January 9, 2017 18:56

nodejs-github-bot added the lts-watch-v6.x label Jan 9, 2017

trevnorris approved these changes Jan 11, 2017

View reviewed changes

pipe: enable writev for pipe handles on Unix

c8ea04b

This commit enables writev for Unix Domain Sockets on supported platforms thus enabling cork/uncork functionality for them and improving IPC performance. Fixes: nodejs#5095

aqrln force-pushed the pipe-cork branch from ddb8bb4 to c8ea04b Compare January 11, 2017 21:58

nodejs-github-bot added the lts-watch-v6.x label Jan 11, 2017

jasnell approved these changes Jan 12, 2017

View reviewed changes

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

aqrln deleted the pipe-cork branch March 10, 2017 04:20

adamlaska mentioned this pull request Nov 25, 2023

[Snyk] Fix for 6 vulnerabilities adamlaska/node#112

Open

aliscco mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 aliscco/alisco-node#665

Open

leonardoadame mentioned this pull request Nov 28, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#187

Open

This was referenced Nov 28, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 aliscco/alisco-node#691

Open

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#698

Open

This was referenced Nov 29, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 leonardoadame/node#212

Open

[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 leonardoadame/node#213

Open

This was referenced Nov 29, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#723

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#727

Open

[Snyk] Fix for 4 vulnerabilities aliscco/alisco-node#746

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#239

Open

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#766

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#262

Open

This was referenced Nov 30, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/alisco-node#822

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#830

Open

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#835

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#840

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pipe: enable writev for pipe handles on Unix #10677

pipe: enable writev for pipe handles on Unix #10677

aqrln commented Jan 7, 2017 •

edited

Loading

jasnell commented Jan 7, 2017

aqrln commented Jan 8, 2017 •

edited

Loading

addaleax commented Jan 8, 2017

addaleax Jan 8, 2017

addaleax Jan 8, 2017

addaleax Jan 8, 2017

aqrln Jan 8, 2017

addaleax Jan 8, 2017

aqrln Jan 8, 2017

aqrln Jan 8, 2017 •

edited

Loading

addaleax left a comment

addaleax Jan 8, 2017

aqrln Jan 8, 2017 •

edited

Loading

aqrln Jan 8, 2017

indutny left a comment

trevnorris commented Jan 9, 2017

aqrln commented Jan 9, 2017

jasnell commented Jan 10, 2017

jasnell commented Jan 10, 2017

trevnorris commented Jan 11, 2017

aqrln commented Jan 11, 2017

aqrln commented Jan 12, 2017

pipe: enable writev for pipe handles on Unix #10677

pipe: enable writev for pipe handles on Unix #10677

Conversation

aqrln commented Jan 7, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell commented Jan 7, 2017

aqrln commented Jan 8, 2017 • edited Loading

addaleax commented Jan 8, 2017

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

Choose a reason for hiding this comment

aqrln Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny left a comment

Choose a reason for hiding this comment

trevnorris commented Jan 9, 2017

aqrln commented Jan 9, 2017

jasnell commented Jan 10, 2017

jasnell commented Jan 10, 2017

trevnorris commented Jan 11, 2017

aqrln commented Jan 11, 2017

aqrln commented Jan 12, 2017

aqrln commented Jan 7, 2017 •

edited

Loading

aqrln commented Jan 8, 2017 •

edited

Loading

aqrln Jan 8, 2017 •

edited

Loading

aqrln Jan 8, 2017 •

edited

Loading