Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
http: optimize checkInvalidHeaderChar()
This commit optimizes checkInvalidHeaderChar() by unrolling the
character checking loop a bit.

Additionally, some changes to the benchmark runner are needed in
order for the included benchmark to be run correctly. Specifically,
the regexp used to parse `key=value` parameters contained a greedy
quantifier that was causing the `key` to match part of the `value`
if `value` contained an equals sign.

PR-URL: #6570
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
  • Loading branch information
mscdex committed Jun 14, 2016
commit 83432bfff1e21797e497aacf4c473db1345f6334
8 changes: 3 additions & 5 deletions benchmark/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ function parseOpts(options) {
var num = keys.length;
var conf = {};
for (var i = 2; i < process.argv.length; i++) {
var match = process.argv[i].match(/^(.+)=(.*)$/);
var match = process.argv[i].match(/^(.+?)=([\s\S]*)$/);
if (!match || !match[1] || !options[match[1]]) {
return null;
} else {
Expand Down Expand Up @@ -238,20 +238,18 @@ Benchmark.prototype.report = function(value) {
console.log('%s: %s', heading, value.toFixed(5));
else if (outputFormat == 'csv')
console.log('%s,%s', heading, value.toFixed(5));

process.exit(0);
};

Benchmark.prototype.getHeading = function() {
var conf = this.config;

if (outputFormat == 'default') {
return this._name + ' ' + Object.keys(conf).map(function(key) {
return key + '=' + conf[key];
return key + '=' + JSON.stringify('' + conf[key]);
}).join(' ');
} else if (outputFormat == 'csv') {
return this._name + ',' + Object.keys(conf).map(function(key) {
return conf[key];
return JSON.stringify('' + conf[key]);
}).join(',');
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Either the changes to this file should be a separate commit or the commit log should explain why they are necessary.

Copy link
Contributor Author

@mscdex mscdex May 8, 2016

Choose a reason for hiding this comment

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

Initially I had planned it to be in a separate commit but there would then technically be a gap where the new benchmark would be broken (causing the benchmark runner to get in a large loop actually), especially if the benchmark would be cherry picked. I can amend the commit message...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, commit message updated.

Expand Down
42 changes: 42 additions & 0 deletions benchmark/http/check_invalid_header_char.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';

const common = require('../common.js');
const _checkInvalidHeaderChar = require('_http_common')._checkInvalidHeaderChar;

const bench = common.createBenchmark(main, {
key: [
// Valid
'',
'1',
'\t\t\t\t\t\t\t\t\t\tFoo bar baz',
'keep-alive',
'close',
'gzip',
'20091',
'private',
'text/html; charset=utf-8',
'text/plain',
'Sat, 07 May 2016 16:54:48 GMT',
'SAMEORIGIN',
'en-US',

// Invalid
'Here is a value that is really a folded header value\r\n this should be \
supported, but it is not currently',
'中文呢', // unicode
'foo\nbar',
'\x7F'
],
n: [5e8],
});

function main(conf) {
var n = +conf.n;
var key = conf.key;

bench.start();
for (var i = 0; i < n; i++) {
_checkInvalidHeaderChar(key);
}
bench.end(n);
}
29 changes: 24 additions & 5 deletions lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,32 @@ exports._checkIsHttpToken = checkIsHttpToken;
* field-value = *( field-content / obs-fold )
* field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
* field-vchar = VCHAR / obs-text
*
* checkInvalidHeaderChar() is currently designed to be inlinable by v8,
* so take care when making changes to the implementation so that the source
* code size does not exceed v8's default max_inlined_source_size setting.
**/
function checkInvalidHeaderChar(val) {
val = '' + val;
for (var i = 0; i < val.length; i++) {
const ch = val.charCodeAt(i);
if (ch === 9) continue;
if (ch <= 31 || ch > 255 || ch === 127) return true;
val += '';
if (val.length < 1)
return false;
var c = val.charCodeAt(0);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
return true;
if (val.length < 2)
return false;
c = val.charCodeAt(1);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
return true;
if (val.length < 3)
return false;
c = val.charCodeAt(2);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
return true;
for (var i = 3; i < val.length; ++i) {
c = val.charCodeAt(i);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
return true;
}
return false;
}
Expand Down