perf: improve to_file_path() #1018
Conversation
url/src/lib.rs
Outdated
| } else { | ||
| Vec::new() | ||
| }; | ||
| let mut bytes = Vec::with_capacity(estimated_capacity); |
There was a problem hiding this comment.
Before 1,000 iterations within an iteration:
test url_to_file_path ... bench: 202,125 ns/iter (+/- 12,207)
After:
test url_to_file_path ... bench: 127,257 ns/iter (+/- 4,782)
| host: Option<&str>, | ||
| segments: str::Split<char>, | ||
| ) -> Result<PathBuf, ()> { | ||
| file_url_segments_to_pathbuf_windows(host, segments) | ||
| file_url_segments_to_pathbuf_windows(estimated_capacity, host, segments) | ||
| } | ||
|
|
||
| // Build this unconditionally to alleviate https://github.com/servo/rust-url/issues/102 | ||
| #[cfg(feature = "std")] | ||
| #[cfg_attr(not(windows), allow(dead_code))] | ||
| fn file_url_segments_to_pathbuf_windows( |
There was a problem hiding this comment.
Before 1000 iterations within an iteration:
test url_to_file_path ... bench: 437,555 ns/iter (+/- 11,519)
After:
test url_to_file_path ... bench: 119,461 ns/iter (+/- 5,927)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
=======================================
Coverage ? 79.97%
=======================================
Files ? 24
Lines ? 4274
Branches ? 0
=======================================
Hits ? 3418
Misses ? 856
Partials ? 0 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
url/src/lib.rs
Outdated
| @@ -2720,7 +2720,7 @@ impl Url { | |||
| _ => return Err(()), | |||
| }; | |||
|
|
|||
| return file_url_segments_to_pathbuf(host, segments); | |||
| return file_url_segments_to_pathbuf(self.as_str().len(), host, segments); | |||
There was a problem hiding this comment.
Please split the estimated_len into its own temp variable so it's cleaner to read what this is.
I think this over-alloc 7 bytes in 99% of cases on unix (file://). On redox, just 2 (//). On Windows, even 8 (file:///). Windows also has hostname support though, so to ensure no-realloc in most cases, you should probably do -5 there too (file: - the extra // is for the leading \\ in hostname cases).
The cost of doing this calculation may be too much perf cost though - so please check if this minor memory improvement wouldn't regress perf too much.
There was a problem hiding this comment.
It adds a few nanoseconds. Worth it to use less memory IMO.
url/src/lib.rs
Outdated
| @@ -2720,7 +2720,18 @@ impl Url { | |||
| _ => return Err(()), | |||
| }; | |||
|
|
|||
| return file_url_segments_to_pathbuf(host, segments); | |||
| let estimated_capacity = self.as_str().len() | |||
| - if cfg!(target_os = "redox") { | |||
There was a problem hiding this comment.
Can you make this a clamped subtract so that it doesn't wrap around in release when someone calls this with something other than file scheme URLs?
There was a problem hiding this comment.
Good catch. I think the logic needs to be a bit smarter here because urls could be like a:/path? I updated the code to at least be a fast path for file://-like urls and maybe in some weird cases like a:/path it might under allocate. Probalby not worth the effort to make it smarter than it is now.
| let file_scheme_len = "file".len(); | ||
| // remove only // because it still has file: | ||
| if scheme_len < file_scheme_len { | ||
| let scheme_diff = file_scheme_len - scheme_len; |
There was a problem hiding this comment.
This still wraps around when scheme is longer than 4.
There was a problem hiding this comment.
There's a check above for this:
if scheme_len < file_scheme_len { // if 5 < 4 {
(or maybe I'm misreading)
Reduces some heap allocations and pre-allocates the vector for building the string.