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

url: offload URLSearchParams initialization #46867

Closed
wants to merge 2 commits into from
Closed

url: offload URLSearchParams initialization #46867

wants to merge 2 commits into from

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

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

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
lib/internal/url.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
lib/internal/url.js Outdated Show resolved Hide resolved
@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

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

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
lib/internal/url.js Show resolved Hide resolved
@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

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

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
@danielleadams danielleadams added the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Apr 3, 2023
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
@danielleadams danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. labels Jul 12, 2023
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

@aymen94 aymen94 aymen94 approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@lemire lemire lemire approved these changes

@panva panva panva 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. backported-to-v18.x PRs backported to the v18.x-staging branch. 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.