url: don't update URL immediately on update to URLSearchParams #51520

MattIPv4 · 2024-01-19T03:22:07Z

This removes the performance impact caused by updating URL when interacting with URL.searchParams, moving that cost to the first access of URL.search, URL.href, URL.toString(), or URL.toJSON(), or to the first update of any part of the URL.

Fixes: #51518

nodejs-github-bot · 2024-01-19T03:22:12Z

Review requested:

@nodejs/url

lib/internal/url.js

This appears to keep the "calling stringifier with this = {} didn't throw TypeError" test happy

MattIPv4 · 2024-01-19T04:26:03Z

tools/test.py wpt/test-url passes locally with the tweaks in b0270bc 🎉

anonrig · 2024-01-19T05:06:06Z

WPT tests can't be changed. I recommend changing them on upstream if you think there is a bug.

MattIPv4 · 2024-01-19T05:15:18Z

Ah, hm. That might explain why this is implemented the rather inefficient way it is currently then, as href has to be stored as it was originally passed, even though properly stringified it is something different.

MattIPv4 · 2024-01-19T06:12:15Z

I've asked in the WPT Matrix as to why those tests expect the encoding to behave the way it does, but I believe this push should achieve the same performance benefit while only switching to URLSearchParams once they've actively been updated.

… updates

MattIPv4 · 2024-01-19T07:47:27Z

Added a little benchmark, very open to suggestions on if there is a better way to do that. It seems to show a marked improvement with this fix though:

~/GitHub/nodejs/node url-searchparams-perf ?1 ❯ node
v21.6.0
~/GitHub/nodejs/node url-searchparams-perf ?1 ❯ node benchmark/url/url-searchparams-append.js 
url/url-searchparams-append.js n=1000 type="URL": 14,406.618538860292
... doesn't progress after 30m

~/GitHub/nodejs/node url-searchparams-perf !1 ❯ out/Release/node -v
v22.0.0-pre
~/GitHub/nodejs/node url-searchparams-perf !1 ❯ out/Release/node benchmark/url/url-searchparams-append.js
url/url-searchparams-append.js n=1000 type="URL": 2,437,045.0341551863
url/url-searchparams-append.js n=1000000 type="URL": 9,538,473.063871915
url/url-searchparams-append.js n=1000 type="URLSearchParams": 4,736,530.491415039
url/url-searchparams-append.js n=1000000 type="URLSearchParams": 10,762,833.70515695

MattIPv4 · 2024-01-19T08:25:47Z

Also, included a benchmark to check that the added logic in #updateContext when searchParams has been used isn't going to impact performance drastically.

~/GitHub/nodejs/node url-searchparams-perf ❯ node -v
v21.6.0
~/GitHub/nodejs/node url-searchparams-perf ❯ node benchmark/url/url-searchparams-update.js
url/url-searchparams-update.js n=1000000 property="pathname" searchParams="true": 3,393,713.6186052123
url/url-searchparams-update.js n=1000000 property="search" searchParams="true": 2,755,369.9839588143
url/url-searchparams-update.js n=1000000 property="pathname" searchParams="false": 3,645,786.3368688347
url/url-searchparams-update.js n=1000000 property="search" searchParams="false": 3,485,167.052351517

~/GitHub/nodejs/node url-searchparams-perf ❯ out/Release/node -v                                      
v22.0.0-pre
~/GitHub/nodejs/node url-searchparams-perf ❯ out/Release/node benchmark/url/url-searchparams-update.js
url/url-searchparams-update.js n=1000000 property="pathname" searchParams="true": 3,687,806.8658666275
url/url-searchparams-update.js n=1000000 property="search" searchParams="true": 2,727,997.506610279
url/url-searchparams-update.js n=1000000 property="pathname" searchParams="false": 3,526,772.9128872664
url/url-searchparams-update.js n=1000000 property="search" searchParams="false": 3,436,533.8775657485

nodejs-github-bot · 2024-01-24T14:36:37Z

Landed in 925a464

MattIPv4 · 2024-01-24T15:16:14Z

Should I have squashed the commits here before they were merged? I notice the commit into main uses the commit message of the first commit here, rather than the PR title, which results in what is now a bit of a misleading commit message?

aduh95 · 2024-01-24T17:41:18Z

Should I have squashed the commits here before they were merged? I notice the commit into main uses the commit message of the first commit here, rather than the PR title, which results in what is now a bit of a misleading commit message?

Yeah the CQ when used with commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. always takes the commit message from the first commit. IMO it would make sense if we added some check if the PR title doesn't match to ask what to do, we'd just need someone to volunteer to set it up.

MattIPv4 · 2024-01-24T17:43:59Z

Ah okay, TIL! Is the not-particularly-accurate commit message in main worth worrying about (might be confusing, esp. for the changelogs etc.)?

aduh95 · 2024-01-24T18:11:19Z

It's unfortunate, but we're way passed the 10 minute window, so the commit will stay as is. It might be worth opening a PR backporting it to 21.x-staging branch with the correct commit message (since that's the commits on that branch that actually end up on the changelog).

node/doc/contributing/collaborator-guide.md

Lines 775 to 777 in 62fc950

    
           messages. You are only allowed to force push to any Node.js branch within 10 
        
           minutes from your original push. If someone else pushes to the branch or the 
        
           10-minute period passes, consider the commit final.

MattIPv4 · 2024-01-24T18:24:25Z

Ack, no worries. I'll look at getting a backport up for 21.x (and ideally 20.x as well).

PR-URL: nodejs#51520 Fixes: nodejs#51518 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs#51520 Fixes: nodejs#51518 Backport-PR-URL: nodejs#51559 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

MattIPv4 · 2024-01-24T18:56:00Z

Would someone be able to add the backport open label for 21.x here, please? ❤️

aduh95 · 2024-01-24T21:01:46Z

(and ideally 20.x as well)

It might not be necessary, IIRC releasers will cherry-pick all the commits from the 21.x branch to the 20.x-staging after the maturation period (unless the commit is linked to a PR with a dont-land-on-20.x or something like that). You could ask for guidance to whoever merges your 21.x backport, as having to deal with a backport PR is more work than simple cherry-picking (both for you and the releaser).

PR-URL: nodejs#51520 Fixes: nodejs#51518 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #51520 Fixes: #51518 Backport-PR-URL: #51559 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs#51520 Fixes: nodejs#51518 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #51520 Fixes: #51518 Backport-PR-URL: #51559 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

MattIPv4 · 2024-03-26T23:12:15Z

Filed some issues w/ browsers etc. for the same performance issue, as this fix seems to have worked for Node.js w/o issue (knock on wood):

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 Jan 19, 2024

This comment was marked as resolved.

Sign in to view

url: remove #context from URLSearchParams

2b2d11b

anonrig reviewed Jan 19, 2024

View reviewed changes

lib/internal/url.js Outdated Show resolved Hide resolved

MattIPv4 added 2 commits January 19, 2024 03:31

url: rely on URLSearchParams if in use for search

129a1a9

url: update URLSearchParams directly from URL search setter

c2e8526

MattIPv4 force-pushed the url-searchparams-perf branch 2 times, most recently from 3c5cb28 to aadda98 Compare January 19, 2024 03:38

MattIPv4 added 4 commits January 19, 2024 04:14

url: create href using searchParams without bindingUrl call

039380e

url: ensure ? is present when relying on URLSearchParams

e015de2

url: don't use public getter for toString/toJSON

2487726

This appears to keep the "calling stringifier with this = {} didn't throw TypeError" test happy

url: update searchParams tests to expect encoded results

b0270bc

MattIPv4 force-pushed the url-searchparams-perf branch from 2d6f0e5 to b0270bc Compare January 19, 2024 04:24

anonrig self-requested a review January 19, 2024 05:05

url: only use URLSearchParams as store when modified

d9c5381

url: store search in URL on first access/update after URLSearchParams…

a05675c

… updates

MattIPv4 changed the title ~~url: use URLSearchParams to store search for URL if in use~~ url: don't update URL immediately on update to URLSearchParams Jan 19, 2024

MattIPv4 force-pushed the url-searchparams-perf branch 2 times, most recently from 863ab4d to 8c0a218 Compare January 19, 2024 08:24

MattIPv4 force-pushed the url-searchparams-perf branch 2 times, most recently from c69208c to 6876ecc Compare January 19, 2024 08:33

url: add benchmark for param append for URL and URLSearchParams

310d275

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2024

nodejs-github-bot merged commit 925a464 into nodejs:main Jan 24, 2024
57 checks passed

MattIPv4 deleted the url-searchparams-perf branch January 24, 2024 15:08

MattIPv4 mentioned this pull request Jan 24, 2024

[v21.x backport] url: don't update URL immediately on update to URLSearchParams #51559

Merged

anonrig added the backport-open-v21.x label Jan 24, 2024

targos added backported-to-v21.x and removed backport-open-v21.x labels Feb 15, 2024

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

MattIPv4 mentioned this pull request Mar 26, 2024

🐛 Bug Report — Runtime APIs: URL.searchParams performance issue cloudflare/workerd#1903

Open

RafaelGSS mentioned this pull request Apr 15, 2024

2024-04-24, Version 22.0.0 (Current) #52505

Merged

targos added the backported-to-v20.x PRs backported to the v20.x-staging branch. label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: don't update URL immediately on update to URLSearchParams #51520

url: don't update URL immediately on update to URLSearchParams #51520

MattIPv4 commented Jan 19, 2024 •

edited

Loading

nodejs-github-bot commented Jan 19, 2024

This comment was marked as resolved.

MattIPv4 commented Jan 19, 2024

anonrig commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024 •

edited

Loading

MattIPv4 commented Jan 19, 2024 •

edited

Loading

nodejs-github-bot commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Mar 26, 2024

url: don't update URL immediately on update to URLSearchParams #51520

url: don't update URL immediately on update to URLSearchParams #51520

Conversation

MattIPv4 commented Jan 19, 2024 • edited Loading

nodejs-github-bot commented Jan 19, 2024

This comment was marked as resolved.

MattIPv4 commented Jan 19, 2024

anonrig commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024

MattIPv4 commented Jan 19, 2024 • edited Loading

MattIPv4 commented Jan 19, 2024 • edited Loading

nodejs-github-bot commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

MattIPv4 commented Jan 24, 2024

aduh95 commented Jan 24, 2024

MattIPv4 commented Mar 26, 2024

MattIPv4 commented Jan 19, 2024 •

edited

Loading

MattIPv4 commented Jan 19, 2024 •

edited

Loading

MattIPv4 commented Jan 19, 2024 •

edited

Loading