url: spec-compliant URLSearchParams serializer by TimothyGu · Pull Request #11626 · nodejs/node

TimothyGu · 2017-03-01T04:19:37Z

Differences with querystring module (which the serializer currently depends on):

code point	`querystring`	spec
U+0020	`%20`	`+`
U+0027 `'`	`'`	`%27`
U+0028 `(`	`(`	`%28`
U+0029 `)`	`)`	`%29`
U+007E `~`	`~`	`%7E`

A C++ implementation cannot seem to reach the JS version's performance.

Benchmark: before vs. after

                                                                                                    improvement confidence      p.value
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="altspaces"          56.97 %        *** 1.131841e-33
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodefake"         85.79 %        *** 8.380179e-49
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodelast"        133.54 %        *** 4.568047e-52
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodemany"        136.69 %        *** 3.839133e-66
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="manyblankpairs"    102.90 %        *** 2.115923e-35
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="manypairs"         423.66 %        *** 2.121638e-43
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multicharsep"      136.13 %        *** 1.320596e-54
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multivalue"         88.71 %        *** 1.524783e-59
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multivaluemany"     25.24 %        *** 3.994424e-41
 url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="noencode"          135.61 %        *** 3.251158e-44

Benchmark: legacy (querystring) vs. WHATWG

url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="noencode": 2,671,975.4115417274
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="noencode": 3,008,967.4994620117
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="multicharsep": 2,615,972.2291527316
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multicharsep": 2,885,936.0222384227
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="encodefake": 1,493,426.4438126145
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodefake": 1,641,878.4775006676
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="encodemany": 2,565,969.8400934297
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodemany": 3,025,840.9510215684
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="encodelast": 2,613,356.200269775
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="encodelast": 2,901,369.8722106437
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="multivalue": 2,615,576.6498150555
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multivalue": 2,502,413.2021715143
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="multivaluemany": 1,383,932.6225206275
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="multivaluemany": 985,021.4923070684
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="manypairs": 399,773.9756297489
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="manypairs": 804,542.90558165
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="manyblankpairs": 14,459,573.094118012
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="manyblankpairs": 10,583,700.054130334
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="legacy" type="altspaces": 1,092,358.4670786818
url/legacy-vs-whatwg-url-searchparams-serialize.js n=1000000 method="whatwg" type="altspaces": 1,174,023.8424377916

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

url

TimothyGu · 2017-03-01T04:23:09Z

CI: https://ci.nodejs.org/job/node-test-commit/8180/

jasnell · 2017-03-01T05:39:54Z

Awesome! Will take a look at it in detail tomorrow

targos · 2017-03-01T16:04:18Z

lib/internal/url.js

nit: len === 0 to avoid conversion to boolean

targos · 2017-03-01T16:08:49Z

lib/internal/url.js

nit: use len also here or do not create it earlier

targos · 2017-03-01T16:17:57Z

test/parallel/test-whatwg-url-searchparams-constructor.js

can you update the Refs SHA? This fix was in e94c604

joyeecheung

Great work! Feel good to see the tests get uncommented =)

joyeecheung · 2017-03-02T02:42:23Z

lib/internal/url.js

nit: Maybe noEscapeTable? The name noEscapeParam together with the function name escapeParam looks a bit weird..

Yeah... I agree. Used the more standard noEscape instead.

joyeecheung · 2017-03-02T03:02:07Z

lib/internal/url.js

Another way would be to fork the hexTable too with hexTable[0x20] = '+'..not sure how much that would gain though.

Nice, that does bring some speedup (consistently 1-2% for altspace benchmark).

TimothyGu · 2017-03-04T02:27:30Z

@targos, @joyeecheung, PR updated to address comments.

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

joyeecheung · 2017-03-04T14:16:29Z

Still LGTM

TimothyGu · 2017-03-08T01:15:19Z

@jasnell PTAL...

joyeecheung · 2017-03-14T06:20:17Z

Ping @jasnell @targos

jasnell

My apologies on the late review.. been a bit distracted. This LGTM so long as CI is green :-)

TimothyGu · 2017-03-14T19:22:31Z

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

TimothyGu · 2017-03-14T20:15:21Z

CI is actually passing. Landed in d77a758. Thanks all.

PR-URL: nodejs#11626 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

italoacasas · 2017-03-20T14:21:37Z

@TimothyGu this is not landing in v7.x-staging because git conflicts.

Question: do you have a list of all the URL commits that need backport?

PR-URL: nodejs#11626 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 1, 2017

TimothyGu mentioned this pull request Mar 1, 2017

url: track status of a spec-compliant URLSearchParams #10821

Closed

11 tasks

targos reviewed Mar 1, 2017

View reviewed changes

joyeecheung approved these changes Mar 2, 2017

View reviewed changes

joyeecheung reviewed Mar 2, 2017

View reviewed changes

TimothyGu force-pushed the urlsearchparams-serialize branch 2 times, most recently from 805fbcf to b05e5de Compare March 4, 2017 02:26

TimothyGu requested a review from jasnell March 6, 2017 05:15

targos approved these changes Mar 14, 2017

View reviewed changes

jasnell approved these changes Mar 14, 2017

View reviewed changes

TimothyGu added 2 commits March 14, 2017 11:29

url: spec-compliant URLSearchParams serializer

d75a00d

test: enable URL query serializer tests

059e1fb

TimothyGu force-pushed the urlsearchparams-serialize branch from b05e5de to 059e1fb Compare March 14, 2017 19:20

watilde approved these changes Mar 14, 2017

View reviewed changes

TimothyGu closed this Mar 14, 2017

TimothyGu deleted the urlsearchparams-serialize branch March 14, 2017 20:15

TimothyGu restored the urlsearchparams-serialize branch March 14, 2017 20:15

TimothyGu deleted the urlsearchparams-serialize branch March 14, 2017 20:29

italoacasas added the dont-land-on-v7.x label Mar 20, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

TimothyGu added backport-requested-v7.x and removed dont-land-on-v7.x labels Apr 19, 2017

TimothyGu added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017

Uh oh!

Conversation

TimothyGu commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu commented Mar 1, 2017

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

targos Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

targos Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

targos Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Mar 4, 2017

Uh oh!

joyeecheung commented Mar 4, 2017

Uh oh!

TimothyGu commented Mar 8, 2017

Uh oh!

joyeecheung commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Mar 14, 2017

Uh oh!

TimothyGu commented Mar 14, 2017

Uh oh!

italoacasas commented Mar 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

TimothyGu commented Mar 1, 2017 •

edited

Loading

joyeecheung commented Mar 14, 2017 •

edited

Loading