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.

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

url: improve URLSearchParams creation performance #47190

Merged
merged 1 commit into from Apr 3, 2023

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 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

@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

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
24 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in bf41f76

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47190
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
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.

None yet

6 participants