Skip to content

url: offload URLSearchParams initialization - #46867

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

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

Conversation

@anonrig

@anonriganonrig commented Feb 27, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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

cc @nodejs/performance

@nodejs-github-bot

nodejs-github-bot commented Feb 27, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-botnodejs-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
@anonriganonrig changed the title url: remove initSearchParams on search setterurl: offload URLSearchParams initializationFeb 27, 2023
@anonrig
anonrig requested a review from lemireFebruary 27, 2023 14:33
@anonriganonrig 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

@lemirelemire left a comment

Copy link
Copy Markdown
Member

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-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 27, 2023

Copy link
Copy Markdown
Collaborator

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

@anonrig
anonrigforce-pushed the fix-search-setter branch 4 times, most recently from dd808db to 6fc393bCompareFebruary 27, 2023 16:36
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
Comment threadlib/internal/url.js Outdated
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
Comment threadlib/internal/url.js Outdated
@anonrig
anonrig requested a review from aduh95February 27, 2023 21:01
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 27, 2023

Copy link
Copy Markdown
Collaborator

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

mcollina
mcollina approved these changes Feb 27, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

mcollina commented Feb 27, 2023

Copy link
Copy Markdown
Member

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

@lemire

lemire commented Feb 27, 2023

Copy link
Copy Markdown
Member

@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
Comment threadlib/internal/url.js Outdated
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 1, 2023

Copy link
Copy Markdown
Collaborator

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

@anonrig

anonrig commented Mar 1, 2023

Copy link
Copy Markdown
MemberAuthor

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

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

nodejs-github-bot commented Mar 1, 2023

Copy link
Copy Markdown
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-actionsgithub-actionsBot 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>
@targostargos mentioned this pull request Mar 14, 2023
Merged
@anonriganonrig mentioned this pull request Jun 23, 2023
Closed
19 tasks
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>
@danielleadamsdanielleadams mentioned this pull request Jul 12, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 approved these changes
@mcollinamcollinamcollina approved these changes
@panvapanvapanva approved these changes
@lemirelemirelemire approved these changes
@jasnelljasnelljasnell approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
+2 more reviewers
@TimothyGuTimothyGuTimothyGu approved these changes
@aymen94aymen94aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@anonrig@nodejs-github-bot@mcollina@lemire@panva@jasnell@benjamingr@TimothyGu@aymen94@aduh95@danielleadams