Skip to content

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 21, 2023

Summary

  • Validation: We didn't check for the length of the iterable objects passed to the URLSearchParams. This pull request adds that.
  • Performance: My local benchmarks show 45 to 80% faster URLSearchParams creation.

Footnote

There is one particular change made regarding to a comment written in 2017:

// Note: per spec we have to first exhaust the lists then process them

Upon investigating URL spec, I couldn't find any specific reasoning for this and removed it. The spec mentions that:

To initialize a [URLSearchParams](https://url.spec.whatwg.org/#urlsearchparams) object query with init, run these steps:

If init is a [sequence](https://webidl.spec.whatwg.org/#idl-sequence), then [for each](https://infra.spec.whatwg.org/#list-iterate) innerSequence of init:

If innerSequence’s [size](https://infra.spec.whatwg.org/#list-size) is not 2, then [throw](https://webidl.spec.whatwg.org/#dfn-throw) a [TypeError](https://webidl.spec.whatwg.org/#exceptiondef-typeerror).

[Append](https://infra.spec.whatwg.org/#list-append) (innerSequence[0], innerSequence[1]) to query’s [list](https://url.spec.whatwg.org/#concept-urlsearchparams-list).

Benchmark Result

                                                                                     confidence improvement accuracy (*)   (**)   (***)
url/url-searchparams-creation.js n=10000 inputType='iterable' type='array'                  ***     45.05 %       ±6.82% ±9.19% ±12.19%
url/url-searchparams-creation.js n=10000 inputType='iterable' type='encodelast'             ***     81.96 %       ±2.30% ±3.08%  ±4.06%
url/url-searchparams-creation.js n=10000 inputType='iterable' type='encodemany'             ***     83.22 %       ±1.73% ±2.32%  ±3.05%
url/url-searchparams-creation.js n=10000 inputType='iterable' type='multiprimitives'        ***     80.23 %       ±2.69% ±3.58%  ±4.66%
url/url-searchparams-creation.js n=10000 inputType='iterable' type='noencode'               ***     80.30 %       ±2.55% ±3.42%  ±4.49%
url/url-searchparams-creation.js n=10000 inputType='object' type='array'                             1.47 %       ±2.71% ±3.62%  ±4.74%
url/url-searchparams-creation.js n=10000 inputType='object' type='encodelast'                **      2.78 %       ±1.72% ±2.30%  ±3.00%
url/url-searchparams-creation.js n=10000 inputType='object' type='encodemany'                       -0.26 %       ±3.28% ±4.41%  ±5.82%
url/url-searchparams-creation.js n=10000 inputType='object' type='multiprimitives'          ***      2.76 %       ±1.21% ±1.62%  ±2.11%
url/url-searchparams-creation.js n=10000 inputType='object' type='noencode'                          1.39 %       ±1.59% ±2.12%  ±2.76%
url/url-searchparams-creation.js n=10000 inputType='string' type='array'                             2.38 %       ±5.54% ±7.46%  ±9.88%
url/url-searchparams-creation.js n=10000 inputType='string' type='encodelast'                        1.04 %       ±1.99% ±2.65%  ±3.47%
url/url-searchparams-creation.js n=10000 inputType='string' type='encodemany'                       -0.97 %       ±1.19% ±1.58%  ±2.06%
url/url-searchparams-creation.js n=10000 inputType='string' type='multiprimitives'                  -1.60 %       ±1.67% ±2.23%  ±2.90%
url/url-searchparams-creation.js n=10000 inputType='string' type='noencode'                          0.74 %       ±4.75% ±6.37%  ±8.40%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 15 comparisons, you can thus expect the following amount of false-positive results:
  0.75 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.15 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

cc @nodejs/url

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 21, 2023
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Mar 21, 2023
@anonrig anonrig force-pushed the url-search-params-improve branch from 7ed296b to 3bb3f8c Compare March 21, 2023 13:59
@anonrig anonrig added url Issues and PRs related to the legacy built-in url module. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig force-pushed the url-search-params-improve branch from 3bb3f8c to cfe7a49 Compare March 21, 2023 15:58
@mscdex mscdex added the needs-benchmark-ci PR that need a benchmark CI run. label Mar 21, 2023
@anonrig
Copy link
Member Author

anonrig commented Mar 21, 2023

@mscdex afaik, benchmark ci is down...

@mscdex
Copy link
Contributor

mscdex commented Mar 21, 2023

@anonrig Wait for it to come back up or ask @nodejs/build about it?

@anonrig
Copy link
Member Author

anonrig commented Mar 21, 2023

@anonrig Wait for it to come back up or ask @nodejs/build about it?

Definitely, @mscdex. I created an issue: nodejs/build#3245

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the review wanted PRs that need reviews. label Mar 22, 2023
@anonrig anonrig requested a review from jasnell March 22, 2023 15:51
@anonrig
Copy link
Member Author

anonrig commented Mar 23, 2023

@nodejs/url please review

@anonrig anonrig force-pushed the url-search-params-improve branch from cfe7a49 to b2cd992 Compare March 24, 2023 13:55
if ((typeof pair !== 'object' && typeof pair !== 'function') ||
pair === null ||
typeof pair[SymbolIterator] !== 'function') {
if (pair == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this branch to be with the catch-all else? You'll need to change the if clause below to

 if (pair == null ||
     typeof pair !== 'object' && typeof pair !== 'function' ||
     typeof pair[SymbolIterator] !== 'function')) {

but it reads a bit nicer.

@anonrig
Copy link
Member Author

anonrig commented Apr 3, 2023

@mscdex I started a benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1316/

@jasnell @nodejs/url Can you review?

url/url-searchparams-creation.js n=1000000 inputType='iterable' type='array'                                           ***     28.65 %       ±3.66%  ±4.87%  ±6.34%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='encodelast'                                      ***     76.70 %       ±5.39%  ±7.22%  ±9.49%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='encodemany'                                      ***     76.64 %       ±5.16%  ±6.92%  ±9.11%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='multiprimitives'                                 ***     72.97 %       ±5.19%  ±6.94%  ±9.10%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='noencode'                                        ***     82.60 %       ±5.46%  ±7.32%  ±9.64%
url/url-searchparams-creation.js n=1000000 inputType='object' type='array'                                                      1.02 %       ±3.15%  ±4.20%  ±5.46%
url/url-searchparams-creation.js n=1000000 inputType='object' type='encodelast'                                                -3.32 %       ±3.88%  ±5.16%  ±6.72%
url/url-searchparams-creation.js n=1000000 inputType='object' type='encodemany'                                                -0.23 %       ±2.83%  ±3.76%  ±4.89%
url/url-searchparams-creation.js n=1000000 inputType='object' type='multiprimitives'                                            0.63 %       ±3.51%  ±4.66%  ±6.07%
url/url-searchparams-creation.js n=1000000 inputType='object' type='noencode'                                                   1.00 %       ±3.57%  ±4.76%  ±6.21%
url/url-searchparams-creation.js n=1000000 inputType='string' type='array'                                                      1.00 %       ±4.28%  ±5.70%  ±7.41%
url/url-searchparams-creation.js n=1000000 inputType='string' type='encodelast'                                                -2.44 %       ±2.92%  ±3.89%  ±5.06%
url/url-searchparams-creation.js n=1000000 inputType='string' type='encodemany'                                                -2.24 %       ±2.74%  ±3.64%  ±4.74%
url/url-searchparams-creation.js n=1000000 inputType='string' type='multiprimitives'                                           -1.51 %       ±3.47%  ±4.61%  ±6.00%
url/url-searchparams-creation.js n=1000000 inputType='string' type='noencode'                                                  -0.19 %       ±3.33%  ±4.42%  ±5.76%

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2023
@nodejs-github-bot nodejs-github-bot merged commit bf41f76 into nodejs:main Apr 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in bf41f76

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. review wanted PRs that need reviews. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants