Make querystring parsing faster #2248

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants

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 commented Dec 2, 2011

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

3rd-Eden commented Dec 2, 2011

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

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.

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 commented Dec 2, 2011

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

isaacs commented Dec 2, 2011

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 commented Dec 2, 2011

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

@tj tj and 3 others commented on an outdated diff Dec 2, 2011

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 Dec 2, 2011

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

@bluesmoon

bluesmoon Dec 3, 2011

you need the try/catch because decodeURIComponent will throw on some
failures that the fallback can handle

@isaacs

isaacs Feb 1, 2012

@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.

On Tue, Jan 31, 2012 at 17:59, Nathan Rajlich
reply@reply.github.com
wrote:

@@ -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 +";

Please use a real Error object: http://www.devthought.com/2011/12/22/a-string-is-not-an-error/


Reply to this email directly or view it on GitHub:
https://github.com/joyent/node/pull/2248/files#r404931

@isaacs

isaacs Feb 1, 2012

@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

TooTallNate Feb 1, 2012

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

@bluesmoon

bluesmoon Feb 1, 2012

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

Anyone looking at this?

isaacs commented Feb 1, 2012

@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.

@isaacs I have now signed it.

isaacs closed this in 5166758 Feb 2, 2012

isaacs commented Feb 2, 2012

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