net: write syscall optimization and a bytesWritten bug fix #2960

brendanashworth · 2015-09-19T00:13:07Z

This pull request contains two changes to net: one that uses .cork()/.uncork() on a socket as it is connecting, and the other to fix the value of .bytesWritten while a socket is connecting and a ._writev call is issued.

`.cork()` / `.uncork()`

By using these two values on the Writable stream of the socket while it is connecting, we can avoid the common stream-ism of a single buffered ._write, then a larger ._writev following it. This change merges multiple writes to a connecting stream into a single writev call, rather than two calls as before. Here is an explanation gist.

`.bytesWritten` fix

As found by reading over the code base, if a writev call is issued and cached (while connecting), _pendingData will be an array. Because there was no special functionality for this, Buffer.byteLength coerced the array to a string and returned the byte length of that. This fixes that bug and adds a regression test.

ronkorving · 2015-09-19T01:01:47Z

Brilliant. We could apply the same cork trick for fs.WriteStream and gain some benefits there.

brendanashworth · 2015-10-12T22:11:35Z

Perhaps r= @trevnorris or @bnoordhuis ?

trevnorris · 2015-10-12T23:17:16Z

lib/net.js

+  this.cork();
+  this.once('connect', function() {
+    this.uncork();
+  });


Sorry, I'm confused. What does this gain?

It is supposed to keep the initial write (if written to multiple times when connecting) inside the stream buffer - then, when it connects (and is writable), all the writes can be flushed at once (with writev), rather than a single write then a writev with the rest.

hm. wonder how this would have worked before. I assume writes to an unconnected socket would have either errored or been lost.

Before they were kept in the _pendingData, _pendingEncoding things, and the callback wouldn't be called, so the streams would buffer the rest. This still happens after this commit though (on a smaller scale with _pendingBuffer) because .end() uncorks completely.

Is there a reason you cork eagerly here instead of lazily in Socket#_writeGeneric()? The change LGTM, just wondering.

By corking eagerly, we make sure (as often as possible) that the writes stay in the stream infrastructure rather than like before, where one sat in net.js.

trevnorris · 2015-10-13T18:32:06Z

I feel like this will have some interactions with the automatic cork/uncork in the http module. Could those be potentially removed and rely on this instead?

brendanashworth · 2015-10-14T05:27:06Z

@trevnorris I'll take a look - but this can only affect the HTTP client, not the server, so that may limit its usefulness. :/

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

trevnorris · 2015-10-22T18:16:18Z

Don't see any test failures related to this PR.

jasnell · 2016-03-22T05:32:17Z

Is this still an issue?

Instead of allowing the socket to lazily buffer up a write while connecting, proactively .cork() the stream and .uncork() when we have connected. This allows the stream to buffer together all writes while connecting into a single writev when connected, rather than an initial write and a follow-up writev. This only leads us to a small caveat: if the stream is uncorked while connecting, writes will begin to be sent to the socket unintentionally. Work around this by preserving a smaller subset of the _pendingData and _pendingEncoding stuff, which can be used in this case. (This also happens when .end() is called on a socket which is still connecting.) This means this will be turned into a single write call: var socket = net.connect(...); socket.write('hello, wor'); socket.write('ld!'); Improving performance when this is the case.

When a writev is caused on a socket (sometimes through corking and uncorking), previously net would call Buffer.byteLength on the array of buffers and chunks, giving a completely erroneous byte length for the bulk of them (this is because byteLength is weird and coerces to strings). This commit fixes this bug by iterating over each chunk in the pending stack and calculating the length individually. Also adds a regression test.

brendanashworth · 2016-03-25T22:50:39Z

@jasnell it could still be fixed, it just needs someone to sign off on it. I've rebased the branch.

jasnell · 2016-03-26T04:31:20Z

/cc @nodejs/ctc

indutny · 2016-03-26T04:40:58Z

A question, will it crash if someone will .uncork() socket right after creation, and will write data to it?

brendanashworth · 2016-03-26T05:04:04Z

@indutny it will not — instead, it will function as it did before, buffering up the data separately, leading to two write calls. So, if it is uncorked, you lose the speed bonus.

indutny · 2016-03-26T14:21:48Z

Ok, good. May I ask you to add a test for this case too? Otherwise LGTM!

jasnell · 2017-02-28T22:55:01Z

@brendanashworth ... is this still something you'd like to pursue?

fhinkel · 2017-03-26T10:49:15Z

Feel free to reopen this PR if you get back to working on it.

When a writev is caused on a socket (sometimes through corking and uncorking), previously net would call Buffer.byteLength on the array of buffers and chunks. This throws a TypeError, because Buffer.byteLength throws when passed a non-string. In dbfe8c4, behavior changed to throw when passed a non-string. This is correct behavior. Previously, it would cast the argument to a string, so before this commit, bytesWritten would give an erroneous value. This commit corrects the behavior equally both before and after dbfe8c4. This commit fixes this bug by iterating over each chunk in the pending stack and calculating the length individually. Also adds a regression test. Refs: nodejs#2960

When a writev is caused on a socket (sometimes through corking and uncorking), previously net would call Buffer.byteLength on the array of buffers and chunks. This throws a TypeError, because Buffer.byteLength throws when passed a non-string. In dbfe8c4, behavior changed to throw when passed a non-string. This is correct behavior. Previously, it would cast the argument to a string, so before this commit, bytesWritten would give an erroneous value. This commit corrects the behavior equally both before and after dbfe8c4. This commit fixes this bug by iterating over each chunk in the pending stack and calculating the length individually. Also adds a regression test. This additionally changes an `instanceof Buffer` check to `typeof !== 'string'`, which should be equivalent. PR-URL: #14236 Reviewed-By: Brian White <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Refs: #2960

brendanashworth added the net Issues and PRs related to the net subsystem. label Sep 19, 2015

trevnorris reviewed Oct 12, 2015
View reviewed changes

anderfjord mentioned this pull request Nov 6, 2015

Incompatible with Node 4.2.1 mattcg/socks5-client#10

Closed

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016

brendanashworth added 2 commits March 25, 2016 15:24

brendanashworth force-pushed the new/net-socket-cork branch from faa1eba to 63e3030 Compare March 25, 2016 22:49

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

fhinkel closed this Mar 26, 2017

brendanashworth mentioned this pull request Jul 14, 2017

net: fix bytesWritten during writev #14236

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: write syscall optimization and a bytesWritten bug fix #2960

net: write syscall optimization and a bytesWritten bug fix #2960

brendanashworth commented Sep 19, 2015

ronkorving commented Sep 19, 2015

brendanashworth commented Oct 12, 2015

trevnorris Oct 12, 2015

brendanashworth Oct 12, 2015

trevnorris Oct 12, 2015

brendanashworth Oct 12, 2015

bnoordhuis Oct 13, 2015

brendanashworth Oct 14, 2015

trevnorris commented Oct 13, 2015

brendanashworth commented Oct 14, 2015

trevnorris commented Oct 22, 2015

jasnell commented Mar 22, 2016

brendanashworth commented Mar 25, 2016

jasnell commented Mar 26, 2016

indutny commented Mar 26, 2016

brendanashworth commented Mar 26, 2016

indutny commented Mar 26, 2016

jasnell commented Feb 28, 2017

fhinkel commented Mar 26, 2017

net: write syscall optimization and a bytesWritten bug fix #2960

net: write syscall optimization and a bytesWritten bug fix #2960

Conversation

brendanashworth commented Sep 19, 2015

.cork() / .uncork()

.bytesWritten fix

ronkorving commented Sep 19, 2015

brendanashworth commented Oct 12, 2015

trevnorris Oct 12, 2015

Choose a reason for hiding this comment

brendanashworth Oct 12, 2015

Choose a reason for hiding this comment

trevnorris Oct 12, 2015

Choose a reason for hiding this comment

brendanashworth Oct 12, 2015

Choose a reason for hiding this comment

bnoordhuis Oct 13, 2015

Choose a reason for hiding this comment

brendanashworth Oct 14, 2015

Choose a reason for hiding this comment

trevnorris commented Oct 13, 2015

brendanashworth commented Oct 14, 2015

trevnorris commented Oct 22, 2015

jasnell commented Mar 22, 2016

brendanashworth commented Mar 25, 2016

jasnell commented Mar 26, 2016

indutny commented Mar 26, 2016

brendanashworth commented Mar 26, 2016

indutny commented Mar 26, 2016

jasnell commented Feb 28, 2017

fhinkel commented Mar 26, 2017

`.cork()` / `.uncork()`

`.bytesWritten` fix