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

url: spec-compliant URLSearchParams parser #11858

Closed
wants to merge 5 commits into from
Closed

url: spec-compliant URLSearchParams parser #11858

wants to merge 5 commits into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Mar 15, 2017 •
edited
Loading 

// before
> new URLSearchParams('a=1&b=2&a=3').toString()
'a=1&a=3&b=2'
// after
> new URLSearchParams('a=1&b=2&a=3').toString()
'a=1&b=2&a=3'

Performance is also drastically improved, so that URLSearchParams is on average 150% faster than querystring.parse.

The URLSearchParams parser is in fact forked from the querystring.parse, with the following changes:

  1. Output a flat array instead of an object
  2. Formerly separate key and value variables are now merged into one buf
  3. Support for multi-char sep and eq is removed
  4. lastPos was formerly used for two different purposes: as the starting position of a pair and as the (index + 1) of the last-added buffer; split it into two variables
  5. Logic for leftover cases is clarified

Some of these changes may be able to be backported to querystring. In particular, 4 and 5 may fix actual bugs in the querystring implementation (e.g. currently querystring.parse('+') => {}). I will investigate that after this PR is landed.

After this PR, the URLSearchParams class will be fully spec-compliant 🎉

Fixes: #10821

Benchmark: before vs. after (avg. 240% improvement) 
                                                                                                improvement confidence      p.value
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="altspaces"         147.66 %        *** 6.450352e-25
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodefake"        214.97 %        *** 8.596431e-25
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodelast"        160.81 %        *** 7.846159e-26
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodemany"        129.28 %        *** 6.855510e-17
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="manyblankpairs"    111.20 %        *** 2.251639e-30
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="manypairs"         759.68 %        *** 1.392651e-25
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multicharsep"      202.40 %        *** 1.409313e-30
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multivalue"        237.10 %        *** 3.232098e-35
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multivaluemany"    182.93 %        *** 9.763223e-36
 url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="noencode"          232.86 %        *** 1.428285e-28
Benchmark: legacy (querystring) vs. WHATWG (avg. 150% improvement) 
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="noencode": 808,271.1185364788
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="noencode": 1,938,217.2737648692
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="multicharsep": 693,339.6072987281
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multicharsep": 1,609,957.6058235439
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="encodefake": 810,276.9526137867
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodefake": 1,794,510.1863450601
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="encodemany": 422,024.59562550695
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodemany": 812,861.0901187251
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="encodelast": 700,483.2831005914
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="encodelast": 1,360,862.1965627724
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="multivalue": 631,531.1961645706
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multivalue": 1,585,023.9933641006
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="multivaluemany": 259,648.7999004816
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="multivaluemany": 584,279.313482464
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="manypairs": 117,903.20162874568
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="manypairs": 769,175.7897286669
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="manyblankpairs": 4,665,651.983783668
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="manyblankpairs": 4,862,063.9270777535
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="legacy" type="altspaces": 605,017.1751065635
url/legacy-vs-whatwg-url-searchparams-parse.js n=1000000 method="whatwg" type="altspaces": 1,152,026.3496627773
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

querystring, url

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x querystring Issues and PRs related to the built-in querystring module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 15, 2017
@TimothyGu
Copy link
Member Author

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

@TimothyGu
Copy link
Member Author

Rerun of CI after last commit: https://ci.nodejs.org/job/node-test-pull-request/6855/

@TimothyGu
Copy link
Member Author

@mscdex, I'd like you to take a look at this if possible since you wrote the optimized querystring.parse().

@TimothyGu TimothyGu requested a review from mscdex March 16, 2017 05:09
@TimothyGu TimothyGu removed the request for review from mscdex March 21, 2017 01:01
@TimothyGu
Copy link
Member Author

Unless there are any additional comments (ping @mscdex) I'm going to merge this tomorrow.

TimothyGu added a commit that referenced this pull request Mar 22, 2017
@TimothyGu
querystring: move isHexTable to internal
c98a802 
PR-URL: #11858
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
TimothyGu added a commit that referenced this pull request Mar 22, 2017
@TimothyGu
url: spec-compliant URLSearchParams parser
c515a98 
The entire `URLSearchParams` class is now fully spec-compliant.

PR-URL: #11858
Fixes: #10821
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@TimothyGu
Copy link
Member Author

Landed in a1028d5...c515a98.

@TimothyGu TimothyGu closed this Mar 22, 2017
@TimothyGu TimothyGu deleted the urlsearchparams-fork-querystring branch March 22, 2017 00:35
@MylesBorins
Copy link
Contributor

this will need to be manually backported to v7.x

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@TimothyGu TimothyGu mentioned this pull request Apr 19, 2017
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@watilde watilde watilde approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
querystring Issues and PRs related to the built-in querystring module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

url: track status of a spec-compliant URLSearchParams
6 participants
@TimothyGu @MylesBorins @jasnell @watilde @joyeecheung @nodejs-github-bot