querystring: avoid indexOf when parsing

[mcollina]

Fixes a performance regression in body-parser with V8 6.0.
Removes the use of an auxiliary array, and just query the object
directly.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

querystring

[MylesBorins]

ci: https://ci.nodejs.org/job/node-test-pull-request/9557/

edit;

master-citgm body-parser: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/943/
citgm body-parser for this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/945/

[evanlucas]

LGTM. The body-parser test suite is significantly faster with this change

[mcollina]

We should probably add a benchmark/unit test to check that we do not regress on this.

Here is what I have used: https://gist.github.com/mcollina/f76e8ad685db1686a49aceee66daf075.

Feel free to push on this branch and land, I will not have time in the next few days.

[mscdex]

In general it looks fine to me since Object.create(null) is fast enough now and using that would mean we wouldn't have to check for an 'own property' anymore, but we definitely should have some good benchmark(s) for this that are ran before landing just in case.

[TimothyGu]

@mcollina Can you post the benchmark results, just for good measure?

[mcollina]

I'm on vacation until next Wednesday. I guess we might want to land this sooner, could someone else take the lead and run the benchmarks?

[TimothyGu]

To clarify I'm more than fine with this being landed before performance results are available, just wondering :)

[MylesBorins]

The body parser suite completely fails on all platforms without this change.

I would like to see this landed before we have benchmarks, it is blocking the release of 8.3.0

[mcollina]

I am +1 in landing as is and getting 8.3.0 out.

[evanlucas]

+1 to landing this now and getting 8.3.0 out as well. I ran the benchmarks yesterday and the results were pretty varied for all but the manypairs filter in querystring-parse. It was ~25% faster with this change. The others fluctuated and I haven't had a chance to run without anything else running locally

[MylesBorins]

If I don't see any complaints I will land this on master in exactly 2 hours

[addaleax]

With n set to 1e5 instead of 1e6, to get benchmark results in a reasonable time:

$ ./node benchmark/compare.js --new ./node --old ./node-master --runs 20 --filter parse querystring | Rscript benchmark/compare.R
[00:05:25|% 100| 1/1 files | 40/40 runs | 10/10 configs]: Done
                                                                  improvement confidence      p.value
 querystring/querystring-parse.js n=100000 type="altspaces"          -6.37 %        *** 1.480084e-11
 querystring/querystring-parse.js n=100000 type="encodefake"         -5.67 %        *** 6.231106e-07
 querystring/querystring-parse.js n=100000 type="encodelast"         -7.49 %        *** 1.104349e-08
 querystring/querystring-parse.js n=100000 type="encodemany"         -3.09 %         ** 3.595906e-03
 querystring/querystring-parse.js n=100000 type="manyblankpairs"      2.28 %        *** 1.031729e-04
 querystring/querystring-parse.js n=100000 type="manypairs"          19.22 %        *** 7.423575e-28
 querystring/querystring-parse.js n=100000 type="multicharsep"       -8.20 %        *** 2.022304e-10
 querystring/querystring-parse.js n=100000 type="multivalue"          0.86 %            4.507649e-01
 querystring/querystring-parse.js n=100000 type="multivaluemany"     15.06 %        *** 1.043772e-12
 querystring/querystring-parse.js n=100000 type="noencode"           -6.47 %        *** 1.616370e-08

[MylesBorins]

@addaleax do you think it is ok to land?

[addaleax]

@MylesBorins Yes, I think so. If you do that now, I can take care of updating the v8.3.0 proposal.

[MylesBorins]

landed in 7ec28a0