http: optimize by corking outgoing requests#7946
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added the performance 
addaleax commented Aug 2, 2016
CI failed with some possibly related failures:
|
ronkorving commented Aug 2, 2016
Makes you wonder if this block could benefit from being changed to multiple sends like we do with Buffers: chunk=len.toString(16)+CRLF+chunk+CRLF;ret=this._send(chunk,encoding,callback); |
brendanashworth commented Aug 3, 2016
I do think those failures are related, so I'm going to do some looking into those. @ronkorving good idea — the string concatenation could be a problem. I might try that, thanks! I'm having slight second thoughts about my approach to this. I think the idea of corking and uncorking on the next tick (like #2020) should be in streams rather than HTTP so I'm going to put together an alternative PR with that in mind. This sort of stuff should also be available to TLS and such. |
jasnell commented Aug 3, 2016
@nodejs/http @nodejs/streams |
mcollina commented Aug 4, 2016
Maybe this might benefit from some string concatenation optimizations, e.g. https://github.com/davidmarkclements/flatstr. cc @davidmarkclements. @brendanashworth#2020 is unrelated to this case. I agree that |
lib/_http_outgoing.js Outdated
There was a problem hiding this comment.
This is not correct. Multiple cork() call increase the corked counter: https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L226-L230.
This needs to be a for loop that calls uncork() until corked is 0.
There was a problem hiding this comment.
AFAIK that would be a backwards-incompatible change. It would change behavior in something like this:
(req,res)=>{res.cork();res.write('should not write');res.destroy();}Because Socket#destroy() stops all IO on the socket, it isn't supposed to flush the last message. With this commit, writing to the stream will only cork it once, and thus we only uncork once, to leave the dev's previous cork-intent there. Looping until we uncork fully would write the message which I don't believe happens right now (edge case, yes 😛 )
There was a problem hiding this comment.
I'm lost. Why are you uncorking on destroy then?
Probably we shouldn't.
There was a problem hiding this comment.
I'm uncorking on destroy because, without it, it broke a test — leaving it corked would be backwards-incompatible as well. Unfortunately this is part of a gray area. There isn't any guarantee that the message would be flushed regardless, as net.js may not be able to write it synchronously. If the message is buffered to be written asynchronously, and res.destroy() is called before that happens, the message will not be written at all. It's an odd gray area, but this is the best way to keep code breakage down.
There was a problem hiding this comment.
which test was failing?
IMHO not transmitting the data when destroy() is hit is the correct behavior of destroy(). destroy is a dangerous operation anyway. @mafintosh what do you think?
Beware that, if the socket was corked twice, we need to call enough uncork() for this to take into effect.
There was a problem hiding this comment.
IMHO not transmitting the data when destroy() is hit is the correct behavior of destroy().
Yes, that's how it behaves right now. .write()can however flush the data synchronously if the socket is ready, so this has undefined behavior:
res.write('this may or may not send');res.destroy();The data may or may not be flushed. This has been around in node forever.
Beware that, if the socket was corked twice, we need to call enough uncork() for this to take into effect.
That's not what I'm doing here — we add only a single cork, so we uncork only once. If the user wants to cork the socket, write to it and destroy it, we shouldn't flush the message. That would be backwards-incompatible.
(edit): which test was failing?
test/parallel/test-http-abort-client.js fails without this change.
There was a problem hiding this comment.
IMHO not transmitting the data when destroy() is hit is the correct behavior of destroy().
Yes, that's how it behaves right now. .write() can however flush the data synchronously if the socket is ready, so this has undefined behavior:
res.write('this may or may not send');res.destroy();The behavior of destroy() is something we should define and clarify throughout core.
I'm ok with this particular change. Can you add a reference to the failing unit test in the comment? Otherwise it will be extremely hard to make the connection when looking at the code.
Beware that, if the socket was corked twice, we need to call enough uncork() for this to take into effect.
That's not what I'm doing here — we add only a single cork, so we uncork only once. If the user wants to cork the socket, write to it and destroy it, we shouldn't flush the message. That would be backwards-incompatible.
In this PR, every time write() is called, this.connection.cork() is called. It is entirely possible that _writableState.corked is greater that one.
res.write('a')res.write('b')res.destroy()// will not uncorkThere was a problem hiding this comment.
The behavior of destroy() is something we should define and clarify throughout core.
Not so much destroy(), it's just the socket.write()API is weird. It returns a boolean about whether or not it works like a sync or async function. It doesn't necessarily have to be fixed, it just has to be worked around because test/parallel/test-http-abort-client.js and userland doesn't always use it perfectly. I'll add a better comment.
It is entirely possible that _writableState.corked is greater that one.
Ah, I think you're right. I'll fix that.
There was a problem hiding this comment.
it's just the socket.write() API is weird. It returns a boolean about whether or not it works like a sync or async function.
This is not true. socket.write() is consistent from the point of view of the stream. It returns true if the user can keep writing, and false otherwise. From an implementation perspective, it will return true for a while after "operations become asynchronous" (that's a simplification) because of the buffering.
Add a unit test for the "write-write-destroy" use case, similar to test/parallel/test-http-abort-client.js.
mcollina commented Aug 4, 2016
Good work! I think it is a good contribution! |
brendanashworth commented Aug 4, 2016
@mcollina thanks for your review! I like your judgement on the streams api, I might open an issue once this PR is 👍 |
mcollina commented Aug 4, 2016
I think this should be semver-major. Any other opinion? |
jasnell commented Aug 4, 2016
I happily defer to your judgement on the semveriness of it. Marking semver-major to be safe. |
jasnell added the semver-major addaleax commented Aug 28, 2016
mcollina commented Aug 28, 2016
@addaleax we are still waiting for some nits to be fixed. However, I would love to see this lands on v7. @brendanashworth how are you with this? Do you need any help? |
brendanashworth commented Aug 28, 2016
@mcollina doing good! It's not so much the nits that I need to spend time working on, but I need to investigate the test failures. I just haven't had the time to ask rvagg for access to one of the nodes yet — I'll try to get it working within the week. |
ronkorving commented Aug 29, 2016
@brendanashworth Any updates on the suggestion I made? |
jbergstroem commented Oct 6, 2016
Just checking in! Keen on progress updates. |
mcollina commented Oct 7, 2016
@brendanashworth I would love to see this landed. Would you like somebody else to continue your work? Have you have more time to work on this? |
jbergstroem commented Oct 7, 2016
I can facilitate access to any test host if need be. Since CI results are long gone, I rebased (no conflicts) and started a new run: https://ci.nodejs.org/job/node-test-commit/5483/ |
brendanashworth commented Oct 7, 2016
Sorry about the lack of updates — I'll be doing the rest of the work on this over the long weekend 😄 happy to see that there's interest in landing this! |
indutny left a comment
indutny left a comment There was a problem hiding this comment.
LGTM if CI is green and benchmarks are good!
brendanashworthforce-pushed the http-fix-cork branch 2 times, most recently from 82dee6b to f3bba20Compare
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
Good work!
LGTM with some nits to be fixed regarding comments.
lib/_http_outgoing.js Outdated
There was a problem hiding this comment.
can you remove "should" here? They are corked.
lib/_http_outgoing.js Outdated
There was a problem hiding this comment.
can you rephrase this? Something like "avoid writing an empty buffer"
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
Sorry for the confusion, not LGTM yet. There is the discussion around _writableState.corked to be finalized.
brendanashworthforce-pushed the http-fix-cork branch from f3bba20 to 1311f0eComparebrendanashworth commented Oct 11, 2016
@mcollina thank you for your very thorough review 😅 please take another look. @ronkorving thanks for your suggestion earlier — I've incorporated it into the latest changes. Regarding the CI and other platforms — there were problems with |
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
Sorry to be picky, but connectionCorkNT is needed
lib/_http_outgoing.js Outdated
There was a problem hiding this comment.
do not allocate a closure here, it will slow things down considerably. Use connectionCorkNT as it was done before.
brendanashworthforce-pushed the http-fix-cork branch 2 times, most recently from c109972 to ec42085Compare
rvaggforce-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare
This commit opts for a simpler way to batch writes to HTTP clients into fewer packets. Instead of the complicated snafu which was before, now OutgoingMessage#write automatically corks the socket and uncorks on the next tick, allowing streams to batch them efficiently. It also makes the code cleaner and removes an ugly-ish hack.
The first change in `_writeRaw`: This reduces drops an unnecessary if check (`outputLength`), because it is redone in `_flushOutput`. It also changes an if/else statement to an if statement, because the blocks were unrelated. The second change in `write`: This consolidates code in #write() that handled different string encodings and Buffers. There was no reason to handle the encodings differently, so after splitting them based on Buffer vs encoding, the code is consolidated. This might see a speedup. Shoutout to Ron Korving <[email protected]> for spotting this.
brendanashworthforce-pushed the http-fix-cork branch from ec42085 to 6acff8fComparePTAL @mcollina 😄 edit: CI: https://ci.nodejs.org/job/node-test-pull-request/5402/ |
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
LGTM if CI is green and the perf results are still confirmed.
I'm seeing regressions with strong significance when both chunks <= 1 and length <= 1024 for strings (mostly the 'bytes' type in the benchmark): |
ronkorving commented Mar 16, 2017
@brendanashworth Any chance you could look into those performance regressions? |
mscdex commented May 11, 2017
A rebase is needed. |
jasnell commented Aug 24, 2017
Ping. What do we want to do with this one? |
9 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
http
Description of change
This commit optimizes outgoing HTTP responses by corking the socket upon each write and then uncorking on the next tick. By doing this we get to remove a "shameful hack" that is years old while at the same time fixing #7914 and getting some performance improvements.
The second commit is just a cleanup commit that makes nothing do even less nothing.
benchmarks!
CI: https://ci.nodejs.org/job/node-test-pull-request/3498/