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.

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

benchmark: differentiate whatwg and legacy url #47377

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 3, 2023

After merging Ada pull request, Ada will be 3x faster than the legacy url.parse method, and we don't even need to run the legacy benchmarks most of the time. This pull request splits the benchmarks into multiple benchmarks.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. url Issues and PRs related to the legacy built-in url module. labels Apr 3, 2023
@richardlau
Copy link
Member

After merging Ada pull request legacy url.parse method will be 3x slower.

Huh? That sounds like a good reason not to backport Ada to Node.js 18 if it's going to cause a regression.
cc @nodejs/releasers @nodejs/lts

@anonrig
Copy link
Member Author

anonrig commented Apr 3, 2023

After merging Ada pull request legacy url.parse method will be 3x slower.

Huh? That sounds like a good reason not to backport Ada to Node.js 18 if it's going to cause a regression.

cc @nodejs/releasers @nodejs/lts

I mean Ada is faster than url.parse by 3x. Bad way to rephrase it, my bad!

@anonrig
Copy link
Member Author

anonrig commented Apr 3, 2023

cc @nodejs/url @nodejs/performance appreciate reviews

@anonrig anonrig requested review from jasnell and TimothyGu April 6, 2023 13:35
@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2023
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

I'm ok with this as long as the results between the new legacy-only and whatwg-only benchmarks are still comparable – which they seem to be.

benchmark/url/legacy-url-parse.js Outdated Show resolved Hide resolved
benchmark/url/legacy-url-parse.js Outdated Show resolved Hide resolved
benchmark/url/legacy-url-get-prop.js Outdated Show resolved Hide resolved
@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47377
✔  Done loading data for nodejs/node/pull/47377
----------------------------------- PR info ------------------------------------
Title      benchmark: differentiate whatwg and legacy url (#47377)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:benchmark-urls -> nodejs:main
Labels     url, benchmark, author ready, commit-queue-squash
Commits    2
 - benchmark: differentiate whatwg and legacy url
 - fixup! benchmark: differentiate whatwg and legacy url
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/47377
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Tiancheng "Timothy" Gu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47377
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Tiancheng "Timothy" Gu 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! benchmark: differentiate whatwg and legacy url
   ℹ  This PR was created on Mon, 03 Apr 2023 02:09:09 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/47377#pullrequestreview-1374972136
   ✔  - Tiancheng "Timothy" Gu (@TimothyGu) (TSC): https://github.com/nodejs/node/pull/47377#pullrequestreview-1375735658
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4693909020

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 13, 2023
@anonrig anonrig requested review from benjamingr and TimothyGu April 13, 2023 21:21
@anonrig
Copy link
Member Author

anonrig commented Apr 13, 2023

@nodejs/performance can you review?

@anonrig anonrig 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 Apr 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit cd0fcf2 into nodejs:main Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in cd0fcf2

@anonrig anonrig deleted the benchmark-urls branch April 13, 2023 21:38
targos pushed a commit that referenced this pull request May 2, 2023
PR-URL: #47377
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@targos targos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47377
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@MoLow MoLow 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 Jul 6, 2023
@MoLow
Copy link
Member

MoLow commented Jul 6, 2023

depends on #47351 and #47179
CC @danielleadams

targos pushed a commit that referenced this pull request Nov 10, 2023
PR-URL: #47377
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#47377
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#47377
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. benchmark Issues and PRs related to the benchmark subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants