lib,src: replace toUSVString with `toWellFormed()` by anonrig · Pull Request #47342 · nodejs/node · GitHub

Fixes #47328, depends on V8 11.3

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1315/

url/url-searchparams-creation.js n=1000000 inputType='iterable' type='array'                                           ***     13.84 %       ±2.92%  ±3.89%  ±5.06%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='encodelast'                                      ***     37.48 %       ±3.82%  ±5.10%  ±6.68%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='encodemany'                                      ***     40.92 %       ±3.38%  ±4.52%  ±5.93%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='multiprimitives'                                 ***     34.02 %       ±3.20%  ±4.26%  ±5.54%
url/url-searchparams-creation.js n=1000000 inputType='iterable' type='noencode'                                        ***     35.28 %       ±3.22%  ±4.28%  ±5.59%
url/url-searchparams-creation.js n=1000000 inputType='object' type='array'                                              **      5.75 %       ±3.45%  ±4.61%  ±6.05%
url/url-searchparams-creation.js n=1000000 inputType='object' type='encodelast'                                        ***     17.50 %       ±2.83%  ±3.77%  ±4.91%
url/url-searchparams-creation.js n=1000000 inputType='object' type='encodemany'                                        ***     16.51 %       ±3.34%  ±4.44%  ±5.78%
url/url-searchparams-creation.js n=1000000 inputType='object' type='multiprimitives'                                   ***     17.07 %       ±2.93%  ±3.90%  ±5.08%
url/url-searchparams-creation.js n=1000000 inputType='object' type='noencode'                                          ***     13.41 %       ±3.73%  ±4.99%  ±6.54%
url/url-searchparams-creation.js n=1000000 inputType='string' type='array'                                             ***      6.40 %       ±3.43%  ±4.57%  ±5.95%
url/url-searchparams-creation.js n=1000000 inputType='string' type='encodelast'                                        ***      7.34 %       ±3.07%  ±4.09%  ±5.32%
url/url-searchparams-creation.js n=1000000 inputType='string' type='encodemany'                                        ***      7.22 %       ±3.63%  ±4.83%  ±6.28%
url/url-searchparams-creation.js n=1000000 inputType='string' type='multiprimitives'                                   ***      8.62 %       ±3.06%  ±4.07%  ±5.30%
url/url-searchparams-creation.js n=1000000 inputType='string' type='noencode'                                          ***      7.33 %       ±3.08%  ±4.10%  ±5.34%
url/url-searchparams-iteration.js n=1000000 loopMethod='forEach'                                                               -2.47 %       ±6.73%  ±8.97% ±11.71%
url/url-searchparams-iteration.js n=1000000 loopMethod='iterator'                                                               0.13 %       ±3.97%  ±5.29%  ±6.88%
url/url-searchparams-read.js n=20000000 param='nonexistent' accessMethod='get'                                         ***     40.19 %       ±3.18%  ±4.25%  ±5.56%
url/url-searchparams-read.js n=20000000 param='nonexistent' accessMethod='getAll'                                      ***     49.89 %       ±3.43%  ±4.57%  ±5.97%
url/url-searchparams-read.js n=20000000 param='nonexistent' accessMethod='has'                                         ***     60.54 %       ±3.37%  ±4.51%  ±5.93%
url/url-searchparams-read.js n=20000000 param='one' accessMethod='get'                                                 ***     78.13 %       ±3.68%  ±4.93%  ±6.51%
url/url-searchparams-read.js n=20000000 param='one' accessMethod='getAll'                                              ***     23.62 %       ±2.74%  ±3.64%  ±4.74%
url/url-searchparams-read.js n=20000000 param='one' accessMethod='has'                                                 ***     84.82 %       ±4.26%  ±5.71%  ±7.50%
url/url-searchparams-read.js n=20000000 param='three' accessMethod='get'                                               ***     72.78 %       ±4.67%  ±6.21%  ±8.08%
url/url-searchparams-read.js n=20000000 param='three' accessMethod='getAll'                                            ***     25.17 %       ±2.42%  ±3.23%  ±4.23%
url/url-searchparams-read.js n=20000000 param='three' accessMethod='has'                                               ***     70.79 %       ±4.05%  ±5.43%  ±7.16%
url/url-searchparams-read.js n=20000000 param='two' accessMethod='get'                                                 ***     65.16 %       ±4.96%  ±6.61%  ±8.63%
url/url-searchparams-read.js n=20000000 param='two' accessMethod='getAll'                                              ***     26.08 %       ±3.88%  ±5.22%  ±6.89%
url/url-searchparams-read.js n=20000000 param='two' accessMethod='has'                                                 ***     60.99 %       ±2.14%  ±2.85%  ±3.72%

Review requested:

@nodejs/url

Let's wait for #47340 to land to re-run the benchmark.

I've re-run the benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1315/

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

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

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

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

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

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

Rebased and force pushed because of the flaky test.

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

I noticed that f548b82 is split into a separate commit and the PR was labeled as commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. . In this case, the first commit (7ec020f) would fail for test/benchmark/test-benchmark-url.js because require(internal/url).toUSVString was removed.

@anonrig would you mind removing the label commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. ?

@legendecas done. can you review and add commit-queue?

Commit Queue failed

- Loading data for nodejs/node/pull/47342
✔  Done loading data for nodejs/node/pull/47342
----------------------------------- PR info ------------------------------------
Title      lib,src: replace toUSVString with `toWellFormed()` (#47342)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:to-usv-string-to-well-formed -> nodejs:main
Labels     semver-minor, lib / src, needs-ci, review wanted, commit-queue-squash, dont-land-on-v18.x
Commits    3
 - lib,src: replace toUSVString with `toWellFormed()`
 - benchmark: remove toUSVString benchmarks
 - fixup! lib,src: replace toUSVString with `toWellFormed()`
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/47342
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
Reviewed-By: Chengzhong Wu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47342
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
Reviewed-By: Chengzhong Wu 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - lib,src: replace toUSVString with `toWellFormed()`
   ⚠  - benchmark: remove toUSVString benchmarks
   ⚠  - fixup! lib,src: replace toUSVString with `toWellFormed()`
   ℹ  This PR was created on Fri, 31 Mar 2023 14:47:37 GMT
   ✔  Approvals: 4
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/47342#pullrequestreview-1367094432
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/47342#pullrequestreview-1367545616
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/47342#pullrequestreview-1746692565
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/47342#pullrequestreview-1749539821
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2023-04-02T23:02:06Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1315/
   ℹ  Last Full PR CI on 2023-11-29T03:05:04Z: https://ci.nodejs.org/job/node-test-pull-request/55995/
- Querying data for job/node-test-pull-request/55995/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/7032789935

Landed in 01dae5f

anonrig added dont-land-on-v14.x labels Mar 31, 2023

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 31, 2023

anonrig force-pushed the to-usv-string-to-well-formed branch 5 times, most recently from cd7df60 to 80a3312 Compare March 31, 2023 14:57

anonrig added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Mar 31, 2023

This comment was marked as resolved.

Sign in to view

anonrig requested review from jasnell and mscdex March 31, 2023 18:44

jasnell approved these changes Mar 31, 2023

View reviewed changes

anonrig force-pushed the to-usv-string-to-well-formed branch from b8411ac to 357ef37 Compare April 1, 2023 16:26

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. review wanted PRs that need reviews. labels Apr 1, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2023

anonrig requested review from mcollina and targos April 1, 2023 21:43

github-actions Bot mentioned this pull request Apr 2, 2023

CI Reliability 2023-04-02 nodejs/reliability#537

Open

30 tasks

ljharb approved these changes Nov 27, 2023

View reviewed changes

legendecas approved these changes Nov 27, 2023

View reviewed changes

anonrig added 3 commits November 28, 2023 22:02

lib,src: replace toUSVString with toWellFormed()

7ec020f

benchmark: remove toUSVString benchmarks

f548b82

fixup! lib,src: replace toUSVString with toWellFormed()

256ec34

anonrig force-pushed the to-usv-string-to-well-formed branch from f69ddee to 256ec34 Compare November 29, 2023 03:02

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2023

anonrig added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 29, 2023

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 29, 2023

legendecas approved these changes Nov 29, 2023

View reviewed changes

legendecas added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 29, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2023

nodejs-github-bot merged commit 01dae5f into nodejs:main Nov 29, 2023

targos mentioned this pull request Dec 4, 2023

v21.4.0 release proposal #51043

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

lib,src: replace toUSVString with `toWellFormed()` by anonrig · Pull Request #47342 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib,src: replace toUSVString with toWellFormed() - #47342

lib,src: replace toUSVString with toWellFormed()#47342nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom anonrig:to-usv-string-to-well-formedanonrig/node:to-usv-string-to-well-formedCopy head branch name to clipboard

Conversation

anonrig commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 31, 2023

Uh oh!

This comment was marked as resolved.

anonrig commented Mar 31, 2023

Uh oh!

anonrig commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 1, 2023

Uh oh!

nodejs-github-bot commented Apr 2, 2023

Uh oh!

nodejs-github-bot commented Nov 26, 2023

Uh oh!

nodejs-github-bot commented Nov 26, 2023

Uh oh!

nodejs-github-bot commented Nov 27, 2023

Uh oh!

nodejs-github-bot commented Nov 28, 2023

Uh oh!

anonrig commented Nov 29, 2023

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

legendecas commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig commented Nov 29, 2023

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

lib,src: replace toUSVString with toWellFormed()#47342
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
anonrig:to-usv-string-to-well-formed

anonrig commented Mar 31, 2023 •
edited
Loading

anonrig commented Apr 1, 2023 •
edited
Loading

legendecas commented Nov 29, 2023 •
edited
Loading