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.

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

http: various performance improvements #10558

Merged
merged 9 commits into from
Jan 11, 2017
Merged

http: various performance improvements #10558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f53a6fb
benchmark: add http header setting scenarios
mscdex Dec 31, 2016
ec8910b
http: check statusCode early
mscdex Dec 31, 2016
c00e4ad
http: optimize header storage and matching
mscdex Dec 31, 2016
c77ed32
http: avoid using object for removed header status
mscdex Dec 31, 2016
73d9445
http: try to avoid lowercasing incoming headers
mscdex Dec 31, 2016
176cdc2
http: misc optimizations and style fixes
mscdex Dec 31, 2016
b888bfe
benchmark: allow zero when parsing http req/s
mscdex Dec 31, 2016
a3539ae
stream: use plain objects for write/corked reqs
mscdex Dec 31, 2016
03b9f6f
stream: avoid instanceof
mscdex Dec 31, 2016
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
4 changes: 2 additions & 2 deletions benchmark/_http-benchmarkers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ WrkBenchmarker.prototype.create = function(options) {
WrkBenchmarker.prototype.processResults = function(output) {
const match = output.match(this.regexp);
const result = match && +match[1];
if (!result) {
if (!isFinite(result)) {
return undefined;
} else {
return result;
Expand Down Expand Up @@ -126,7 +126,7 @@ exports.run = function(options, callback) {
}

const result = benchmarker.processResults(stdout);
if (!result) {
if (result === undefined) {
callback(new Error(`${options.benchmarker} produced strange output: ` +
stdout, code));
return;
Expand Down
88 changes: 58 additions & 30 deletions benchmark/http/_http_simple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ var http = require('http');

var port = parseInt(process.env.PORT || 8000);

var fixed = 'C'.repeat(20 * 1024),
storedBytes = {},
storedBuffer = {},
storedUnicode = {};
var fixed = 'C'.repeat(20 * 1024);
var storedBytes = Object.create(null);
var storedBuffer = Object.create(null);
var storedUnicode = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be Maps instead? Also, let?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK Map is still slow and http benchmarks already take a very long time as it is.

As far as converting to ES6 goes, that's for a separate PR I think. All I was doing in this part here was the changing style.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK Map is still slow and http benchmarks already take a very long time as it is.

That is why we have https://github.com/nodejs/node/blob/master/benchmark/es/map-bench.js - tl;dr, it's faster.

Copy link
Member

Choose a reason for hiding this comment

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

Map performance definitely appears to have improved significantly as of late.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but I seem to recall some issue with that benchmark. I think there was discussion in some issue/PR awhile ago about it?


var useDomains = process.env.NODE_USE_DOMAINS;

Expand All @@ -29,11 +29,13 @@ var server = module.exports = http.createServer(function(req, res) {
dom.add(res);
}

var commands = req.url.split('/');
var command = commands[1];
// URL format: /<type>/<length>/<chunks>/<responseBehavior>
var params = req.url.split('/');
var command = params[1];
var body = '';
var arg = commands[2];
var n_chunks = parseInt(commands[3], 10);
var arg = params[2];
var n_chunks = parseInt(params[3], 10);
var resHow = (params.length >= 5 ? params[4] : 'normal');
var status = 200;

var n, i;
Expand All @@ -45,7 +47,6 @@ var server = module.exports = http.createServer(function(req, res) {
storedBytes[n] = 'C'.repeat(n);
}
body = storedBytes[n];

} else if (command === 'buffer') {
n = ~~arg;
if (n <= 0)
Expand All @@ -57,7 +58,6 @@ var server = module.exports = http.createServer(function(req, res) {
}
}
body = storedBuffer[n];

} else if (command === 'unicode') {
n = ~~arg;
if (n <= 0)
Expand All @@ -66,23 +66,30 @@ var server = module.exports = http.createServer(function(req, res) {
storedUnicode[n] = '\u263A'.repeat(n);
}
body = storedUnicode[n];

} else if (command === 'quit') {
res.connection.server.close();
body = 'quitting';

} else if (command === 'fixed') {
body = fixed;

} else if (command === 'echo') {
const headers = {
'Content-Type': 'text/plain',
'Transfer-Encoding': 'chunked'
};
res.writeHead(200, headers);
switch (resHow) {
case 'setHeader':
res.statusCode = 200;
res.setHeader('Content-Type', 'text/plain');
res.setHeader('Transfer-Encoding', 'chunked');
break;
case 'setHeaderWH':
res.setHeader('Content-Type', 'text/plain');
res.writeHead(200, { 'Transfer-Encoding': 'chunked' });
break;
default:
res.writeHead(200, {
'Content-Type': 'text/plain',
'Transfer-Encoding': 'chunked'
});
}
req.pipe(res);
return;

} else {
status = 404;
body = 'not found\n';
Expand All @@ -91,11 +98,22 @@ var server = module.exports = http.createServer(function(req, res) {
// example: http://localhost:port/bytes/512/4
// sends a 512 byte body in 4 chunks of 128 bytes
if (n_chunks > 0) {
const headers = {
'Content-Type': 'text/plain',
'Transfer-Encoding': 'chunked'
};
res.writeHead(status, headers);
switch (resHow) {
case 'setHeader':
res.statusCode = status;
res.setHeader('Content-Type', 'text/plain');
res.setHeader('Transfer-Encoding', 'chunked');
break;
case 'setHeaderWH':
res.setHeader('Content-Type', 'text/plain');
res.writeHead(status, { 'Transfer-Encoding': 'chunked' });
break;
default:
res.writeHead(status, {
'Content-Type': 'text/plain',
'Transfer-Encoding': 'chunked'
});
}
// send body in chunks
var len = body.length;
var step = Math.floor(len / n_chunks) || 1;
Expand All @@ -105,12 +123,22 @@ var server = module.exports = http.createServer(function(req, res) {
}
res.end(body.slice((n_chunks - 1) * step));
} else {
const headers = {
'Content-Type': 'text/plain',
'Content-Length': body.length.toString()
};

res.writeHead(status, headers);
switch (resHow) {
case 'setHeader':
res.statusCode = status;
res.setHeader('Content-Type', 'text/plain');
res.setHeader('Content-Length', body.length.toString());
break;
case 'setHeaderWH':
res.setHeader('Content-Type', 'text/plain');
res.writeHead(status, { 'Content-Length': body.length.toString() });
break;
default:
res.writeHead(status, {
'Content-Type': 'text/plain',
'Content-Length': body.length.toString()
});
}
res.end(body);
}
});
Expand Down
6 changes: 4 additions & 2 deletions benchmark/http/simple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@ var bench = common.createBenchmark(main, {
type: ['bytes', 'buffer'],
length: [4, 1024, 102400],
chunks: [0, 1, 4], // chunks=0 means 'no chunked encoding'.
c: [50, 500]
c: [50, 500],
res: ['normal', 'setHeader', 'setHeaderWH']
});

function main(conf) {
process.env.PORT = PORT;
var server = require('./_http_simple.js');
setTimeout(function() {
var path = '/' + conf.type + '/' + conf.length + '/' + conf.chunks;
var path = '/' + conf.type + '/' + conf.length + '/' + conf.chunks + '/' +
conf.res;

bench.http({
path: path,
Expand Down
11 changes: 9 additions & 2 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,12 @@ function ClientRequest(options, cb) {
self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
options.headers);
} else if (self.getHeader('expect')) {
if (self._header) {
throw new Error('Can\'t render headers after they are sent to the ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

This additional throw probably makes this PR semver-major?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't, I just had to move the check from the old _renderHeaders() to the appropriate call sites since that function no longer exists. I tried just moving it to _storeHeader() but that caused problems IIRC.

'client');
}
self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
self._renderHeaders());
self._headers);
}

this._ended = false;
Expand Down Expand Up @@ -224,8 +228,11 @@ ClientRequest.prototype._finish = function _finish() {
};

ClientRequest.prototype._implicitHeader = function _implicitHeader() {
if (this._header) {
throw new Error('Can\'t render headers after they are sent to the client');
Copy link
Contributor

Choose a reason for hiding this comment

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

As does this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't, I just had to move the check from the old _renderHeaders() to the appropriate call sites since that function no longer exists. I tried just moving it to _storeHeader() but that caused problems IIRC.

}
this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
this._renderHeaders());
this._headers);
};

ClientRequest.prototype.abort = function abort() {
Expand Down
4 changes: 2 additions & 2 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ const debug = require('util').debuglog('http');
exports.debug = debug;

exports.CRLF = '\r\n';
exports.chunkExpression = /chunk/i;
exports.continueExpression = /100-continue/i;
exports.chunkExpression = /(?:^|\W)chunked(?:$|\W)/i;
exports.continueExpression = /(?:^|\W)100-continue(?:$|\W)/i;
Copy link
Member

Choose a reason for hiding this comment

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

why those changes? are they faster, or safer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more explicit and therefore at least a little safer and it's consistent with the other regexps we use in _http_server.js when searching header values that can be comma-separated lists.

I did not measure the performance of this particular change on its own.

exports.methods = methods;

const kOnHeaders = HTTPParser.kOnHeaders | 0;
Expand Down
Loading