url: improve invalid url performance by anonrig · Pull Request #49692 · nodejs/node · GitHub

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

Review requested:

@nodejs/url

@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.

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

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

lgtm

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?

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.

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

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

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.

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

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.

@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.

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

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

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

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

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

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

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

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

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

The comment is addressed.

Landed in c829c03

PR-URL: #49692 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#49692 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

anonrig added performance Issues and PRs related to the performance of Node.js. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Sep 17, 2023

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 17, 2023

anonrig force-pushed the improve-invalid-url-performance branch from 0501c68 to 79415ed Compare September 17, 2023 21:36

anonrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 17, 2023

Uzlopak reviewed Sep 17, 2023

View reviewed changes

Comment thread lib/internal/errors.js Outdated

rluvaton added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 18, 2023

mcollina approved these changes Sep 18, 2023

View reviewed changes

ronag approved these changes Sep 18, 2023

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

BridgeAR previously requested changes Sep 19, 2023

View reviewed changes

anonrig force-pushed the improve-invalid-url-performance branch from 147dd75 to a9c408a Compare September 19, 2023 22:30

anonrig force-pushed the improve-invalid-url-performance branch from a9c408a to 29267d9 Compare September 19, 2023 22:32

anonrig requested a review from BridgeAR September 19, 2023 22:33

url: improve invalid url performance

c59b136

anonrig force-pushed the improve-invalid-url-performance branch from 29267d9 to c59b136 Compare September 19, 2023 22:38

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023

github-actions Bot mentioned this pull request Sep 21, 2023

CI Reliability 2023-09-21 nodejs/reliability#672

Open

22 tasks

github-actions Bot mentioned this pull request Sep 22, 2023

CI Reliability 2023-09-22 nodejs/reliability#673

Open

17 tasks

github-actions Bot mentioned this pull request Sep 23, 2023

CI Reliability 2023-09-23 nodejs/reliability#674

Open

16 tasks

benjamingr approved these changes Sep 23, 2023

View reviewed changes

github-actions Bot mentioned this pull request Sep 24, 2023

CI Reliability 2023-09-24 nodejs/reliability#675

Open

20 tasks

anonrig added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 26, 2023

anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Sep 26, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 26, 2023

nodejs-github-bot merged commit c829c03 into nodejs:main Sep 26, 2023

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

url: improve invalid url performance

7d91a73

PR-URL: #49692 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

ruyadorno mentioned this pull request Sep 28, 2023

v20.7.1 proposal #49917

Closed

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

url: improve invalid url performance

097b598

PR-URL: #49692 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

ruyadorno mentioned this pull request Sep 28, 2023

v20.8.0 proposal #49932

Merged

coderabbitai Bot mentioned this pull request Aug 5, 2026

feat(clickhouse): wire the statement limiter, one client, one door langwatch/langwatch#6614

Merged

url: improve invalid url performance by anonrig · Pull Request #49692 · nodejs/node · GitHub

Uh oh!

Uh oh!

url: improve invalid url performance - #49692

url: improve invalid url performance#49692nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom anonrig:improve-invalid-url-performanceanonrig/node:improve-invalid-url-performanceCopy head branch name to clipboard

Conversation

anonrig commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 17, 2023

Uh oh!

anonrig commented Sep 17, 2023

Uh oh!

aduh95 commented Sep 17, 2023

Uh oh!

Uh oh!

rluvaton commented Sep 18, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Sep 18, 2023

Uh oh!

anonrig commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

tniessen commented Sep 19, 2023

Uh oh!

Uzlopak commented Sep 19, 2023

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

anonrig commented Sep 19, 2023

Uh oh!

nodejs-github-bot commented Sep 20, 2023

Uh oh!

nodejs-github-bot commented Sep 20, 2023

Uh oh!

nodejs-github-bot commented Sep 20, 2023

Uh oh!

nodejs-github-bot commented Sep 21, 2023

Uh oh!

anonrig commented Sep 22, 2023

Uh oh!

benjamingr commented Sep 23, 2023

Uh oh!

anonrig commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented Sep 23, 2023

Uh oh!

anonrig commented Sep 26, 2023

Uh oh!

targos commented Sep 26, 2023

Uh oh!

nodejs-github-bot commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

url: improve invalid url performance#49692
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:improve-invalid-url-performance

anonrig commented Sep 17, 2023 •
edited
Loading

anonrig commented Sep 23, 2023 •
edited
Loading