Skip to content

http: improve outgoing string write performance #13013

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 26, 2017
Merged

http: improve outgoing string write performance #13013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 65 additions & 78 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ utcDate._onTimeout = function _onTimeout() {
};


function noopPendingOutput(amount) {}
Copy link
Contributor

@thefourtheye thefourtheye May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



function OutgoingMessage() {
Stream.call(this);

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

this._onPendingData = null;
this._onPendingData = noopPendingOutput;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid creating a new noop for each response.

}
util.inherits(OutgoingMessage, Stream);

Expand Down Expand Up @@ -234,12 +237,18 @@ OutgoingMessage.prototype._send = function _send(data, encoding, callback) {
(encoding === 'utf8' || encoding === 'latin1' || !encoding)) {
data = this._header + data;
} else {
this.output.unshift(this._header);
this.outputEncodings.unshift('latin1');
this.outputCallbacks.unshift(null);
this.outputSize += this._header.length;
if (typeof this._onPendingData === 'function')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.outputCallbacks = [null];
} else {
this.output.unshift(header);
this.outputEncodings.unshift('latin1');
this.outputCallbacks.unshift(null);
}
this.outputSize += header.length;
this._onPendingData(header.length);
}
this._headerSent = true;
}
Expand Down Expand Up @@ -275,19 +284,13 @@ function _writeRaw(data, encoding, callback) {
return conn.write(data, encoding, callback);
}
// Buffer, as long as we're not destroyed.
return this._buffer(data, encoding, callback);
}


OutgoingMessage.prototype._buffer = function _buffer(data, encoding, callback) {
this.output.push(data);
this.outputEncodings.push(encoding);
this.outputCallbacks.push(callback);
this.outputSize += data.length;
if (typeof this._onPendingData === 'function')
this._onPendingData(data.length);
this._onPendingData(data.length);
return false;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



OutgoingMessage.prototype._storeHeader = _storeHeader;
Expand Down Expand Up @@ -624,27 +627,31 @@ Object.defineProperty(OutgoingMessage.prototype, 'headersSent', {

const crlf_buf = Buffer.from('\r\n');
OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
if (this.finished) {
return write_(this, chunk, encoding, callback, false);
};

function write_(msg, chunk, encoding, callback, fromEnd) {
if (msg.finished) {
var err = new Error('write after end');
nextTick(this.socket[async_id_symbol],
writeAfterEndNT.bind(this),
nextTick(msg.socket[async_id_symbol],
writeAfterEndNT.bind(msg),
err,
callback);

return true;
}

if (!this._header) {
this._implicitHeader();
if (!msg._header) {
msg._implicitHeader();
}

if (!this._hasBody) {
if (!msg._hasBody) {
debug('This type of response MUST NOT have a body. ' +
'Ignoring write() calls.');
return true;
}

if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
if (!fromEnd && typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
throw new TypeError('First argument must be a string or Buffer');
}

Expand All @@ -654,38 +661,28 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
if (chunk.length === 0) return true;

var len, ret;
if (this.chunkedEncoding) {
if (typeof chunk === 'string' &&
encoding !== 'hex' &&
encoding !== 'base64' &&
encoding !== 'latin1') {
if (msg.chunkedEncoding) {
if (typeof chunk === 'string')
len = Buffer.byteLength(chunk, encoding);
chunk = len.toString(16) + CRLF + chunk + CRLF;
ret = this._send(chunk, encoding, callback);
} else {
// buffer, or a non-toString-friendly encoding
if (typeof chunk === 'string')
len = Buffer.byteLength(chunk, encoding);
else
len = chunk.length;

if (this.connection && !this.connection.corked) {
this.connection.cork();
process.nextTick(connectionCorkNT, this.connection);
}
else
len = chunk.length;

this._send(len.toString(16), 'latin1', null);
this._send(crlf_buf, null, null);
this._send(chunk, encoding, null);
ret = this._send(crlf_buf, null, callback);
if (msg.connection && !msg.connection.corked) {
msg.connection.cork();
process.nextTick(connectionCorkNT, msg.connection);
}

msg._send(len.toString(16), 'latin1', null);
msg._send(crlf_buf, null, null);
msg._send(chunk, encoding, null);
ret = msg._send(crlf_buf, null, callback);
} else {
ret = this._send(chunk, encoding, callback);
ret = msg._send(chunk, encoding, callback);
}

debug('write ret = ' + ret);
return ret;
};
}


function writeAfterEndNT(err, callback) {
Expand Down Expand Up @@ -736,49 +733,40 @@ function onFinish(outmsg) {
outmsg.emit('finish');
}

OutgoingMessage.prototype.end = function end(data, encoding, callback) {
if (typeof data === 'function') {
callback = data;
data = null;
OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
if (typeof chunk === 'function') {
callback = chunk;
chunk = null;
} else if (typeof encoding === 'function') {
callback = encoding;
encoding = null;
}

if (data && typeof data !== 'string' && !(data instanceof Buffer)) {
throw new TypeError('First argument must be a string or Buffer');
}

if (this.finished) {
return false;
}

if (!this._header) {
if (data) {
if (typeof data === 'string')
this._contentLength = Buffer.byteLength(data, encoding);
var uncork;
if (chunk) {
if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
throw new TypeError('First argument must be a string or Buffer');
}
if (!this._header) {
if (typeof chunk === 'string')
this._contentLength = Buffer.byteLength(chunk, encoding);
else
this._contentLength = data.length;
} else {
this._contentLength = 0;
this._contentLength = chunk.length;
}
if (this.connection) {
this.connection.cork();
uncork = true;
}
write_(this, chunk, encoding, null, true);
} else if (!this._header) {
this._contentLength = 0;
this._implicitHeader();
}

if (data && !this._hasBody) {
debug('This type of response MUST NOT have a body. ' +
'Ignoring data passed to end().');
data = null;
}

if (this.connection && data)
this.connection.cork();

if (data) {
// Normal body write.
this.write(data, encoding);
}

if (typeof callback === 'function')
this.once('finish', callback);

Expand All @@ -792,7 +780,7 @@ OutgoingMessage.prototype.end = function end(data, encoding, callback) {
ret = this._send('', 'latin1', finish);
}

if (this.connection && data)
if (uncork)
this.connection.uncork();

this.finished = true;
Expand Down Expand Up @@ -871,8 +859,7 @@ OutgoingMessage.prototype._flushOutput = function _flushOutput(socket) {
this.output = [];
this.outputEncodings = [];
this.outputCallbacks = [];
if (typeof this._onPendingData === 'function')
this._onPendingData(-this.outputSize);
this._onPendingData(-this.outputSize);
this.outputSize = 0;

return ret;
Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-http-abort-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@
const common = require('../common');
const http = require('http');

let serverRes;
const server = http.Server(function(req, res) {
console.log('Server accepted request.');
serverRes = res;
res.writeHead(200);
res.write('Part of my res.');

res.destroy();
});

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

@refack refack May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


console.log(`Got res: ${res.statusCode}`);
console.dir(res.headers);
Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-http-client-aborted-event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
const common = require('../common');
const http = require('http');

let serverRes;
const server = http.Server(function(req, res) {
res.write('Part of my res.');
res.destroy();
serverRes = res;
});

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