url: improve invalid url performance

[anonrig]

Some easy wins for Invalid URL path.

                                                    confidence improvement accuracy (*)   (**)  (***)
url/whatwg-url-validity.js e=100000 type='invalid'        ***    120.33 %       ±2.06% ±2.74% ±3.56%
url/whatwg-url-validity.js e=100000 type='valid'                   1.35 %       ±2.21% ±2.96% ±3.91%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

cc @nodejs/url @nodejs/performance

[nodejs-github-bot]

Review requested:

• @nodejs/url

[anonrig]

@nodejs/tsc I've removed the error.input from URL error - ERR_INVALID_URL to make sure we share a similar output with Chrome and Safari. I've also added semver-major label because of the removal of error.input attribute.

Safari returns: TypeError: Type error
Chrome returns: VM78:1 Uncaught TypeError: Failed to construct 'URL': Invalid URL

None of them have the error.input attribute.

[aduh95]

I would keep the input, and even add the base, it can very hard to understand what is the error without seeing the input and base

[rluvaton]

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

[mcollina]

lgtm

[aduh95]

Could there be a way to get the same performance improvements without removing helpful debugging information? If not, maybe we could use a flag to opt-in to the slower option that outputs the input and base?

[anonrig]

Could there be a way to get the same performance improvements without removing helpful debugging information? If not, maybe we could use a flag to opt-in to the slower option that outputs the input and base?

Probably, although I'm not sure. This requires a similar approach to what ThrowAccessDenied is doing in https://github.com/nodejs/node/blob/main/src/permission/permission.cc#L102.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[tniessen]

FWIW, I tend to agree with @aduh95 that removing error information for the sake of performance (on an error path!) does not benefit users. I thought that URL.canParse() was specifically added to avoid having to rely on the performance of the URL error path.

[Uzlopak]

How much is actually the performance hit when keeping the input as an attribute?

[BridgeAR]

I do not think it's a good idea to remove important debug information in favor of performance. We can definitely improve the performance but debug information is more important. I had an open PR to improve the situation a lot and I have to get back to that again.

[anonrig]

@BridgeAR @aduh95 @tniessen I've updated the implementation. Previously this pull request was 142% faster than main. Right now it is 120% faster. I also added base field to the error object.

Appreciate it, if you can review it once again.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[anonrig]

Hey @BridgeAR, can you re-review this pull request, and remove your block?

[benjamingr]

I suspect he would still object since the URL isn't part of the message string, what's the cost of adding that back?

[anonrig]

I suspect he would still object since the URL isn't part of the message string, what's the cost of adding that back?

I don't follow. URL was never part of the message string? Input is a property of the error object. On top of that I've also added base property to the error as well.

[benjamingr]

@anonrig ah, I'm probably mixing it up with another PR then, lgtm

[anonrig]

Adding to @nodejs/tsc agenda to resolve the block.

[targos]

There's no need for the TSC to intervene if you addressed the blocking comment

The comment is addressed.

[nodejs-github-bot]

Landed in c829c03