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

anonrig · 2023-01-29T22:19:33Z

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
Fixesnodejs/performance#33
Closes#41220

nodejs-github-bot · 2023-01-29T22:19:38Z

mcollina

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.

joyeecheung · 2023-01-30T17:47:21Z

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

anonrig · 2023-01-30T17:57:21Z

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.

anonrig · 2023-01-30T21:06:02Z

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)

MoLow · 2023-01-30T21:20:36Z

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

TimothyGu · 2023-02-01T19:25:19Z

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

lemire · 2023-02-01T19:40:28Z

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.

nodejs-github-bot · 2023-02-06T19:21:42Z

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>

richardlau · 2023-02-06T19:24:45Z

@nodejs/lts I've put baking-for-ltsPRs 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>

anonrig · 2023-03-22T16:22:26Z

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

richardlau · 2023-03-22T16:41:13Z

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

targos · 2023-03-22T16:44:45Z

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

danielleadams · 2023-04-03T11:41:35Z

@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 4a68c84Compare 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 6f070dcCompare 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 a1c7401Compare January 30, 2023 20:35

anonrig force-pushed the deps/ada branch 5 times, most recently from eb45d67 to 17d3ed7Compare 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-actionsBot 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

Uh oh!

Uh oh!

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

anonrig commented Jan 29, 2023•
edited
Loading

lemire commented Feb 1, 2023•
edited
Loading