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

perf(ext/url): optimize UrlParts op serialization #11765

Merged
merged 1 commit into from Aug 19, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Aug 18, 2021

Marshalling it across the op-layer by joining the substrings into a single "fat" newline-joined string, then splitting JS side.

This shaves off a little over a 1/3 of the overhead per call:
main (after merging #11763)

bench:   3,726,203 ns/iter (+/- 177,656)

this PR:

bench:   2,400,289 ns/iter (+/- 53,911)

So with this PR and #11763 that's a combined improvement of over 2x ! (~5000ns/call => ~2400ns/call)

Whilst this might appear counterintuitive and "hacky", it works because:

  • We have 11 string values to return
  • Marshalling them in a struct creates an obj + 11 optimized/internalized strings (the keys) + 11 unoptimized "normal" strings (the values) + 11 owned strings in Rust
  • This approach creates 1 fat owned string in rust, then resplitting it in JS creates 11 efficient substrings (that aren't individually allocated and refer to the fat string)
  • \n is a safe separator because it's not allowed in URLs, technically could have used any control char but \n works and is familiar

quirks::hash(&url),
quirks::host(&url),
quirks::hostname(&url),
&quirks::origin(&url),
Copy link
Member

Choose a reason for hiding this comment

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

quirks::origin returns a String

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Have you considered making UrlParts into strurct UrlParts(Url), and then implementing serde::Serialize on it manually? All the serialization logic could be abstracted into the serializer.

LGTM though.

@AaronO
Copy link
Contributor Author

AaronO commented Aug 19, 2021

Have you considered making UrlParts into strurct UrlParts(Url), and then implementing serde::Serialize on it manually? All the serialization logic could be abstracted into the serializer.

LGTM though.

Yes, I patched the url crate so that it exposes its internal offsets. That's a more efficient serialization that I've prototyped locally, but it either requires patching url or doing pointer arithmetic to recompute those internal offsets.

This PR shaves off over 33% of the call overhead whilst being conceptually simple.

@AaronO AaronO merged commit 37c983d into denoland:main Aug 19, 2021
@AaronO AaronO deleted the perf/ext-url-parsing-op-returns branch August 19, 2021 11:41
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

3 participants