-
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
perf: optimize URL serialization #15663
Conversation
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.
Blocked on servo/rust-url#788
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.
Looks great!
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.
Can you add a comment explaining how op_url_parse
, op_url_get_serialization
, and op_url_parse_with_base
interact? Would be very helpful for future readers. After that, LGTM!
return core.ops.op_url_get_serialization(); | ||
} else { | ||
throw new TypeError("Invalid URL"); | ||
} | ||
} | ||
|
||
class URLSearchParams { |
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.
i did some benchmarking and there is a small change we can make here that passes all the wpt tests and bumps perf by 15-20% when api consumer accesses searchParams getter on URL and there are no query params, which is likely the common case. i can do this in a separate PR if you like as i think there are a few other small wins we can find here.
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.
actually, we could just move that up to the top of the function i think?
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.
Nice find!
i can do this in a separate PR
yup, that'll be great
2.4x improvement in
(new URL(path)).pathname
.