GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — deps: replace url parser with Ada by anonrig · Pull Request #46410 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Jan 29, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yvele reacted with thumbs up emoji mariuz and worstpractice reacted with hooray emoji tonivj5, climba03003, tony-go, miguelteixeiraa, GeoffreyBooth, Qard, bricss, benjamingr, robpalme, jasonwilliams, and 8 more reacted with heart emoji Trott, RaisinTen, acommodari, ronag, debadree25, tonivj5, marco-ippolito, tony-go, marvinhagemeister, miguelteixeiraa, and 10 more reacted with rocket emoji bricss, tonivj5, acommodari, tony-go, Qard, and worstpractice reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 🎉 2 reactions
  • ❤️ 18 reactions
  • 🚀 20 reactions
  • 👀 6 reactions
@nodejs-github-bot

nodejs-github-bot commented Jan 29, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jan 29, 2023
@anonrig
anonrig force-pushed the deps/ada branch 9 times, most recently from 8f4d1a4 to 4a68c84 Compare January 30, 2023 02:37
mcollina
mcollina reviewed Jan 30, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tony-go reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@anonrig
anonrig force-pushed the deps/ada branch 3 times, most recently from 21adb01 to 6f070dc Compare January 30, 2023 17:28
@anonrig anonrig mentioned this pull request Jan 30, 2023
Add Ada to Node.js ada-url/ada#142
Closed
@joyeecheung

joyeecheung commented Jan 30, 2023

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Jan 30, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the deps/ada branch 2 times, most recently from 5e1e1ac to a1c7401 Compare January 30, 2023 20:35
@anonrig

anonrig commented Jan 30, 2023

Copy link
Copy Markdown
Member Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow

MoLow commented Jan 30, 2023

Copy link
Copy Markdown
Member

@anonrig it might very well be related to parsing the debugger websocket url
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the deps/ada branch 5 times, most recently from eb45d67 to 17d3ed7 Compare February 1, 2023 19:14
@TimothyGu

TimothyGu commented Feb 1, 2023

Copy link
Copy Markdown
Member

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.)
tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lemire

lemire commented Feb 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot closed this Feb 6, 2023
nodejs-github-bot pushed a commit that referenced this pull request Feb 6, 2023
@anonrig @nodejs-github-bot
deps: add ada as a dependency
df05cf7 
PR-URL: #46410
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Feb 6, 2023
@anonrig @nodejs-github-bot
url: replace url-parser with ada
c4581ab 
PR-URL: #46410
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@richardlau richardlau added the baking-for-lts PRs whose changes need more time before an LTS backport. label Feb 6, 2023
@richardlau

richardlau commented Feb 6, 2023

Copy link
Copy Markdown
Member

@nodejs/lts I've put baking-for-lts PRs whose changes need more time before an LTS backport. 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.
anonrig reacted with thumbs up emoji styfle reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions 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
@tniessen
src: fix array overrun in node::url::SetArgs()
c405efd 
Refs: nodejs#46410
@tniessen tniessen mentioned this pull request Feb 7, 2023
src: fix array overrun in node::url::SetArgs() #46541
Closed
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@anonrig @MylesBorins
deps: add ada as a dependency
7d6c27e 
PR-URL: #46410
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
v19.7.0 proposal #46725
Merged
@v1gnesh v1gnesh mentioned this pull request Feb 20, 2023
Use ada url parser aio-libs/aiohttp#7217
Closed
@zloirock zloirock mentioned this pull request Feb 21, 2023
URL issues #46755
Closed
@asamuzaK asamuzaK mentioned this pull request Feb 26, 2023
Quotes in the URL are not percent encoded #46850
Closed
@juanarbol juanarbol mentioned this pull request Mar 1, 2023
src: use string_view for report and related code #46723
Merged
@styfle styfle mentioned this pull request Mar 2, 2023
Deprecate url.parse API in favor of URL constructor watson/original-url#11
Open
@anonrig

anonrig commented Mar 22, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Mar 22, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Mar 22, 2023

Copy link
Copy Markdown
Member

I'd be ok for a backport to active v18.x, not earlier.
MoLow, anonrig, benjamingr, and danielleadams reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danielleadams

danielleadams commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 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 RaisinTen mentioned this pull request May 6, 2023
Ada's license #47894
Closed
@pluris pluris mentioned this pull request Aug 10, 2023
src: add a condition if the argument of DomainToUnicode is empty #49097
Merged
@pluris pluris mentioned this pull request Oct 18, 2023
src: use ostringstream instead of string in FromFilePath() #50253
Closed
@zloirock zloirock mentioned this pull request Nov 8, 2026
url: setters overwrite URL changes made during value coercion #67345
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@mcollina mcollina mcollina approved these changes
@benjamingr benjamingr benjamingr approved these changes
@ronag ronag ronag approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
+3 more reviewers
@dougwilson dougwilson dougwilson left review comments
@bmeck bmeck bmeck left review comments
@TimothyGu TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. review wanted PRs that need review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

URL API: ensure it roundtrips for opaque paths Investigate slow new URL custom protocol parse result is different from chrome and firefox Bug in WHATWG URL parsing Performance of WHATWG URL API
Strip tab and newline from URL's protocol setter

Uh oh!

There was an error while loading. Please reload this page.

18 participants

@anonrig @nodejs-github-bot @joyeecheung @MoLow @TimothyGu @lemire @Trott @targos @radekg @richardlau @danielleadams @mcollina @dougwilson @bmeck @jasnell @benjamingr @ronag @RafaelGSS
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.