Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ada-url dependency, initial impl of jsg::Url #1273

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 5, 2023

This will serve as the new underlying implementation of the url-standard.{h|c++} class while also supporting handling of module import specifiers as URLs.

Why using ada-url at all? It's fast and spec compliant (verified against the web platform tests for the URL spec). And it allows us to delete a bunch of code relating to our current standard URL parser. That'll be done in a separate URL as we need to verify that switching won't break any edge cases.

/cc @anonrig

src/workerd/jsg/url.c++ Outdated Show resolved Hide resolved
Copy link
Collaborator

@irvinebroque irvinebroque left a comment

Choose a reason for hiding this comment

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

🎉 exciting! two questions

  1. Re: edge cases — maybe a way to run ada in an observe only mode for some period of time, for some subset of requests, log any differences in output between our current implementation and ada-url? or do we know enough with confidence to hunt these down upfront?
  2. Ada is fast, wonder how much faster than what we have today?

src/workerd/jsg/url.c++ Outdated Show resolved Hide resolved
src/workerd/jsg/url.c++ Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2023

@irvinebroque:

Re: edge cases — maybe a way to run ada in an observe only mode for some period of time, for some subset of requests, log any differences in output between our current implementation and ada-url? or do we know enough with confidence to hunt these down upfront?

I do not plan on replacing the underlying implementation of url-standard.c++ right away. What I want to do is set up a parallel of tests first to ensure that all of our existing URL tests are handled just fine,.

Ada is fast, wonder how much faster than what we have today?

The current url-standard implementation was based entirely off of the original Node.js implementation. I would expect the performance improvement here to be able the same as we see from Node.js' switch to ada.

@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2023

This is currently having challenges on windows due to the simd instructions. Will need to get that figured out.

@fhanau
Copy link
Collaborator

fhanau commented Oct 5, 2023

I believe the SIMD problem here is the same problem we had with other code after the Github runner image upgraded to LLVM 16 – this caused bazel to include the wrong SIMD header. Orion developed a workaround and contributed a fix (bazelbuild/bazel#19391) that will be available in bazel 6.4.0. To solve the issues for this PR, you can adjust .bazelrc to modify the includes like Orion did in the workaround. 6.4.0 should be released next week though, so you can also see if that fixes things here too by testing with the release candidate as in #1277.

build/BUILD.ada-url Outdated Show resolved Hide resolved
src/workerd/jsg/url.c++ Outdated Show resolved Hide resolved
src/workerd/jsg/url.c++ Show resolved Hide resolved
build/BUILD.ada-url Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/ada-url branch 2 times, most recently from b3f5a3f to 3d9e6cb Compare October 5, 2023 17:54
@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2023

woo green ci! next step is to update the internal repo workspace to make sure this will compile.

@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2023

Internal CI looks good.

@gitguardian

This comment was marked as resolved.

@jasnell jasnell force-pushed the jsnell/ada-url branch 2 times, most recently from e2eb114 to bfa307a Compare October 5, 2023 22:48
@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2023

Added basic parse tests based on the corpus of WPT url tests. These don't test all of the getters/setters on jsg::Url but they do test that URLs that should be parseable are parseable and that the url.getHref() value is correct. These prove that we're able to successfully parse even more than the current standard workerd::api::URL supports!

src/workerd/jsg/url.c++ Outdated Show resolved Hide resolved
src/workerd/jsg/url.h Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Oct 10, 2023

Going to wait to land this until after we also get the url search params iterator support landed up stream, that way we can convert URLSearchParams over to this also. ada-url/ada#532

@jasnell
Copy link
Member Author

jasnell commented Oct 10, 2023

Ok, jsg::UrlSearchParams added

This will serve as the new underlying implementation of the
url-standard.{h|c++} class while also supporting handling of
module import specifiers as URLs.
@jasnell
Copy link
Member Author

jasnell commented Oct 11, 2023

Merging this. There's still a lot to do before we can really start using it.

Just a taste:

  • Need to update the URLPattern implementation to decouple it from the old url-standard impl
  • Need to add more fuzz testing for the ada url implementation (just for our own testing)
  • Need to update the url-standard impl to use this instead

@jasnell jasnell merged commit d9ed653 into main Oct 11, 2023
7 checks passed
@fhanau fhanau deleted the jsnell/ada-url branch December 4, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants