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

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

Merged
merged 3 commits into from
Nov 29, 2023
Merged

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

merged 3 commits into from
Nov 29, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 31, 2023 •
edited
Loading

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%

@anonrig anonrig added dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Mar 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot 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 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 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
@github-actions

This comment was marked as resolved.

Sign in to view
@anonrig anonrig requested review from jasnell and mscdex March 31, 2023 18:44
@anonrig
Copy link
Member Author

anonrig commented Mar 31, 2023

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

@anonrig
Copy link
Member Author

anonrig commented Apr 1, 2023 •
edited
Loading

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

@anonrig anonrig force-pushed the to-usv-string-to-well-formed branch from b8411ac to 357ef37 Compare April 1, 2023 16:26
@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig requested review from mcollina and targos April 1, 2023 21:43
@github-actions github-actions bot mentioned this pull request Apr 2, 2023
Open
30 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

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

anonrig commented Nov 29, 2023

Rebased and force pushed because of the flaky test.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member

legendecas commented Nov 29, 2023 •
edited
Loading

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. ?

@anonrig 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
@anonrig
Copy link
Member Author

anonrig commented Nov 29, 2023

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

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2023
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas 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 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 nodejs-github-bot merged commit 01dae5f into nodejs:main Nov 29, 2023
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 01dae5f

targos pushed a commit that referenced this pull request Dec 4, 2023
@anonrig @targos
lib,src: replace toUSVString with toWellFormed()
246cf73 
PR-URL: #47342
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targos targos removed the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 4, 2023
@targos targos mentioned this pull request Dec 4, 2023
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@anonrig @richardlau
lib,src: replace toUSVString with toWellFormed()
8a08275 
PR-URL: #47342
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@ljharb ljharb ljharb approved these changes

@mscdex mscdex Awaiting requested review from mscdex

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. review wanted PRs that need reviews.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

large performance regressions since f51c152