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.

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

perf: improve to_file_path() #1018

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

perf: improve to_file_path() #1018

Show file tree
Hide file tree
Changes from 1 commit
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 @@ -2722,7 +2722,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);
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

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

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 @@ -3032,6 +3032,7 @@ fn path_to_file_url_segments_windows(
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 @@ -3049,11 +3050,10 @@ fn file_url_segments_to_pathbuf(
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 @@ -3085,22 +3085,26 @@ fn file_url_segments_to_pathbuf(

#[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('\\');
dsherret marked this conversation as resolved.
Show resolved Hide resolved
string.push_str(host);
} else {
let first = segments.next().ok_or(())?;

Expand All @@ -3110,7 +3114,7 @@ fn file_url_segments_to_pathbuf_windows(
return Err(());
}

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

4 => {
Expand All @@ -3122,7 +3126,8 @@ fn file_url_segments_to_pathbuf_windows(
return Err(());
}

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

_ => return Err(()),
Expand All @@ -3133,7 +3138,7 @@ fn file_url_segments_to_pathbuf_windows(
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() {
Ok(s) => string.push_str(&s),
Err(..) => return Err(()),
}
Expand Down
Loading