deps: replace url parser with Ada by anonrig · Pull Request #46410 · nodejs/node · GitHub

This work was done in collaboration with me, @miguelteixeiraa and @lemire. I would also like to thank @addaleax and @ronag for their help.

This pull request replaces the existing URL parser with Ada, a fast spec-compliant URL parser written from scratch using modern C++ focused on performance.

A little bit about Ada:

100% spec compliant (even has its own WPT updater, which led us to find a couple of errors in WPT)
Fully tested (using both web platform tests, custom tests not covered by WPT) on Big Endian Systems (s390x), Ubuntu 20.04, Ubuntu 22.04 (with G++ and Clang++), Windows VS 2022 (with and without ClangCL)
Well documented and available for the public at https://ada-url.github.io/ada
On typical URLs, we use between 50 and 60 instructors per input byte for fully parsing and validating a URL string into a URL data structure.

The possibilities with this pull request:

Soon: Dropping ICU as a dependency of Ada will allow Node.js releases that do not have ICU to parse URL hosts correctly.

Side Note: Current benchmarks show up to 87% faster URL parsing, with similar but sometimes faster execution speeds compared to url.parse.

Fixes #46332
Fixes #46063
Fixes #30334
Fixes #44476
Fixes nodejs/performance#33
Closes #41220

Review requested:

@nodejs/crypto
@nodejs/gyp
@nodejs/loaders
@nodejs/modules
@nodejs/node-api
@nodejs/tsc
@nodejs/vm

Something that I think should be experimented upon is to avoid copying all data back-and-forth every time a field is updated.

Maybe it might be better to fetch them from C++ every time they are accessed, or possibly use tricks to use the V8 fast API.

Deprecating url.parse

I think that has always been possible, with or without Ada? It's kept for compatibility reasons, not performance.

Removes the need to update and conform to URL WPT

We probably still want to run the JS tests to make sure that the JS glues work properly

We probably still want to run the JS tests to make sure that the JS glues work properly

You're right. This pull request does not remove them.

test/sequential/test-inspector.js test is failing. Appreciate any help in pinpointing why it causes timeouts. cc @targos @Trott @addaleax (according to git blame, you were the only ones who worked on this file in the last 3 years)

@anonrig it might very well be related to parsing the debugger websocket url

I see right now Ada has Windows-specific IDNA handling using IdnToAscii. I'm a little concerned about this, as it would introduce platform-dependent URL parsing results. I also doubt Windows's IdnToAscii implements UTS46 fully.

I see plans to switch to an internal implementation (ada-url/ada#89) which would alleviate my concern. Do note though, that there are (still) outstanding issues with UTS46 on the spec side (see whatwg/url#744).

(Context: I helped create the previous WHATWG URL parser, and also maintain https://github.com/jsdom/tr46.)

I see right now Ada has Windows-specific IDNA handling using IdnToAscii.

When ICU is unavailable and we are under Windows, then ada falls back on Windows functions. That is correct.

Wherever we are, if ICU is available, we rely on ICU.

Landed in 4c08c20...c4581ab

PR-URL: #46410 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

@nodejs/lts I've put baking-for-lts PRs that need to wait before landing in a LTS release. on this -- I feel we should err on the side of caution and allow more time for this in current releases before considering for LTS.

Refs: nodejs#46410

PR-URL: #46410 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

@nodejs/lts I've put baking-for-lts PRs that need to wait before landing in a LTS release. on this -- I feel we should err on the side of caution and allow more time for this in current releases before considering for LTS.

@richardlau Should we remove the backing-for-lts label? In total, there were 2 issues reported and all of them is fixed in 19.8.0.

@nodejs/lts I've put baking-for-lts PRs that need to wait before landing in a LTS release. on this -- I feel we should err on the side of caution and allow more time for this in current releases before considering for LTS.

@richardlau Should we remove the backing-for-lts label? In total, there were 2 issues reported and all of them is fixed in 19.8.0.

@nodejs/lts thoughts?

I'd be ok for a backport to active v18.x, not earlier.

@anonrig this will need a backport to v18.x.

nodejs-github-bot added 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 Jan 29, 2023

anonrig force-pushed the deps/ada branch 9 times, most recently from 8f4d1a4 to 4a68c84 Compare January 30, 2023 02:37

mcollina reviewed Jan 30, 2023

View reviewed changes

anonrig force-pushed the deps/ada branch 3 times, most recently from 21adb01 to 6f070dc Compare January 30, 2023 17:28

anonrig mentioned this pull request Jan 30, 2023

Add Ada to Node.js ada-url/ada#142

Closed

anonrig force-pushed the deps/ada branch 2 times, most recently from 5e1e1ac to a1c7401 Compare January 30, 2023 20:35

anonrig force-pushed the deps/ada branch 5 times, most recently from eb45d67 to 17d3ed7 Compare February 1, 2023 19:14

nodejs-github-bot closed this Feb 6, 2023

richardlau added the baking-for-lts PRs that need to wait before landing in a LTS release. label Feb 6, 2023

github-actions Bot mentioned this pull request Feb 7, 2023

CI Reliability 2023-02-07 nodejs/reliability#498

Open

18 tasks

tniessen added a commit to tniessen/node that referenced this pull request Feb 7, 2023

src: fix array overrun in node::url::SetArgs()

c405efd

Refs: nodejs#46410

tniessen mentioned this pull request Feb 7, 2023

src: fix array overrun in node::url::SetArgs() #46541

Closed

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

v1gnesh mentioned this pull request Feb 20, 2023

Use ada url parser aio-libs/aiohttp#7217

Closed

zloirock mentioned this pull request Feb 21, 2023

URL issues #46755

Closed

asamuzaK mentioned this pull request Feb 26, 2023

Quotes in the URL are not percent encoded #46850

Closed

juanarbol mentioned this pull request Mar 1, 2023

src: use string_view for report and related code #46723

Merged

styfle mentioned this pull request Mar 2, 2023

Deprecate url.parse API in favor of URL constructor watson/original-url#11

Open

This was referenced Apr 3, 2023

deps: update ada to v1.0.1 #46550

Closed

url: fix url spec compliance issues #46547

Closed

deps: update ada to 1.0.3 #46784

Merged

deps: update ada to 1.0.4 #46853

Merged

anonrig mentioned this pull request Apr 5, 2023

[v18.x backport] replace url parser with ada #47435

Closed

This was referenced Apr 11, 2023

meta: update CODEOWNERS of url implementations #46775

Merged

v18.16.0 release proposal #47502

Merged

RaisinTen mentioned this pull request May 6, 2023

Ada's license #47894

Closed

pluris mentioned this pull request Aug 10, 2023

src: add a condition if the argument of DomainToUnicode is empty #49097

Merged

pluris mentioned this pull request Oct 18, 2023

src: use ostringstream instead of string in FromFilePath() #50253

Closed

deps: replace url parser with Ada by anonrig · Pull Request #46410 · nodejs/node · GitHub

Uh oh!

Uh oh!

deps: replace url parser with Ada - #46410

deps: replace url parser with Ada#46410anonrig wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom anonrig:deps/adaanonrig/node:deps/adaCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 29, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 30, 2023

Uh oh!

anonrig commented Jan 30, 2023

Uh oh!

anonrig commented Jan 30, 2023

Uh oh!

MoLow commented Jan 30, 2023

Uh oh!

TimothyGu commented Feb 1, 2023

Uh oh!

lemire commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 6, 2023

Uh oh!

richardlau commented Feb 6, 2023

Uh oh!

anonrig commented Mar 22, 2023

Uh oh!

richardlau commented Mar 22, 2023

Uh oh!

targos commented Mar 22, 2023

Uh oh!

danielleadams commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

18 participants

deps: replace url parser with Ada#46410
anonrig wants to merge 2 commits into
nodejs:mainfrom
anonrig:deps/ada

anonrig commented Jan 29, 2023 •
edited
Loading

lemire commented Feb 1, 2023 •
edited
Loading