More URLSearchParams improvements #10399

TimothyGu · 2016-12-21T23:07:22Z

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)

url

Description of change

Various improvements are done to the URLSearchParams class, including making the storage an array (in preparation for writing native C++ parsing and serialization routines, as well as to preserve the order of param entry addition), and adding support for util.inspect. Also, the URLSearchParams constructor is now exported as a property of the url module.

                                                                            improvement significant      p.value
 url/url-searchparams-iteration.js n=1000000 method="forEach"                 1862.07 %         *** 8.733519e-13
 url/url-searchparams-iteration.js n=1000000 method="iterator"                 438.64 %         *** 5.047367e-08
 url/url-searchparams-parse.js n=100000 type="encodelast"                      -13.24 %         *** 4.168480e-05
 url/url-searchparams-parse.js n=100000 type="encodemany"                      -11.09 %         *** 5.541885e-04
 url/url-searchparams-parse.js n=100000 type="manypairs"                       -27.59 %         *** 2.321083e-06
 url/url-searchparams-parse.js n=100000 type="multicharsep"                    -50.31 %         *** 5.783889e-08
 url/url-searchparams-parse.js n=100000 type="multivalue"                      -17.29 %           * 2.935815e-02
 url/url-searchparams-parse.js n=100000 type="multivaluemany"                  -14.88 %         *** 5.745400e-05
 url/url-searchparams-parse.js n=100000 type="noencode"                        -18.15 %         *** 1.963056e-06
 url/url-searchparams-read.js n=1000000 param="nonexistent" method="get"        61.67 %         *** 1.116965e-10
 url/url-searchparams-read.js n=1000000 param="nonexistent" method="getAll"     55.08 %         *** 3.448118e-06
 url/url-searchparams-read.js n=1000000 param="nonexistent" method="has"        44.27 %         *** 3.973790e-08
 url/url-searchparams-read.js n=1000000 param="one" method="get"                88.68 %         *** 1.517226e-12
 url/url-searchparams-read.js n=1000000 param="one" method="getAll"             38.12 %         *** 2.725035e-09
 url/url-searchparams-read.js n=1000000 param="one" method="has"                43.55 %           * 4.359301e-02
 url/url-searchparams-read.js n=1000000 param="three" method="get"              67.10 %         *** 1.838817e-06
 url/url-searchparams-read.js n=1000000 param="three" method="getAll"           20.47 %         *** 3.453068e-04
 url/url-searchparams-read.js n=1000000 param="three" method="has"              37.42 %         *** 2.080444e-14
 url/url-searchparams-read.js n=1000000 param="two" method="get"                74.53 %         *** 4.785676e-06
 url/url-searchparams-read.js n=1000000 param="two" method="getAll"             27.61 %         *** 2.396046e-10
 url/url-searchparams-read.js n=1000000 param="two" method="has"                42.86 %         *** 1.950919e-05
 url/url-searchparams-stringifier.js n=100000 type="encodelast"                -24.12 %         *** 2.144126e-06
 url/url-searchparams-stringifier.js n=100000 type="encodemany"                -21.01 %           * 1.564314e-02
 url/url-searchparams-stringifier.js n=100000 type="manypairs"                 -50.85 %         *** 1.451279e-07
 url/url-searchparams-stringifier.js n=100000 type="multicharsep"              -23.25 %         *** 1.969569e-07
 url/url-searchparams-stringifier.js n=100000 type="multivalue"                -21.85 %         *** 6.488499e-08
 url/url-searchparams-stringifier.js n=100000 type="multivaluemany"            -20.78 %         *** 3.153256e-09
 url/url-searchparams-stringifier.js n=100000 type="noencode"                  -23.54 %         *** 3.425512e-07

> new url.URLSearchParams('=&=&=&=&=&=&=&=&=')
URLSearchParams {
  '' => '',
  '' => '',
  '' => '',
  '' => '',
  '' => '',
  '' => '',
  '' => '',
  '' => '',
  '' => '' }
> new url.URLSearchParams('=&=&=&=&=&=&=&=&=').entries()
URLSearchParamsIterator {
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ] }

TimothyGu · 2016-12-21T23:09:21Z

We could also consider exporting URLSearchParams from the querystring module.

mscdex · 2016-12-21T23:20:43Z

lib/internal/url.js

I haven't benched Object.create(null) with V8 5.4 to see if it performs any better now, but you might want to consider using the same trick used in querystring.js for better performance.

mscdex · 2016-12-21T23:24:19Z

I'm concerned about the performance hits here. Is an array absolutely necessary?

TimothyGu · 2016-12-21T23:40:08Z

@mscdex, first off this API is still considered experimental. Performance is not a first priority. Second, the benchmarks that really matter (iteration and reading) all show significant performance increase. Why I said "really matter" is because the plan is to eventually stop using querystring module, and because of that the use of conversion functions like getObjectFromParams and getParamsFromObject will be stopped. Third, an array is necessary to preserve standard compliance. For example,

var params = new URLSearchParams();
params.append('a', 'a');
params.append('b', 'b');
params.append('a', 'c');
for (var [ name, val ] of params) { console.log(name, val); }

With object

a a
a c
b b

With array (correct)

a a
b b
a c

jasnell · 2016-12-23T04:00:59Z

We will definitely need to put some work into the performance very soon, but I agree that the focus on standards compliance first is the right approach.

jasnell

Almost there!

jasnell · 2016-12-23T04:02:23Z

benchmark/url/url-searchparams-iteration.js

Look at https://github.com/nodejs/node/blob/master/benchmark/common.js#L211 .. there is a utility method that takes care of the V8 optimization bits.

The v8ForceOptimization function forces the this context of the function to be null, which means that functions that depend on this to be set properly, like params.forEach cannot be used with v8ForceOptimization.

jasnell · 2016-12-23T04:03:00Z

benchmark/url/url-searchparams-iteration.js

why do it like this? why not ['forEach', 'iterator']?

This was from the existing url/new-url-parse.js benchmark.

joyeecheung · 2017-01-14T20:32:03Z

@TimothyGu Can you rebase this onto the master?

TimothyGu · 2017-01-14T20:32:22Z

@joyeecheung, I have just done so.

joyeecheung · 2017-01-15T07:38:48Z

benchmark/url/url-searchparams-common.js

I think this file should be in benchmark/fixtures?

Hmm did not notice that before. Thanks.

The convention seems to be to include this type of data in all benchmark files, so I did that instead.

joyeecheung · 2017-01-15T07:45:57Z

benchmark/url/url-searchparams-iteration.js

createBenchmark now takes a third argument for flags that will be turned on when running main(see the guide), so I think this can be replaced by passing a { flags: ['--allow_natives_syntax'] } to createBenchmark as the third argument.

joyeecheung · 2017-01-15T08:01:06Z

benchmark/url/url-searchparams-iteration.js

To make OptimizeFunctionOnNextCall work properly, we should call it after making the first call to params.forEach(it needs enough type info to optimize, which can be gathered in the first call).

BTW I am not sure we do need to do this step, because the optimizing compiler doesn't necessarily have enough type feedback to properly optimize this with just one call. I believe this conventions is to prevent the OSR(on-stack replacement) from affecting the result, but if the compiler doesn't have enough type info during the first optimization, it might still kick in again in the loop.

I guess I do have to look into these calls more carefully, esp. wrt. their necessity.

joyeecheung · 2017-01-15T08:19:05Z

benchmark/url/url-searchparams-iteration.js

Testing against a noop can easily lead to weird results when the optimizing compiler can do JIT optimizations. This line is a loop invariant so could be code-motioned, leaving us timing an empty loop(which could explain the 1862.07 % improvement, can't be sure without looking into the IR). Maybe we can do something with i and the params in the loop(thus breaking the invariant), and add another case with for-in?

I believe it is the fact that we now store an array so no further processing (conversion from object to array) is needed.

When callback is noop, it is possible for the optimizing compiler to eliminate callback.call(thisArg, value, key, this) completely (again, can't be sure without looking into IR, but it's possible). Stopping reassigning pairs definitely could contribute to it.

Added a noDead variable similar to your benchmark PR. Also note the increased performance of iterators as well, as iterators do not have a callback.

joyeecheung · 2017-01-15T09:12:05Z

+1 for better spec compliance. I am not very sure about the benchmark results without looking into IR but we can improve them later, no reason to block this(especially when this is blocking others like #10801 and #10635).

Aside: the micro benchmarks under benchmark would need to be looked into again when TurboFan starts to replace Crankshaft in deps/v8. These two have very different approaches about optimizing stuff(and very different IRs).

joyeecheung · 2017-01-15T09:22:41Z

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

Anything else that need to be addressed other than the benchmark stuff? @jasnell

jasnell · 2017-01-15T17:05:48Z

Other than that I think this is good to go

TimothyGu · 2017-01-17T04:41:34Z

@jasnell, any other things I need to change before this can be applied?

TimothyGu · 2017-01-18T17:09:54Z

@jasnell, ping.

jasnell · 2017-01-18T17:13:07Z

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

italoacasas · 2017-01-19T19:37:10Z

Landed 98bb65f

- add some benchmarks for URLSearchParams - change URLSearchParams backing store to an array - add custom inspection for URLSearchParams and its iterators PR-URL: #10399 Reviewed-By: James M Snell <[email protected]>

- add some benchmarks for URLSearchParams - change URLSearchParams backing store to an array - add custom inspection for URLSearchParams and its iterators PR-URL: nodejs#10399 Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 21, 2016

TimothyGu force-pushed the urlsearchparams branch from dd8fecb to 06ba384 Compare December 21, 2016 23:11

mscdex reviewed Dec 21, 2016

View reviewed changes

TimothyGu force-pushed the urlsearchparams branch from 06ba384 to 22fb822 Compare December 21, 2016 23:41

jasnell requested changes Dec 23, 2016

View reviewed changes

joyeecheung mentioned this pull request Dec 23, 2016

url: make WHATWG URL properties spec compliant #10408

Closed

3 tasks

targos added dont-land-on-v4.x labels Dec 28, 2016

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017

mscdex removed the url Issues and PRs related to the legacy built-in url module. label Jan 5, 2017

This was referenced Jan 12, 2017

url: extend URLSearchParams constructor (whatwg #175) #10635

Closed

url: export URLSearchParams alongside URL #10761

Closed

url: export URLSearchParams #10801

Closed

TimothyGu force-pushed the urlsearchparams branch from 22fb822 to 009a147 Compare January 14, 2017 20:29

TimothyGu force-pushed the urlsearchparams branch from ddfc3bb to 7df717c Compare January 15, 2017 01:07

joyeecheung reviewed Jan 15, 2017

View reviewed changes

TimothyGu added 2 commits January 15, 2017 10:20

benchmark: add some benchmarks for URLSearchParams

97b477a

url: change URLSearchParams's backing store to an array

36e8820

TimothyGu force-pushed the urlsearchparams branch from 7df717c to 5d67048 Compare January 15, 2017 18:22

url: add custom inspection for URLSearchParams and its iterators

87f6c3a

TimothyGu force-pushed the urlsearchparams branch from 5d67048 to 87f6c3a Compare January 15, 2017 18:28

TimothyGu mentioned this pull request Jan 15, 2017

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

Closed

11 tasks

TimothyGu force-pushed the urlsearchparams branch from 2900381 to 87f6c3a Compare January 15, 2017 20:28

jasnell approved these changes Jan 18, 2017

View reviewed changes

italoacasas closed this Jan 19, 2017

TimothyGu deleted the urlsearchparams branch January 19, 2017 23:07

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

skyclouds2001 mentioned this pull request Nov 11, 2024

Update Node.js data for URLSearchParams mdn/browser-compat-data#24998

Merged

Uh oh!

More URLSearchParams improvements #10399

More URLSearchParams improvements #10399

Uh oh!

Conversation

TimothyGu commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

TimothyGu commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 21, 2016

Uh oh!

TimothyGu commented Dec 21, 2016

Uh oh!

jasnell commented Dec 23, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 14, 2017

Uh oh!

TimothyGu commented Jan 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Jan 15, 2017

Uh oh!

TimothyGu commented Jan 17, 2017

Uh oh!

TimothyGu commented Jan 18, 2017

Uh oh!

jasnell commented Jan 18, 2017

Uh oh!

italoacasas commented Jan 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

TimothyGu commented Dec 21, 2016 •

edited

Loading

TimothyGu commented Dec 21, 2016 •

edited

Loading

joyeecheung Jan 15, 2017 •

edited

Loading

joyeecheung commented Jan 15, 2017 •

edited

Loading

joyeecheung commented Jan 15, 2017 •

edited

Loading