perf: remove heap allocation in parse_host #1021
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
=======================================
Coverage ? 80.25%
=======================================
Files ? 24
Lines ? 4299
Branches ? 0
=======================================
Hits ? 3450
Misses ? 849
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| @@ -80,15 +79,34 @@ impl Host<String> { | |||
| /// | |||
| /// <https://url.spec.whatwg.org/#host-parsing> | |||
| pub fn parse(input: &str) -> Result<Self, ParseError> { | |||
There was a problem hiding this comment.
I didn't add, change, or remove from the public API, but please verify this as well in case I missed something.
url/src/host.rs
Outdated
| // SAFETY: If borrowed, then the original string is ascii and we can return the | ||
| // original Cow in order to save an allocation |
There was a problem hiding this comment.
My reading of the code suggests this is correct right now, but:
There is no guarantee that idna::domain_to_ascii_cow could not return a Cow::Borrowed(x) where x.len() < domain.len() in the future (where this code would break or possibly be unsafe). To avoid that refactoring hazard, we should hoist this up into idna.
There was a problem hiding this comment.
Hoisted. Not to sure about the new function name.
| /// and using Punycode as necessary. | ||
| /// | ||
| /// This process may fail. | ||
| pub fn domain_to_ascii_from_cow( |
There was a problem hiding this comment.
@hsivonen Please check if you find this new public API of idna to be acceptable.
Before:
After: