url: refactor isIpv6Hostname function to use string methods

[rayark1]

Refactor the isIpv6Hostname function to improve readability and performance

[nodejs-github-bot]

Review requested:

• @nodejs/url

[avivkeller]

What about the primordials? Are they not important?

CC @nodejs/primordials

[ljharb]

they definitely are

[Uzlopak]

How Is using startsWith and endsWith can be considered being faster as it claims that it has even better performance?

[avivkeller]

Can you provide a benchmark to show the performance improvement?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (cc26951) to head (6a536a0).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54486      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182544   182537       -7     
  Branches    35030    35032       +2     
==========================================
- Hits       159445   159425      -20     
- Misses      16372    16375       +3     
- Partials     6727     6737      +10     

Files	Coverage Δ
lib/url.js	97.87% <100.00%> (-0.02%)	⬇️

... and 18 files with indirect coverage changes

[ronag]

This is just going back to how it was before. I don't think we should be going back and forth between the same things.

[lemire]

improve readability and performance

Do we have benchmarks?

[Uzlopak]

How Is using startsWith and endsWith can be considered being faster as it claims that it has even better performance?

I doubt that this has any performance gains. More likeliy performance regressions.

Should be rejected.

[Uzlopak]

For completeness:
I benchmarked it with this scirpt

'use strict';
const common = require('../common.js');
const { isIpv6Hostname } = require('url');

const bench = common.createBenchmark(main, {
  value: [
    '[::1]',
    '127.0.0.1',
    'example.org'
  ],
  n: [1e6],
});

function main({ n, value }) {
  
  bench.start();
  for (let i = 0; i < n; ++i)
    isIpv6Hostname(value);
  bench.end(n);
}

and i exposed is isIpv6Hostname in url.js

This PR:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node benchmark/url/url-isIpv6hostname.js 
url/url-isIpv6hostname.js n=1000000 value="[::1]": 129,560,278.89145634
url/url-isIpv6hostname.js n=1000000 value="127.0.0.1": 194,712,162.8510222
url/url-isIpv6hostname.js n=1000000 value="example.org": 198,888,017.09641397

Main:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node benchmark/url/url-isIpv6hostname.js 
url/url-isIpv6hostname.js n=1000000 value="[::1]": 290,188,915.88613105
url/url-isIpv6hostname.js n=1000000 value="127.0.0.1": 216,782,851.43589374
url/url-isIpv6hostname.js n=1000000 value="example.org": 205,617,088.7539995

So in case that it is encapsulated in brackets we lose about 50% performance and in other cases we lose few percents because of startsWith is slower than directly checking the first character.

THe fastest implementation would be by avoiding .:

function isIpv6Hostname(hostname) {
  return (
    hostname[0] === CHAR_LEFT_SQUARE_BRACKET &&
    hostname[hostname.length - 1] ===
    CHAR_RIGHT_SQUARE_BRACKET
  );
}

with:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node benchmark/url/url-isIpv6hostname.js 
url/url-isIpv6hostname.js n=1000000 value="[::1]": 387,195,446.5815482
url/url-isIpv6hostname.js n=1000000 value="127.0.0.1": 305,417,184.60330886
url/url-isIpv6hostname.js n=1000000 value="example.org": 361,797,773.1347063

Is the need for primordials really needed for an internal function where it is ensured, that we only pass strings?

[ljharb]

@Uzlopak especially for an internal function, in which it could easily be a security issue to provide a hook point for user code - even if it's only ever passed strings (which could also change in the future) the string methods are globally replaceable.

[Uzlopak]

To be honest... i looked into the code and i had to laugh.