Skip to content

perf: improve to_file_path() #1018

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Feb 8, 2025
+62 −13
Merged

perf: improve to_file_path() #1018

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbaf6df
perf: improve to_file_path()
dsherret Jan 27, 2025
93682a6
fix
dsherret Jan 27, 2025
313bde0
Merge branch 'main' into perf_improve_url_to_file_path
dsherret Feb 4, 2025
3e1aeb4
make estimate more exact
dsherret Feb 6, 2025
51bc795
use saturating_sub
dsherret Feb 6, 2025
22873d0
better to try_reserve
dsherret Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions url/benches/parse_url.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,19 @@ fn punycode_rtl(bench: &mut Bencher) {
bench.iter(|| black_box(url).parse::<Url>().unwrap());
}

fn url_to_file_path(bench: &mut Bencher) {
let url = if cfg!(windows) {
"file:///C:/dir/next_dir/sub_sub_dir/testing/testing.json"
} else {
"file:///data/dir/next_dir/sub_sub_dir/testing/testing.json"
};
let url = url.parse::<Url>().unwrap();

bench.iter(|| {
black_box(url.to_file_path().unwrap());
});
}

benchmark_group!(
benches,
short,
Expand All @@ -95,5 +108,6 @@ benchmark_group!(
punycode_ltr,
unicode_rtl,
punycode_rtl,
url_to_file_path
);
benchmark_main!(benches);
31 changes: 18 additions & 13 deletions url/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2720,7 +2720,7 @@
_ => return Err(()),
};

return file_url_segments_to_pathbuf(host, segments);
return file_url_segments_to_pathbuf(self.as_str().len(), host, segments);
Copy link
Collaborator

@lucacasonato lucacasonato Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dsherret dsherret Feb 6, 2025

Choose a reason for hiding this comment

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

It adds a few nanoseconds. Worth it to use less memory IMO.

Copy link
Collaborator

@lucacasonato lucacasonato Feb 6, 2025

Choose a reason for hiding this comment

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

Agreed.

}
Err(())
}
Expand Down Expand Up @@ -3030,6 +3030,7 @@
any(unix, target_os = "redox", target_os = "wasi", target_os = "hermit")
))]
fn file_url_segments_to_pathbuf(
estimated_capacity: usize,
host: Option<&str>,
segments: str::Split<'_, char>,
) -> Result<PathBuf, ()> {
Expand All @@ -3047,11 +3048,10 @@
return Err(());
}

let mut bytes = if cfg!(target_os = "redox") {
b"file:".to_vec()
} else {
Vec::new()
};
let mut bytes = Vec::with_capacity(estimated_capacity);
Copy link
Contributor Author

@dsherret dsherret Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

if cfg!(target_os = "redox") {
bytes.extend(b"file:");
}

for segment in segments {
bytes.push(b'/');
Expand Down Expand Up @@ -3083,22 +3083,26 @@

#[cfg(all(feature = "std", windows))]
fn file_url_segments_to_pathbuf(
estimated_capacity: usize,
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(
Copy link
Contributor Author

@dsherret dsherret Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

estimated_capacity: usize,
host: Option<&str>,
mut segments: str::Split<'_, char>,
) -> Result<PathBuf, ()> {
use percent_encoding::percent_decode;
let mut string = if let Some(host) = host {
r"\\".to_owned() + host
use percent_encoding::percent_decode_str;
let mut string = String::with_capacity(estimated_capacity);
if let Some(host) = host {
string.push_str(r"\\");
string.push_str(host);

Check warning on line 3105 in url/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

url/src/lib.rs#L3102-L3105 

Added lines #L3102 - L3105 were not covered by tests
} else {
let first = segments.next().ok_or(())?;

Expand All @@ -3108,7 +3112,7 @@
return Err(());
}

first.to_owned()
string.push_str(first);

Check warning on line 3115 in url/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

url/src/lib.rs#L3115 

Added line #L3115 was not covered by tests
}

4 => {
Expand All @@ -3120,7 +3124,8 @@
return Err(());
}

first[0..1].to_owned() + ":"
string.push_str(&first[0..1]);
string.push(':');

Check warning on line 3128 in url/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

url/src/lib.rs#L3127-L3128 

Added lines #L3127 - L3128 were not covered by tests
}

_ => return Err(()),
Expand All @@ -3131,7 +3136,7 @@
string.push('\\');

// Currently non-unicode windows paths cannot be represented
match String::from_utf8(percent_decode(segment.as_bytes()).collect()) {
match percent_decode_str(segment).decode_utf8() {

Check warning on line 3139 in url/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

url/src/lib.rs#L3139 

Added line #L3139 was not covered by tests
Ok(s) => string.push_str(&s),
Err(..) => return Err(()),
}
Expand Down
Loading