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

refactor(op_crates/web): Move URL parsing to Rust #9276

Merged
merged 6 commits into from
Mar 2, 2021

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Jan 26, 2021

cc @lucacasonato

Demo of replacing as much as possible of URL and URLSearchParams with rust-url. The biggest compromise is having to reparse URLs when any individual part setter is invoked, since *parsed* URLs are neither resources nor serialisable. There is also the op interface overhead. Note that we already op out to Rust for host parsing in new URL() and host setting. Benchmarks are potentially needed. If improvement is needed, some setters can be reverted to parse parts in isolation like currently. Ideally this wouldn't bring back too much duplicated encoding logic.

The changes in the tests are corrective other than some drive letter tests commented with a FIXME. We should be able to enable more wpt URL tests than we could now but rust-url is also not perfectly to spec. However, we use it for URL handling in Rust already and the issues can be upstreamed.

Fixes #9383.

@lucacasonato
Copy link
Member

I want to hold this until the URL wpt are relanded.

op_crates/web/lib.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator Author

Ready for review.

op_crates/web/lib.rs Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Jan 29, 2021

I'm in favor of this PR because:

  1. This is more compliant to WPT than our current implementation (ref: expectation.json)
  2. This reduces the maintenance cost of our code base significantly.
  3. This should slightly reduce the V8 snapshot size and the entire runtime size.

@ry ry requested a review from lucacasonato January 29, 2021 12:23
@piscisaureus
Copy link
Member

I'm also in favor of landing this PR, mostly because it ensures that URL parsing on the Rust side is consistent with URL behavior on the JavaScript side.

However I find it disappointing that it is not faster than our current implementation. I tried the following microbenchmark:

const t1 = Date.now();
let i = 0;
for(; i < 1e6; i++) {
  new URL("http:https://www.google.com");
}
const t2 = Date.now();
const dt = (t2 - t1) / 1e3;
const r = i / dt;
console.log(`n = ${i}, dt = ${dt.toFixed(3)}s, r = ${r.toFixed(0)}/s`);

... with the following (approximate) results on my laptop:

  • deno: 30000/s
  • deno w/ this patch: 30000/s
  • chrome: 800000/s
  • node.js: 800000/s

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Feb 5, 2021

Improved by roughly 50% by creating url.searchParams on demand.

@nayeemrmn nayeemrmn force-pushed the rust-url branch 2 times, most recently from f1e6611 to cedaa62 Compare February 10, 2021 14:22
Base automatically changed from master to main February 19, 2021 14:59
* Clippy warning "manual implementation of Option::map()"
* Use type_error()/generic_error() constructors
* Make fewer String copies
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your patience @nayeemrmn.
I get very excited when a patch removes 700 lines of code :)

@piscisaureus piscisaureus merged commit badc88b into denoland:main Mar 2, 2021
@nayeemrmn nayeemrmn deleted the rust-url branch March 2, 2021 01:32
kt3k added a commit to kt3k/deno_std that referenced this pull request Mar 2, 2021
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.

Bugs in URL implementation
4 participants