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

std: remove an allocation in Path::with_extension #113106

Merged
merged 2 commits into from
Jul 21, 2023
Merged

std: remove an allocation in Path::with_extension #113106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11f35d6
std: remove an allocation in `Path::with_extension`
marcospb19 Jun 27, 2023
6ffca76
std: add tests for `Path::with_extension`
marcospb19 Jul 14, 2023
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
24 changes: 21 additions & 3 deletions library/std/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2608,9 +2608,27 @@ impl Path {
}

fn _with_extension(&self, extension: &OsStr) -> PathBuf {
let mut buf = self.to_path_buf();
buf.set_extension(extension);
buf
let self_len = self.as_os_str().len();
let self_bytes = self.as_os_str().as_os_str_bytes();

let (new_capacity, slice_to_copy) = match self.extension() {
None => {
// Enough capacity for the extension and the dot
let capacity = self_len + extension.len() + 1;
let whole_path = self_bytes.iter();
(capacity, whole_path)
}
Some(previous_extension) => {
let capacity = self_len + extension.len() - previous_extension.len();
let path_till_dot = self_bytes[..self_len - previous_extension.len()].iter();
(capacity, path_till_dot)
}
};

let mut new_path = PathBuf::with_capacity(new_capacity);
new_path.as_mut_vec().extend(slice_to_copy);
new_path.set_extension(extension);
new_path
}

/// Produces an iterator over the [`Component`]s of the path.
Expand Down
50 changes: 45 additions & 5 deletions library/std/src/path/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,7 @@ pub fn test_prefix_ext() {
#[test]
pub fn test_push() {
macro_rules! tp (
($path:expr, $push:expr, $expected:expr) => ( {
($path:expr, $push:expr, $expected:expr) => ({
let mut actual = PathBuf::from($path);
actual.push($push);
assert!(actual.to_str() == Some($expected),
Expand Down Expand Up @@ -1281,7 +1281,7 @@ pub fn test_push() {
#[test]
pub fn test_pop() {
macro_rules! tp (
($path:expr, $expected:expr, $output:expr) => ( {
($path:expr, $expected:expr, $output:expr) => ({
let mut actual = PathBuf::from($path);
let output = actual.pop();
assert!(actual.to_str() == Some($expected) && output == $output,
Expand Down Expand Up @@ -1335,7 +1335,7 @@ pub fn test_pop() {
#[test]
pub fn test_set_file_name() {
macro_rules! tfn (
($path:expr, $file:expr, $expected:expr) => ( {
($path:expr, $file:expr, $expected:expr) => ({
let mut p = PathBuf::from($path);
p.set_file_name($file);
assert!(p.to_str() == Some($expected),
Expand Down Expand Up @@ -1369,7 +1369,7 @@ pub fn test_set_file_name() {
#[test]
pub fn test_set_extension() {
macro_rules! tfe (
($path:expr, $ext:expr, $expected:expr, $output:expr) => ( {
($path:expr, $ext:expr, $expected:expr, $output:expr) => ({
let mut p = PathBuf::from($path);
let output = p.set_extension($ext);
assert!(p.to_str() == Some($expected) && output == $output,
Expand All @@ -1394,6 +1394,46 @@ pub fn test_set_extension() {
tfe!("/", "foo", "/", false);
}

#[test]
pub fn test_with_extension() {
macro_rules! twe (
($input:expr, $extension:expr, $expected:expr) => ({
let input = Path::new($input);
let output = input.with_extension($extension);

assert!(
output.to_str() == Some($expected),
"calling Path::new({:?}).with_extension({:?}): Expected {:?}, got {:?}",
$input, $extension, $expected, output,
);
});
);

twe!("foo", "txt", "foo.txt");
twe!("foo.bar", "txt", "foo.txt");
twe!("foo.bar.baz", "txt", "foo.bar.txt");
twe!(".test", "txt", ".test.txt");
twe!("foo.txt", "", "foo");
twe!("foo", "", "foo");
twe!("", "foo", "");
twe!(".", "foo", ".");
twe!("foo/", "bar", "foo.bar");
twe!("foo/.", "bar", "foo.bar");
twe!("..", "foo", "..");
twe!("foo/..", "bar", "foo/..");
twe!("/", "foo", "/");
Comment on lines +1412 to +1424
Copy link
Contributor Author

@marcospb19 marcospb19 Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After inspecting these, I realized that my implementation over-allocates in some of the corner cases.

/* ok     */ twe!("foo", "txt", "foo.txt");
/* ok     */ twe!("foo.bar", "txt", "foo.txt");
/* ok     */ twe!("foo.bar.baz", "txt", "foo.bar.txt");
/* ok     */ twe!(".test", "txt", ".test.txt");
/* over 1 */ twe!("foo.txt", "", "foo");
/* over 1 */ twe!("foo", "", "foo");
/* over 4 */ twe!("", "foo", "");
/* over 4 */ twe!(".", "foo", ".");
/* over 1 */ twe!("foo/", "bar", "foo.bar");
/* over 1 */ twe!("foo/.", "bar", "foo.bar");
/* over 4 */ twe!("..", "foo", "..");
/* over 4 */ twe!("foo/..", "bar", "foo/..");
/* over 4 */ twe!("/", "foo", "/");

For this one specifically, it does an extra (potentially) unused allocation, because it's an empty path:

/* over 4 */ twe!("", "foo", "");

EDIT: I believe they're mostly OK, because I assume that 99% of calls fit in the first 4 scenarios (or are only over-allocating 1 byte), but please let me know what you think.


// New extension is smaller than file name
twe!("aaa_aaa_aaa", "bbb_bbb", "aaa_aaa_aaa.bbb_bbb");
// New extension is greater than file name
twe!("bbb_bbb", "aaa_aaa_aaa", "bbb_bbb.aaa_aaa_aaa");

// New extension is smaller than previous extension
twe!("ccc.aaa_aaa_aaa", "bbb_bbb", "ccc.bbb_bbb");
// New extension is greater than previous extension
twe!("ccc.bbb_bbb", "aaa_aaa_aaa", "ccc.aaa_aaa_aaa");
Comment on lines +1426 to +1434
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are necessary to get the bug you spotted.

The other tests do not, because they all use foo, bar and baz, which all contain the same length, this can be a problem in the case of some hidden bugs that only manifest when the path piece sizes are different.

}

#[test]
fn test_eq_receivers() {
use crate::borrow::Cow;
Expand Down Expand Up @@ -1669,7 +1709,7 @@ fn into_rc() {
#[test]
fn test_ord() {
macro_rules! ord(
($ord:ident, $left:expr, $right:expr) => ( {
($ord:ident, $left:expr, $right:expr) => ({
use core::cmp::Ordering;

let left = Path::new($left);
Expand Down