Skip to content

url: offload URLSearchParams initialization#46867

Closed
anonrig wants to merge 2 commits intonodejs:mainfrom
anonrig:fix-search-setter
Closed

url: offload URLSearchParams initialization#46867
anonrig wants to merge 2 commits intonodejs:mainfrom
anonrig:fix-search-setter

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Feb 27, 2023 •
edited
Loading

Removed unused kFormat from URL and offloaded URLSearchParams to not get created or updated unless accessed.

cc @nodejs/performance

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 27, 2023

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 Feb 27, 2023
@anonrig anonrig changed the title url: remove initSearchParams on search setter url: offload URLSearchParams initialization Feb 27, 2023
@anonrig anonrig requested a review from lemire February 27, 2023 14:33
@anonrig anonrig added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. performance Issues and PRs related to the performance of Node.js. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 27, 2023
lemire
lemire approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

The search parameters are expensive to instantiate so they are computed 'on demand' or lazily.

@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

nodejs-github-bot commented Feb 27, 2023

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

@anonrig anonrig force-pushed the fix-search-setter branch 4 times, most recently from dd808db to 6fc393b Compare February 27, 2023 16:36
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
@anonrig anonrig requested a review from aduh95 February 27, 2023 21:01
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label 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

nodejs-github-bot commented Feb 27, 2023

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

mcollina
mcollina approved these changes 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.

lgtm

@mcollina
Copy link
Member

mcollina commented Feb 27, 2023

Including a benchmark run would be helpful ;). I saw the tweet.

@lemire
Copy link
Member

lemire commented Feb 27, 2023

@mcollina If this has the massive performance gains that @anonrig states, then this should be reported on GitHub. ;-)

aduh95
aduh95 approved these changes Feb 27, 2023
View reviewed changes
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2023

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

@anonrig
Copy link
Member Author

anonrig commented Mar 1, 2023

I had to force-push to fix the conflict. Can you re-review it?

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2023

Landed in 3646a66...e3bb668

nodejs-github-bot pushed a commit that referenced this pull request Mar 1, 2023
@anonrig @nodejs-github-bot
url: remove unused kFormat from url
a851391 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 1, 2023
@anonrig @nodejs-github-bot
url: offload URLSearchParams initialization
e3bb668 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@github-actions github-actions bot mentioned this pull request Mar 2, 2023
Open
15 tasks
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @targos
url: remove unused kFormat from url
8422ac2 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @targos
url: offload URLSearchParams initialization
1674601 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
@anonrig @targos
url: remove unused kFormat from url
f8765be 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
@anonrig @targos
url: offload URLSearchParams initialization
1fa6352 
PR-URL: #46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@targos targos mentioned this pull request Mar 14, 2023
Merged
anonrig added a commit to anonrig/node that referenced this pull request Jun 23, 2023
@anonrig
url: remove unused kFormat from url
8fa78d8 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jun 23, 2023
@anonrig
url: offload URLSearchParams initialization
fc3356f 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@anonrig anonrig mentioned this pull request Jun 23, 2023
Closed
19 tasks
anonrig added a commit to anonrig/node that referenced this pull request Jun 26, 2023
@anonrig
url: remove unused kFormat from url
ff7593b 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jun 26, 2023
@anonrig
url: offload URLSearchParams initialization
3921767 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jul 6, 2023
@anonrig
url: remove unused kFormat from url
1d3c55f 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jul 6, 2023
@anonrig
url: offload URLSearchParams initialization
8f54f6c 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jul 9, 2023
@anonrig
url: remove unused kFormat from url
65c64b4 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
anonrig added a commit to anonrig/node that referenced this pull request Jul 9, 2023
@anonrig
url: offload URLSearchParams initialization
efdd768 
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 12, 2023
@anonrig @danielleadams
url: remove unused kFormat from url
3db235b 
PR-URL: #46867
Backport-PR-URL: #48345
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 12, 2023
@anonrig @danielleadams
url: offload URLSearchParams initialization
c79e1b7 
PR-URL: #46867
Backport-PR-URL: #48345
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 approved these changes

@mcollina mcollina mcollina approved these changes

@panva panva panva approved these changes

@lemire lemire lemire approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@aymen94 aymen94 aymen94 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. 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.