std: remove an allocation in Path::with_extension
#113106
Conversation
|
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
`Path::with_extension` used to reallocate (and copy) paths twice per call, now it does it once, by checking the size of the previous and new extensions it's possible to call `PathBuf::with_capacity` and pass the exact capacity it takes. Also reduce the memory consumption of the path returned from `Path::with_extension` by using exact capacity instead of using amortized exponential growth.
marcospb19
force-pushed
the
improve-path-with-extension-function
branch
from
3346752
to
11f35d6
Compare
|
@rustbot author (Please do rustbot ready when you have added tests and it will go back into my queue, thanks) |
rustbot
added
S-waiting-on-author
| 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", "/"); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Thanks! |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1a44b45): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 650.755s -> 650.086s (-0.10%) |
Path::with_extensionused to reallocate (and copy) paths twice per call, now it does it once, by checking the size of the previous and new extensions it's possible to callPathBuf::with_capacityand pass the exact capacity required.This also reduces the memory consumption of the path returned from
Path::with_extensionby using exact capacity instead of using amortized exponential growth.