Skip to content

url: improve canParse() performance for non-onebyte strings - #58023

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/improve-perf-url-canparse
Apr 27, 2025
Merged

url: improve canParse() performance for non-onebyte strings#58023
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/improve-perf-url-canparse

Conversation

@anonrig

@anonriganonrig commented Apr 25, 2025

Copy link
Copy Markdown
Member

V8 Fast API now supports v8::Localv8::Value as a parameter type and we can access isolate and context through the fast api. Let's update our implementation to reflect the new limitations.

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

 confidence improvement accuracy (*) (**) (***)
url/whatwg-url-canParse.js n=1000000 type='auth' *** 5.55 % ±2.05% ±2.75% ±3.61%
url/whatwg-url-canParse.js n=1000000 type='dot' *** 3.90 % ±0.69% ±0.92% ±1.20%
url/whatwg-url-canParse.js n=1000000 type='file' 1.13 % ±1.20% ±1.60% ±2.09%
url/whatwg-url-canParse.js n=1000000 type='idn' *** 2.78 % ±0.31% ±0.42% ±0.55%
url/whatwg-url-canParse.js n=1000000 type='javascript' *** 1.85 % ±0.91% ±1.22% ±1.61%
url/whatwg-url-canParse.js n=1000000 type='long' *** 9.09 % ±0.65% ±0.87% ±1.13%
url/whatwg-url-canParse.js n=1000000 type='percent' *** 1.81 % ±0.31% ±0.41% ±0.53%
url/whatwg-url-canParse.js n=1000000 type='short' *** 2.23 % ±0.90% ±1.20% ±1.58%
url/whatwg-url-canParse.js n=1000000 type='ws' *** 7.30 % ±1.65% ±2.20% ±2.87%

cc @nodejs/performance

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Apr 25, 2025
@anonrig
anonrig requested a review from mcollinaApril 25, 2025 14:48
@anonrig
anonrigforce-pushed the yagiz/improve-perf-url-canparse branch from d9dca31 to cdd187cCompareApril 25, 2025 14:49
@codecov

codecovBot commented Apr 25, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.26%. Comparing base (9cc0195) to head (375bc78).
Report is 121 commits behind head on main.

Files with missing linesPatch %Lines
src/node_url.cc72.72%3 Missing and 3 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #58023 +/- ##
==========================================
- Coverage 90.28% 90.26% -0.02% 
==========================================
Files 630 630 Lines 186158 186347 +189 Branches 36484 36499 +15 ==========================================
+ Hits 168067 168212 +145 - Misses 10974 11007 +33 - Partials 7117 7128 +11 
Files with missing linesCoverage Δ
src/node_external_reference.h100.00% <ø> (ø)
src/node_url.h100.00% <ø> (ø)
src/node_url.cc78.72% <72.72%> (+0.88%)⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anonriganonrig 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. labels Apr 25, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig
anonrigforce-pushed the yagiz/improve-perf-url-canparse branch from cdd187c to ffba018CompareApril 26, 2025 21:13
@anonrig
anonrigforce-pushed the yagiz/improve-perf-url-canparse branch from ffba018 to 375bc78CompareApril 26, 2025 21:15
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig
anonrig requested a review from H4adApril 27, 2025 00:11
@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-bot
nodejs-github-bot merged commit e0cf8ae into nodejs:mainApr 27, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in e0cf8ae

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #58023
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #58023
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 6, 2025
@aduh95

Copy link
Copy Markdown
Contributor

Build fails on v22.x-staging with this change

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@anonrig@nodejs-github-bot@aduh95@lemire@jasnell@H4ad@gurgunday