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.

Sign up for GitHub

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

Jump to bottom

benchmark: add a benchmark for URLSearchParams creation and toString() #46810

Merged
merged 1 commit into from
Mar 13, 2023
Merged

benchmark: add a benchmark for URLSearchParams creation and toString() #46810

merged 1 commit into from
Mar 13, 2023

Conversation

debadree25
Copy link
Member

Was trying to work on nodejs/performance#56 noticed there is no benchmark on URLSearchParams.toString() hence adding one same as benchmark/querystring/querystring-stringify.js

Refs: nodejs/performance#56

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. url Issues and PRs related to the legacy built-in url module. labels Feb 24, 2023
@debadree25
Copy link
Member Author

I am also trying to improve the perf of URLSearchParams.toString() here f15896c, there seems to be some improvement, running this benchmark the results look something like

                                                                  confidence improvement accuracy (*)   (**)  (***)
url/url-searchparams-toString.js n=1000000 type='array'                  ***     23.29 %       ±4.91% ±6.54% ±8.51%
url/url-searchparams-toString.js n=1000000 type='encodelast'                     -4.55 %       ±4.63% ±6.16% ±8.03%
url/url-searchparams-toString.js n=1000000 type='encodemany'               *     -4.37 %       ±3.99% ±5.31% ±6.92%
url/url-searchparams-toString.js n=1000000 type='multiprimitives'                -4.40 %       ±4.43% ±5.90% ±7.68%
url/url-searchparams-toString.js n=1000000 type='noencode'                       -1.37 %       ±4.41% ±5.87% ±7.64%

Trying to follow the cues from the code of https://github.com/anonrig/fast-querystring but it seems the main bottleneck lies in the regex checking over

node/lib/internal/util.js

Line 78 in f15896c

const match = RegExpPrototypeExec(unpairedSurrogateRe, str);

which is required for spec compliance I believe? so cannot entirely copy the code from https://github.com/anonrig/fast-querystring too? If anyone has any more ideas would be grateful!!

anonrig
anonrig reviewed Feb 24, 2023
View reviewed changes
benchmark/url/url-searchparams-toString.js Outdated

bench.start();
for (let i = 0; i < n; i++)
new URLSearchParams(input).toString();
Copy link
Member

Choose a reason for hiding this comment

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

If you are only testing the performance of toString, it feels like initialization should not be inside the bench.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should spilt it into two, there is also no benchmark for the creation of the URLSearchParams object would that be good?

benchmark/url/url-searchparams-toString.js Show resolved Hide resolved
@anonrig anonrig added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 24, 2023
@debadree25 debadree25 changed the title benchmark: add a benchmark for URLSearchParams.toString() benchmark: add a benchmark for URLSearchParams creation and toString() Feb 24, 2023
@debadree25 debadree25 force-pushed the ft/benchmark-url-toString branch from 1c52b18 to b2bd057 Compare February 24, 2023 19:27
@debadree25
Copy link
Member Author

Used all types of possible parameter types for URLSearchParams and added another benchmark for the creation of the object PTAL @anonrig

@anonrig
Copy link
Member

anonrig commented Feb 26, 2023

cc @nodejs/performance @nodejs/url

@debadree25 debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think it might be more useful to compare it against querystring than on itself.

@debadree25
Copy link
Member Author

Is there any example of comparative benchmarks with other functions in our benchmark tests?

@debadree25
Copy link
Member Author

Ok we already have benchmark/url/legacy-vs-whatwg-url-searchparams-parse.js for comparison and this one would only add for individual perf checking @mcollina

@github-actions github-actions bot mentioned this pull request Feb 28, 2023
Open
16 tasks
This was referenced Mar 1, 2023
Open
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@debadree25 debadree25 requested a review from mcollina March 2, 2023 05:08
This was referenced Mar 3, 2023
Open
Open
This was referenced Mar 5, 2023
Open
Open
Open
@debadree25 debadree25 requested review from mcollina and removed request for mcollina March 9, 2023 17:58
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 28c9fb9 into nodejs:main Mar 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 28c9fb9

targos pushed a commit that referenced this pull request Mar 13, 2023
@debadree25 @targos
benchmark: add a benchmark for URLSearchParams creation and toString()
cf80847 
Refs: nodejs/performance#56
PR-URL: #46810
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
@debadree25 @targos
benchmark: add a benchmark for URLSearchParams creation and toString()
7cfd31a 
Refs: nodejs/performance#56
PR-URL: #46810
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@debadree25 @danielleadams
benchmark: add a benchmark for URLSearchParams creation and toString()
b5db3b5 
Refs: nodejs/performance#56
PR-URL: #46810
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@UlisesGascon UlisesGascon UlisesGascon approved these changes

@anonrig anonrig anonrig approved these changes

@watilde watilde watilde approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@debadree25 @anonrig @nodejs-github-bot @mcollina @jasnell @benjamingr @watilde @UlisesGascon @RafaelGSS