Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make querystring parsing faster #2248

Closed
wants to merge 3 commits into from

6 participants

@bluesmoon

decodeURIComponent is faster than QueryString.unescape, but fails in some cases. This patch will use decodeURIComponent where appropriate and falls back to unescape on failure.

@tj
tj commented

is parsing query-strings really a bottleneck? (doubtful)

@3rd-Eden

How is adding a try catch making querystring parsing faster? In 0.6 it just makes V8's crankshaft optimizer bailout in that function.

@bluesmoon

Parsing querystrings is a bottleneck when that's the primary thing your app does (like my app). On my macbook pro, this pushes querystring parsing from 217 microseconds to 130 microseconds per url. I need to do about a billion urls a day.

@bluesmoon

Here's a crude benchmark, I parsed the following URL 100000 times and timed how long it took:
http://foo.com/?foo=918854443121279438895193&foo=bar&foo=bar&foo=quux&foo=1&bar=2&my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F&my%20weird%20field=q1!2%22\'w%245%267%2Fz8)%3F&foo%3Dbaz=bar&foo=baz=bar&foo=baz%3Dbar&str=foo&arr=1&arr=2&arr=3&somenull=&undef=& foo = bar &%20foo%20=%20bar%20&foo=%zx&foo=%25zx&foo=%EF%BF%BD&hasOwnProperty=x&toString=foo&valueOf=bar&__defineGetter__=baz

note that this is just a concatenation of all the querystrings used in the querystring unit test case.

timing information:

before patch:
Parsed 100000 qs in 29537 ms 3386 qs/s (295.37 us/qs)

after patch:
Parsed 100000 qs in 15804 ms 6328 qs/s (158.04 us/qs)

@KiNgMaR

Related: A patch to speed up QueryString.unescape in issue joyent#641

@isaacs
Owner

It looks like the try/catch isn't as expensive as it once was. This is about 40% faster over a variety of different query strings.

@bluesmoon Can you sign the CLA? http://nodejs.org/cla.html

@tj
tj commented

what about if/when v8 optimizes it? cant imagine it really optimizing anything in a short benchmark since it'll go straight to the "dumb" machine code

lib/querystring.js
@@ -170,9 +170,17 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq) {
}
qs.split(sep).forEach(function(kvp) {
- var x = kvp.split(eq);
- var k = QueryString.unescape(x[0], true);
- var v = QueryString.unescape(x.slice(1).join(eq), true);
+ var x = kvp.split(eq), k, v, useQS=false;
+ try {
+ if (kvp.match(/\+/)) {
+ throw "has +";
@tj
tj added a note

why wouldn't you use this conditional and drop the try/catch? also if you REALLY need to micro-optimize drop the forEach() too

@isaacs Owner
isaacs added a note
@isaacs Owner
isaacs added a note

@TooTallNate In this case, throwing a string is fine, and cheaper than
constructing an Error object. Note that he's catching it right away
anyhow. It's just a glamorized goto.

@TooTallNate Owner

Ah ok, missed that. Mind just automatically goes there when I see that :)

@isaacs right you are. If JavaScript had a goto statement that worked, I'd use that instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bluesmoon

Anyone looking at this?

@isaacs
Owner

@bluesmoon Yes, I'd love to merge this in, but you haven't signed the CLA. Can you do that, please? http://nodejs.org/cla.html

Thanks.

@bluesmoon

@isaacs I have now signed it.

@isaacs isaacs closed this pull request from a commit
@bluesmoon bluesmoon Make QueryString.parse run faster
Use decodeURIComponent when appropriate, and only fall back to
querystring.decode if it throws, or if the character is a '+'.

Fix #2248
5166758
@isaacs isaacs closed this in 5166758
@isaacs
Owner

Landed on 5166758. Will be in 0.6.10, and 0.7.3.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 3 deletions.
  1. +11 −3 lib/querystring.js
View
14 lib/querystring.js
@@ -170,9 +170,17 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq) {
}
qs.split(sep).forEach(function(kvp) {
- var x = kvp.split(eq);
- var k = QueryString.unescape(x[0], true);
- var v = QueryString.unescape(x.slice(1).join(eq), true);
+ var x = kvp.split(eq), k, v, useQS=false;
+ try {
+ if (kvp.match(/\+/)) { // decodeURIComponent does not decode + to space
+ throw "has +";
+ }
+ k = decodeURIComponent(x[0]);
+ v = decodeURIComponent(x.slice(1).join(eq) || "");
+ } catch(e) {
+ k = QueryString.unescape(x[0], true);
+ v = QueryString.unescape(x.slice(1).join(eq), true);
+ }
if (!hasOwnProperty(obj, k)) {
obj[k] = v;
Something went wrong with that request. Please try again.