GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — More URLSearchParams improvements by TimothyGu · Pull Request #10399 · nodejs/node

Conversation

@TimothyGu

@TimothyGu TimothyGu commented Dec 21, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
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 {
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ],
  [ '', '' ] }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

domenic reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 21, 2016
@TimothyGu

TimothyGu commented Dec 21, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

We could also consider exporting URLSearchParams from the querystring module.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu
TimothyGu force-pushed the urlsearchparams branch from dd8fecb to 06ba384 Compare December 21, 2016 23:11
mscdex
mscdex reviewed Dec 21, 2016
View reviewed changes
Comment thread lib/internal/url.js Outdated

@mscdex mscdex Dec 21, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex

mscdex commented Dec 21, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu

TimothyGu commented Dec 21, 2016

Copy link
Copy Markdown
Member Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jasnell commented Dec 23, 2016

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell requested changes Dec 23, 2016
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread benchmark/url/url-searchparams-iteration.js Outdated

@jasnell jasnell Dec 23, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Dec 23, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread benchmark/url/url-searchparams-iteration.js Outdated

@jasnell jasnell Dec 23, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Dec 23, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@joyeecheung joyeecheung mentioned this pull request Dec 23, 2016
url: make WHATWG URL properties spec compliant #10408
Closed
3 tasks done
@targos targos added dont-land-on-v4.x labels Dec 28, 2016
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
@mscdex 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
TimothyGu force-pushed the urlsearchparams branch from 22fb822 to 009a147 Compare January 14, 2017 20:29
@joyeecheung

joyeecheung commented Jan 14, 2017

Copy link
Copy Markdown
Member

@TimothyGu Can you rebase this onto the master?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu

TimothyGu commented Jan 14, 2017

Copy link
Copy Markdown
Member Author

@joyeecheung, I have just done so.

joyeecheung reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu
TimothyGu force-pushed the urlsearchparams branch from ddfc3bb to 7df717c Compare January 15, 2017 01:07
joyeecheung
joyeecheung reviewed Jan 15, 2017
View reviewed changes
Comment thread benchmark/url/url-searchparams-common.js Outdated

@joyeecheung joyeecheung Jan 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file should be in benchmark/fixtures?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Jan 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm did not notice that before. Thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Jan 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread benchmark/url/url-searchparams-iteration.js Outdated

@joyeecheung joyeecheung Jan 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread benchmark/url/url-searchparams-iteration.js Outdated

@joyeecheung joyeecheung Jan 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Jan 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread benchmark/url/url-searchparams-iteration.js Outdated

@joyeecheung joyeecheung Jan 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Jan 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@joyeecheung joyeecheung Jan 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@TimothyGu TimothyGu Jan 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@joyeecheung

joyeecheung commented Jan 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

+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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Jan 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Jan 15, 2017

Copy link
Copy Markdown
Member

Other than that I think this is good to go

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TimothyGu added 2 commits January 15, 2017 10:20
@TimothyGu
benchmark: add some benchmarks for URLSearchParams
97b477a
@TimothyGu
url: change URLSearchParams's backing store to an array
36e8820
@TimothyGu
TimothyGu force-pushed the urlsearchparams branch from 7df717c to 5d67048 Compare January 15, 2017 18:22
@TimothyGu
url: add custom inspection for URLSearchParams and its iterators
87f6c3a
@TimothyGu
TimothyGu force-pushed the urlsearchparams branch from 5d67048 to 87f6c3a Compare January 15, 2017 18:28
@TimothyGu TimothyGu mentioned this pull request Jan 15, 2017
url: track status of a spec-compliant URLSearchParams #10821
Closed
11 tasks done
@TimothyGu
TimothyGu force-pushed the urlsearchparams branch from 2900381 to 87f6c3a Compare January 15, 2017 20:28
@TimothyGu

TimothyGu commented Jan 17, 2017

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu

TimothyGu commented Jan 18, 2017

Copy link
Copy Markdown
Member Author

@jasnell, ping.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Jan 18, 2017
View reviewed changes
@jasnell

jasnell commented Jan 18, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@italoacasas

italoacasas commented Jan 19, 2017

Copy link
Copy Markdown

Landed 98bb65f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@italoacasas italoacasas closed this Jan 19, 2017
italoacasas pushed a commit that referenced this pull request Jan 19, 2017
@TimothyGu Italo A. Casas
url: improving URLSearchParams
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]>
@TimothyGu
TimothyGu deleted the urlsearchparams branch January 19, 2017 23:07
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@TimothyGu Italo A. Casas
url: improving URLSearchParams
a239d53
- 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]>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Proposal to update v7.x-staging #10989
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@TimothyGu Italo A. Casas
url: improving URLSearchParams
5d33c96
- 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]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
v7.5.0 proposal #11062
Merged
@skyclouds2001 skyclouds2001 mentioned this pull request Nov 11, 2024
Update Node.js data for URLSearchParams mdn/browser-compat-data#24998
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments
@jasnell jasnell jasnell approved these changes
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@TimothyGu @mscdex @jasnell @joyeecheung @italoacasas @targos @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.