-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
I want to hold this until the URL wpt are relanded. |
Ready for review. |
I'm in favor of this PR because:
|
a50d6a9
to
799346a
Compare
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://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:
|
Improved by roughly 50% by creating |
f1e6611
to
cedaa62
Compare
* Clippy warning "manual implementation of Option::map()" * Use type_error()/generic_error() constructors * Make fewer String copies
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.
LGTM, thank you for your patience @nayeemrmn.
I get very excited when a patch removes 700 lines of code :)
cc @lucacasonato
Demo of replacing as much as possible of
URL
andURLSearchParams
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 innew 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.