search

SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.41k stars 3.18k forks source link

Proposal: Replacing existing URL parser with Ada #17584

Closed anonrig closed 1 year ago

anonrig commented 1 year ago

Hello all,

I'm one of the authors of Ada (https://github.com/ada-url/ada). I've been a long-term follower/supporter of this project and am excited to propose using Ada as the default URL parser. We're already spec compliant and used by Node.js (v19.7.0). Moving to Ada would reduce the workload of maintaining the compliance with the changes happening in the URL spec.

Even though we tend not to share any benchmarks (at the moment), you can find a sneak peek from: https://github.com/ada-url/ada/pull/223

AtkinsSJ commented 1 year ago

Hi anonrig!

We have a very strong belief in implementing everything ourselves, and not using outside libraries. (Qt for Ladybird is the one exception I'm aware of.) So we wouldn't adopt Ada, even though it does look very nice. :^)

ADKaster commented 1 year ago

Speaking of your benchmarks though, I would be curious to see how AK::URLParser compares to the other parsers in the test suite:

https://github.com/SerenityOS/serenity/blob/master/AK/URLParser.cpp#L200

It should be straightforward enough to use the FetchLagom.cmake module from Ladybird/ or from linusg/libjs-test262 and target_link_libraries(Lagom::Core) to include it into a test file.

But like Sam already mentioned, third party code is not in the cards for the serenity project: NIH is half the point 🙂.

anonrig commented 1 year ago

@AtkinsSJ @ADKaster Happy to add a benchmark to see where we both are, but it does not seem an easy task just to include URLParser in SerenityOS rather than the whole project.

Our benchmark suite lives inside: https://github.com/ada-url/ada/blob/main/benchmarks/CMakeLists.txt