Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: improve outgoing string write performance #13013

Merged
merged 1 commit into from May 26, 2017

Conversation

Projects
None yet
9 participants
@mscdex
Copy link
Contributor

commented May 13, 2017

These changes could be semver-major-y. One behavioral change introduced by this commit is that when writing a string that is the first chunk, it is no longer guaranteed to be sent immediately with the header. This is why the two tests had to be modified. It should be noted that this behavioral change actually brings it in line with what happens when writing a first Buffer chunk, so there would be consistency after this change. However I'm not sure if there is anyone that may be relying on the old string writing behavior...

One other possibly noticeable change is that chunk/data in end() is no longer validated if the outgoing message is already finished.

Anyway, here are some results:

                                                                                       improvement confidence      p.value
 http/simple.js res="normal" c=50 chunks=0 len=1024 type="buffer" benchmarker="wrk"       -1.46 %            3.762163e-01
 http/simple.js res="normal" c=50 chunks=0 len=1024 type="bytes" benchmarker="wrk"        -0.51 %            7.115831e-01
 http/simple.js res="normal" c=50 chunks=0 len=102400 type="buffer" benchmarker="wrk"     -1.34 %            4.010301e-01
 http/simple.js res="normal" c=50 chunks=0 len=102400 type="bytes" benchmarker="wrk"       0.59 %            1.274991e-01
 http/simple.js res="normal" c=50 chunks=0 len=4 type="buffer" benchmarker="wrk"          -0.40 %            8.526384e-01
 http/simple.js res="normal" c=50 chunks=0 len=4 type="bytes" benchmarker="wrk"            0.37 %            8.122313e-01
 http/simple.js res="normal" c=50 chunks=1 len=1024 type="buffer" benchmarker="wrk"       -0.94 %            3.148300e-01
 http/simple.js res="normal" c=50 chunks=1 len=1024 type="bytes" benchmarker="wrk"        -0.05 %            9.575339e-01
 http/simple.js res="normal" c=50 chunks=1 len=102400 type="buffer" benchmarker="wrk"      0.58 %            8.019575e-01
 http/simple.js res="normal" c=50 chunks=1 len=102400 type="bytes" benchmarker="wrk"     319.43 %        *** 1.099052e-27
 http/simple.js res="normal" c=50 chunks=1 len=4 type="buffer" benchmarker="wrk"           0.23 %            8.886316e-01
 http/simple.js res="normal" c=50 chunks=1 len=4 type="bytes" benchmarker="wrk"           -3.08 %            1.276294e-01
 http/simple.js res="normal" c=50 chunks=4 len=1024 type="buffer" benchmarker="wrk"        2.05 %            1.916095e-01
 http/simple.js res="normal" c=50 chunks=4 len=1024 type="bytes" benchmarker="wrk"      1376.72 %        *** 5.568434e-35
 http/simple.js res="normal" c=50 chunks=4 len=102400 type="buffer" benchmarker="wrk"     -1.57 %            5.306198e-01
 http/simple.js res="normal" c=50 chunks=4 len=102400 type="bytes" benchmarker="wrk"       3.76 %            5.205095e-02
 http/simple.js res="normal" c=50 chunks=4 len=4 type="buffer" benchmarker="wrk"          -2.78 %            1.683912e-01
 http/simple.js res="normal" c=50 chunks=4 len=4 type="bytes" benchmarker="wrk"         1409.81 %        *** 3.412588e-30

CI: https://ci.nodejs.org/job/node-test-pull-request/8060/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/789/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • http

@mscdex mscdex added the performance label May 13, 2017

@mscdex mscdex force-pushed the mscdex:http-outgoing-string-perf branch May 16, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@ronkorving

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

I would mark this semver-major. I can imagine code out there on the receiving-end may depend on packets containing that first chunk. Not a good thing, but probably best to assume that exists.

@@ -78,6 +78,9 @@ utcDate._onTimeout = function _onTimeout() {
};


function noopPendingOutput(amount) {}

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 16, 2017

Contributor

Mmmm, I thought our linter would complain about amount. It didn't.

@AndreasMadsen
Copy link
Member

left a comment

The AsyncHooks changes doesn't appear to have anything to do with this optimization, is there another PR or can you comment on why these changes were made?

edit: I see #13045 now. I'm not really comfortable with the socket._handle.asyncReset change until we understand why.

lib/_http_agent.js Outdated
@@ -167,7 +167,8 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
// we have a free socket, so use that.
var socket = this.freeSockets[name].shift();
// Assign the handle a new asyncId and run any init() hooks.
socket._handle.asyncReset();
if (socket._handle.asyncReset)

This comment has been minimized.

Copy link
@AndreasMadsen

AndreasMadsen May 16, 2017

Member

Why is this necessary?

This comment has been minimized.

Copy link
@refack

refack May 18, 2017

Member

Might not be necessary. At Least for the TLS case.
Ref #13092

lib/_http_agent.js Outdated
@@ -182,7 +183,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
// If we are under maxSockets create a new one.
this.createSocket(req, options, function(err, newSocket) {
if (err) {
nextTick(newSocket._handle.getAsyncId(), function() {
process.nextTick(function() {

This comment has been minimized.

Copy link
@AndreasMadsen

AndreasMadsen May 16, 2017

Member

Is this because the newSocket._handle isn't set when an error occurs?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@AndreasMadsen Pay no attention to the async hooks workaround commit as it's temporary. I added it so that citgm and the like will work for this PR.

@@ -117,7 +120,7 @@ function OutgoingMessage() {
this._header = null;
this[outHeadersKey] = null;

this._onPendingData = null;
this._onPendingData = noopPendingOutput;

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

Since you don't need it's name to unregister why not just () => {}?

This comment has been minimized.

Copy link
@mscdex

mscdex May 16, 2017

Author Contributor

To avoid creating a new noop for each response.

this.outputEncodings.unshift('latin1');
this.outputCallbacks.unshift(null);
this.outputSize += this._header.length;
if (typeof this._onPendingData === 'function')

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

I like the "polymorphic" approach 👍
Question is the noop call faster than the if (typeof

This comment has been minimized.

Copy link
@mscdex

mscdex May 16, 2017

Author Contributor

I didn't measure it by itself, but it should be to some degree since it no longer has to execute a conditional before calling the function.

return false;
};
}

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

Isn't this the end of OutgoingMessage.prototype._send = function _send, so should have ;

This comment has been minimized.

Copy link
@mscdex

mscdex May 16, 2017

Author Contributor

No, this is the end of _writeRaw() which uses a function declaration, so there is no semicolon needed.

@@ -37,6 +37,7 @@ server.listen(0, common.mustCall(function() {
headers: { connection: 'keep-alive' }
}, common.mustCall(function(res) {
server.close();
serverRes.destroy();

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

What is the benefit of moving it here?
Found the comment.

@refack
Copy link
Member

left a comment

Some comments and quastions

@refack

This comment has been minimized.

Copy link
Member

commented May 16, 2017

I'm +1, since except for the performance, IMHO it makes the code more readable.

this behavioral change actually brings it in line with what happens when writing a first Buffer chunk

and more consistent.

@mscdex mscdex force-pushed the mscdex:http-outgoing-string-perf branch May 20, 2017

@mscdex mscdex force-pushed the mscdex:http-outgoing-string-perf branch May 20, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2017

Thoughts on semver-ness @nodejs/ctc ?

@jasnell

This comment has been minimized.

Copy link
Member

commented May 23, 2017

hmm... really hard to say. Apps really shouldn't be depending on the data coming with the header. I think I'm ok with this as a patch.

@mcollina

This comment has been minimized.

Copy link
Member

commented May 23, 2017

Given some recent digging I did, I would be cautious. I would say semver-patch, but don't backport to LTS for several months (if at all).

var header = this._header;
if (this.output.length === 0) {
this.output = [header];
this.outputEncodings = ['latin1'];

This comment has been minimized.

Copy link
@trevnorris

trevnorris May 23, 2017

Contributor

General note on how we handle the response body. Browsers treat 'charset=latin1' as windows-1252, which is problematic. Example:

const headers =
  'HTTP/1.1 200 OK\r\n' +
  'Content-Type: text/html; charset=latin1\r\n' +
  'Content-Length: 2\r\n\r\n' +
  // Technically 0x83 is a control code, but browsers don't see it that way.
  '\u0083\n';
const data = Buffer.from(headers, 'latin1');

require('net').createServer(c => c.end(data)).listen(8778);

Hit that with the browser and you'll see ƒ, not a control character symbol.

@jasnell

This comment has been minimized.

Copy link
Member

commented May 23, 2017

semver-patch with no backporting works for me.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2017

semver-patch with no backporting works for me.

@jasnell Isn't that effectively semver-major then, or were you thinking about landing it in v8.0.0?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2017

Any reviews/approvals for this PR @nodejs/collaborators ?

@jasnell

This comment has been minimized.

Copy link
Member

commented May 25, 2017

@mscdex ... not necessarily because after we give it a few months we may decide that it's safe to go ahead and pull back.

@mscdex mscdex force-pushed the mscdex:http-outgoing-string-perf branch May 26, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

http: improve outgoing string write performance
PR-URL: #13013
Reviewed-By: James M Snell <jasnell@gmail.com>

@mscdex mscdex force-pushed the mscdex:http-outgoing-string-perf branch to a10bdb5 May 26, 2017

@mscdex mscdex merged commit a10bdb5 into nodejs:master May 26, 2017

@mscdex mscdex deleted the mscdex:http-outgoing-string-perf branch May 26, 2017

jasnell added a commit that referenced this pull request May 28, 2017

http: improve outgoing string write performance
PR-URL: #13013
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.