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

Jump to bottom

querystring: improve parse() performance #4675

Closed
wants to merge 1 commit into from
Closed

querystring: improve parse() performance #4675

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
5 changes: 3 additions & 2 deletions benchmark/querystring/querystring-parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ var querystring = require('querystring');
var v8 = require('v8');

var bench = common.createBenchmark(main, {
type: ['noencode', 'encodemany', 'encodelast'],
type: ['noencode', 'encodemany', 'encodelast', 'multivalue'],
n: [1e6],
});

Expand All @@ -14,7 +14,8 @@ function main(conf) {
var inputs = {
noencode: 'foo=bar&baz=quux&xyzzy=thud',
encodemany: '%66%6F%6F=bar&%62%61%7A=quux&xyzzy=%74h%75d',
encodelast: 'foo=bar&baz=quux&xyzzy=thu%64'
encodelast: 'foo=bar&baz=quux&xyzzy=thu%64',
multivalue: 'foo=bar&foo=baz&foo=quux&quuy=quuz'
};
var input = inputs[type];

Expand Down
30 changes: 27 additions & 3 deletions lib/querystring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) {
return obj;
}

var regexp = /\+/g;
qs = qs.split(sep);

var maxKeys = 1000;
Expand All @@ -230,7 +229,9 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) {

var keys = [];
for (var i = 0; i < len; ++i) {
const x = qs[i].replace(regexp, '%20');
// replacePlus() is used instead of a regexp because it is ~15-30% faster
// with v8 4.7
const x = replacePlus(qs[i]);
const idx = x.indexOf(eq);
var k, v;

Expand All @@ -242,10 +243,14 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) {
v = '';
}

// Use a key array lookup instead of using hasOwnProperty(), which is slower
if (keys.indexOf(k) === -1) {
obj[k] = v;
keys.push(k);
} else if (Array.isArray(obj[k])) {
} else if (obj[k] instanceof Array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is instanceof reliable for arrays?

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 should be and we're the only one creating the values in this function, so it's safe.

// `instanceof Array` is used instead of Array.isArray() because it is
// ~15-20% faster with v8 4.7 and is safe to use because we are using it
// with values being created within this function
obj[k].push(v);
} else {
obj[k] = [obj[k], v];
Expand All @@ -256,6 +261,25 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) {
};


function replacePlus(str) {
var ret = '';
var start = 0;
var i = -1;
while ((i = str.indexOf('+', i + 1)) !== -1) {
ret += str.slice(start, i);
ret += '%20';
start = i + 1;
}
if (start === 0)
return str;
if (start < str.length)
ret += str.slice(start);
return ret;
}


// v8 does not optimize functions with try-catch blocks, so we isolate them here
// to minimize the damage
function decodeStr(s, decoder) {
try {
return decoder(s);
Expand Down