Skip to content

Conversation

@gurgunday
Copy link
Member

branch:

node % ./node benchmark/run.js --filter whatwg-url-to-and-from-path url
url/whatwg-url-to-and-from-path.js
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null" method="fileURLToPath": 3,853,044.868707496
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool" method="fileURLToPath": 3,490,012.0935340663
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool#hash" method="fileURLToPath": 3,167,181.914377771

main:

node % ./node benchmark/run.js --filter whatwg-url-to-and-from-path url
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null" method="fileURLToPath": 2,034,421.4266934511
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool" method="fileURLToPath": 2,062,544.6025270298
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool#hash" method="fileURLToPath": 2,022,311.8969784945

The majority of file paths won't need decoding, so this PR avoids calling decodeURIComponent when the path is pure ASCII with no encodings

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot 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 Nov 16, 2025
@gurgunday gurgunday added the performance Issues and PRs related to the performance of Node.js. label Nov 16, 2025
@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (2856475) to head (d756a3c).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/url.js 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60749      +/-   ##
==========================================
+ Coverage   88.54%   88.55%   +0.01%     
==========================================
  Files         703      703              
  Lines      208222   208256      +34     
  Branches    40140    40152      +12     
==========================================
+ Hits       184360   184420      +60     
+ Misses      15882    15836      -46     
- Partials     7980     8000      +20     
Files with missing lines Coverage Δ
lib/internal/url.js 93.59% <50.00%> (+0.24%) ⬆️

... and 41 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.

pathname = SideEffectFreeRegExpPrototypeSymbolReplace(FORWARD_SLASH, pathname, '\\');
pathname = decodeURIComponent(pathname);
// Fast-path: if there is no percent-encoding, avoid decodeURIComponent.
if (StringPrototypeIndexOf(pathname, '%') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why indexOf?

Suggested change
if (StringPrototypeIndexOf(pathname, '%') !== -1) {
if (StringPrototypeIncludes(pathname, '%')) {

Copy link
Member Author

@gurgunday gurgunday Nov 16, 2025

Choose a reason for hiding this comment

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

It's a convention from Fastify, we prefered indexOf as it was faster until node 22. The difference was even bigger in earlier node versions.

For instance, in node 20:

Starting benchmark...
Searching for "lazy dog" in "The quick brown fox jumps over the lazy dog. This is a long sentence used for benchmarking string search methods."

String#includes x 952,187,610 ops/sec ±0.81% (98 runs sampled)
String#indexOf x 960,298,473 ops/sec ±0.16% (102 runs sampled)

-----------------------------------
Fastest is String#indexOf

benchmark:

const Benchmark = require('benchmark');

// --- Setup ---
const suite = new Benchmark.Suite;

// The string to search within
const text = 'The quick brown fox jumps over the lazy dog. This is a long sentence used for benchmarking string search methods.';

// The substring to find
const substring = 'lazy dog';

// --- Benchmark Suite ---
console.log('Starting benchmark...');
console.log(`Searching for "${substring}" in "${text}"\n`);

suite
  .add('String#includes', function() {
    text.includes(substring);
  })
  .add('String#indexOf', function() {
    text.indexOf(substring) !== -1;
  })
  .on('cycle', function(event) {
    console.log(String(event.target));
  })
  .on('complete', function() {
    console.log('\n-----------------------------------');
    console.log('Fastest is ' + this.filter('fastest').map('name'));
    console.log('-----------------------------------');
  })
  .run();

Copy link
Member Author

Choose a reason for hiding this comment

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

I can use includes though if you're fine with the small difference for node 20

Copy link
Member Author

@gurgunday gurgunday Nov 16, 2025

Choose a reason for hiding this comment

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

In latest V8 it seems to have gone away though, so I changed to includes:

url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null" method="fileURLToPath": 3,903,087.8889524657
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool" method="fileURLToPath": 3,469,377.354475161
url/whatwg-url-to-and-from-path.js n=5000000 input="file:///dev/null?key=param&bool#hash" method="fileURLToPath": 3,498,714.659701892

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

Labels

needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants