Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
1627e2f to
33e69e1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
33e69e1 to
ed9d33d
Compare
This comment has been minimized.
This comment has been minimized.
ed9d33d to
0b0f84c
Compare
This comment has been minimized.
This comment has been minimized.
0b0f84c to
8ed33ea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2151b8f to
83cdbed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
83cdbed to
3921fff
Compare
rustbot
added
S-waiting-on-author
0a25dda to
92e0132
Compare
|
New benchmarking results. You can see what the benchmark code looks like here and run it yourself to see if there are any difference in measurements on your end: This is the measurement of the current implementation of This is the measurement of the new implementation of Edit: Updated |
|
Here are the benchmark results with black box: From current From this Edit: Updated Edit 2: Took off Path ordering benchmark here since it was incorrect see below to see corrected path ordering benchmarks. |
92e0132 to
574d7f2
Compare
rustbot
added
S-waiting-on-review
|
I'm confident this code works (passed CI in previous run, the current amended commit change I made doesn't change logic, but makes the code written in a more idiomatic way). In my opinion, the logic in this code should look more readable than how |
|
@rustbot label +I-libs-nominated Since |
rustbot
added
the
I-libs-nominated
| // and will be handled below. | ||
| parse_prefix(other_path.as_os_str()).is_some() | ||
| } else { | ||
| // On Unix: prefix is always None. |
There was a problem hiding this comment.
Irrelevant here, since this is Windows code.
| self_path.inner = res; | ||
| return; | ||
|
|
||
| // `path` has a root but no prefix, e.g., `\windows` (Windows only) |
There was a problem hiding this comment.
Again, (Windows only) is irrelevant here, I think?
| /// Says whether the first byte after the prefix is a separator. | ||
| fn has_physical_root(s: &[u8], prefix: Option<Prefix<'_>>) -> bool { | ||
| let path = if let Some(p) = prefix { &s[p.len()..] } else { s }; | ||
| !path.is_empty() && is_sep_byte(path[0]) | ||
| } | ||
| // /// Says whether the first byte after the prefix is a separator. | ||
| // fn has_physical_root(s: &[u8], prefix: Option<Prefix<'_>>) -> bool { | ||
| // let path = if let Some(p) = prefix { &s[p.len()..] } else { s }; | ||
| // !path.is_empty() && is_sep_byte(path[0]) | ||
| // } |
There was a problem hiding this comment.
Can just be deleted?
There was a problem hiding this comment.
My bad, will delete this soon.
| match self.path[front..].iter().position(|b| { | ||
| if !is_sep_byte(*b) { | ||
| if *b == b'.' && !cur_dir_present { | ||
| cur_dir_present = true; | ||
| false | ||
| } else { | ||
| true | ||
| } | ||
| } else { | ||
| cur_dir_present = false; | ||
| false | ||
| } | ||
| }) { |
There was a problem hiding this comment.
Gonna be honest, I kind of hate this logic, mostly because having stateful position code is just very confusing to reason about.
I kind of would rather you just use a for loop here to figure this out.
There was a problem hiding this comment.
(Note: it is certainly clever, but looking at this code makes my brain hurt, and I'm not sure it actually has any performance justifications.)
There was a problem hiding this comment.
Will have to check if rewriting it as a for loop here will affect performance. Unsure if LLVM will optimize it as well as the iter version.
I think I might have start off with understanding how position iterates underneath the hood. The thing I'm worried about implementing this in a for loop is ensuring that I'm implementing this in a way that it gets autovectorized really well; I believe any incrementation with front here has to be done in the end so you prevent subsequent iterations from having a dependency on the previous iteration
There was a problem hiding this comment.
Honestly, I'm kind of surprised this autovectorises very well. If there are compelling performance benefits I'm fine keeping it, just, it feels like there might be a nicer way of doing this.
There was a problem hiding this comment.
Honestly, I'm kind of surprised this autovectorises very well.
I took a look at what position does underneath the hood:
rust/library/core/src/slice/iter/macros.rs
Lines 372 to 388 in d080e7d
Okay, so I think I get what it's autovectorizing. This part of the code:
match self.path[front..].iter().position(|b| {
if !is_sep_byte(*b) {
if *b == b'.' && !cur_dir_present {
cur_dir_present = true;
false
} else {
true
}
} else {
cur_dir_present = false;
false
}
})This is based on my own assumption on what could happen underneath the hood (haven't actually looked at machine instructions), but what I can see here that could be done in parallel by the CPU/compiler optimizations is: fetching the next byte in the self.path via .next() method, checking if that byte is a separator from !is_sept_byte(*b), and as well as checking if the byte is a cur dir byte from *b == b'.'; none of those portions of the code have a dependency on previous iterations. Unfortunately, the !cur_dir_present condition is dependent on previous iterations, as we set the cur_dir_present variable to true if it's currently false and we see that the byte we're looking at is a cur dir byte, so some sort of synchronization has to occur over there. That's unavoidable since we want to normalize away non-starting cur dir components, but not normalize things like "..".
However, there is maybe one more thing that the CPU could do in parallel that we can optimize. In Iter::position, it's incrementing a variable i by 1 in each loop. However, technically, we don't need a local variable that's incremented by 1 in each loop as that forces subsequent iterations to depend on previous iterations for that i to be incremented appropriately. Since we know the bounds of our indices from self.path[front..], it should be pretty clear to the compiler that if we enumerated both the byte and index, we could do this in parallel fine. This is how I'm thinking about the for loop version right now:
fn normalize_front(&mut self, front: usize) -> usize {
let mut cur_dir_present = false;
for (index, &byte) in self.path[front..].iter().enumerate() { // could be done in parallel by CPU
if !is_sep_byte(byte) { // could be done in parallel by CPU
if byte == b'.' && !cur_dir_present { // first condition parallel, second not parallel
cur_dir_present = true;
} else {
if cur_dir_present {
// Minus 1 to reorient the path back to the starting the "." in e.g. ".a" or "
return front + index - 1;
} else {
return front + index;
}
}
} else {
cur_dir_present = false;
}
}
// Consumed the iterator, so there's nothing left to normalize
self.state = State::Done;
self.path.len()
}There was a problem hiding this comment.
If there are compelling performance benefits I'm fine keeping it, just, it feels like there might be a nicer way of doing this.
So the for loop way I showed earlier does not run as fast as the iter version. I haven't run all the benchmark cases for all different component byte sizes, but it's very telling to see that performance was being dropped noticeably across all the cases for 1 and 3 byte components (e.g. 150 ns original to 200 ns using this for loop on running the components next iter case with a 1 byte component). I even tried doing this:
fn normalize_front(&mut self, mut front: usize) -> usize {
for &byte in self.path[front..].iter() {
if !is_sep_byte(byte) {
if byte == b'.' && !cur_dir_present {
cur_dir_present = true;
} else {
if cur_dir_present {
return front - 1;
} else {
return front;
}
}
} else {
cur_dir_present = false;
}
front += 1;
}
self.state = State::Done;
self.path.len()
}Which should be the exact equivalent to what the current iter version I have here is doing, and it still showed some degradation (e.g. 150 ns original to 190 ns using this for loop on running the components next iter case with a 1 byte component). It's still better than the current standard library implementation of Components, but I'm dissatisfied with losing 25-33% performance writing it from this iterator style to imperative style.
I think it's possible there are other compiler optimizations done on the iterator/functional pattern version of the code that makes it faster than the imperative way I was going about things. I know you said you said you would be fine with this if there are compelling performance benefits, but I wanted to just make sure and ask again: are you okay with me keeping this as is? I'm unsure how to go about writing this in an imperative style that achieves the same performance as the iter/functional style. I can also show you the performance results across all the component size if that is something you are curious about and you want to know how big the effects are.
| if !is_sep_byte(*b) { | ||
| if *b == b'.' && !cur_dir_present { | ||
| cur_dir_present = true; | ||
| false | ||
| } else { | ||
| true | ||
| } | ||
| } else { | ||
| cur_dir_present = false; | ||
| false | ||
| } | ||
| }) { |
There was a problem hiding this comment.
Ditto here on just using a for loop.
| let (done, back) = match self.path.iter().rposition(|b| { | ||
| if !is_sep_byte(*b) { | ||
| if *b == b'.' && !cur_dir_present { | ||
| cur_dir_present = true; | ||
| false | ||
| } else { | ||
| true | ||
| } | ||
| } else { | ||
| cur_dir_present = false; | ||
| false | ||
| } | ||
| }) { |
There was a problem hiding this comment.
Aaand here too.
| } | ||
|
|
||
| /// Prefix-less `Components` equality | ||
| pub fn eq_components(mut left: Components<'_>, mut right: Components<'_>) -> bool { |
There was a problem hiding this comment.
I'm just going to leave a comment here indicating I haven't fully wrapped my head around this yet, so that I make sure to review before merging.
There was a problem hiding this comment.
All good, there's a lot that goes on in here. This may be glossed over on a quick glance, but do note that we're checking the mismatched byte from right to left since it's more common to notice the difference between paths from last components (e.g. "project_name/src/some_file.rs == project_name/src/other_file.rs").
I think the most confusing part that took me some time to debug to realize the issue was the last part checking if the mismatched byte from the left component or right component is a separator byte.
For example, take this case: "foobar/foo/bar" == "barfoo/bar"
The mismatched byte here is the "/" on left and "r" on right. It's important that we capture the full component to ensure that Iter::eq(left.rev(), right.rev()) does comparisons on the right mismatching components (i.e. the "foo" component on left and "barfoo" component on right). This means finding the next separator byte after the mismatched byte.
In "foobar/foo/bar" == "barfoo/bar", our next component position is done relative to where the next separator byte is from the mismatched byte in "barfoo/bar" not "foobar/foo/bar" as finding the next separator byte for "foobar/foo" will just lead us to the same spot it was at. Since we know that the next separator byte for the "r" in "barfoo/bar" is 4 bytes away, we can increment the mismatched byte index for "foobar/foo/bar" by the same amount as next separator byte in "barfoo/bar", slice both Components' path by that index and have Iter::eq(left.rev(), right.rev()) take it away from there.
| } | ||
| }; | ||
|
|
||
| if let Some(previous_sep) = left.path[..first_difference].iter().rposition(|&b| is_sep_byte(b)) |
There was a problem hiding this comment.
Gonna wrap my head around this later. Mostly just leaving an extra comment.
|
Okay, this time I've finally (mostly) reviewed this and am cautiously optimistic about the result, especially since you seem to have done a fair bit of benchmarking on this. Funny enough, the unprefixed path logic is the one that caught me the most off guard, and I'm not sure if that's just because you haven't put as much effort into that as the prefixed logic (which is harder to optimize) or if I just reviewed it second and was a bit tired from the first part of the review. Anyway, I am going to try and get back to reviewing this and properly closing out concerns from earlier in the review and taking a closer look to the benchmarks you've done. Thank you again for putting the work into this, and sorry it took so long to get to! |
This comment has been minimized.
This comment has been minimized.
63da5d7 to
0aa1bf8
Compare
|
Pending this try job, if it passes, I think the refactoring and minor updates to the prefixed version of @bors try jobs=aarch64-pc-windows-msvc,x86_64-pc-windows-gnu,x86_64-pc-windows-msvc |
This comment has been minimized.
This comment has been minimized.
Split `Components` iterator to prefixed and non-prefixed versions and optimize `Components` for non-prefix path based platforms try-job: aarch64-pc-windows-msvc try-job: x86_64-pc-windows-gnu try-job: x86_64-pc-windows-msvc
rust-bors
Bot
added
S-waiting-on-author
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for 86c1566 failed: CI. Failed job:
|
|
@bors try jobs=test-x86_64-msvc-,test-i686-msvc,test-aarch64-msvc- |
This comment has been minimized.
This comment has been minimized.
Split `Components` iterator to prefixed and non-prefixed versions and optimize `Components` for non-prefix path based platforms try-job: test-x86_64-msvc-* try-job: test-i686-msvc try-job: test-aarch64-msvc-*
|
@rustbot ready |
rustbot
added
S-waiting-on-review
This branch has not been deployed
9 participants
View all comments
This PR fixes #154521 specifically for Unix/non-prefixed platforms. By non-prefixed filepath platforms this includes:
all(target_vendor = "fortanix", target_env = "sgx")not(any(target_os = "cygwin", target_os = "uefi", target_os = "solid_asp3", target_os = "windows")(the default/fallback_path insys/path/mod.rs, which notably includes Unix platforms)For every other prefixed filepath platforms (notably Windows), the
Componentsiterator implementation remains largely unchanged (just some minor refactoring).The point of the non-prefixed
Componentsimplementation is to:Prefixcomponents that Windows/other prefixed-based filepath platform use as Unix-based platforms do not have those prefix components in their filepaths;Componentsiterator traversal algorithm as well as improvePathequality andPathcomparisonFrom benchmarking, I noticed that all the
Prefixchecking/returning code specific for Windows affects howComponentsperform on Unix, though to be fair, part of the reason that is the case is because thePrefixenum takes up 40 bytes to represent (which we shouldn't want Unix to take the consequence of handlingPrefix). There are other factors in the currentComponents::next/Components::next_backimplementation that could've been optimized better or written in a way that would enable the compiler to optimize that code better (e.g. autovectorization optimizations). It's certainly possible that the prefixed/currentComponentsimplementation could be enhanced to make performance better on Windows, but I will leave that to someone who can verifiably test and benchmark those changes on Windows.In terms of the optimizations I made for the non-prefixed
Components, a smaller one I could talk about first is that the size ofComponentshas been reduced. In the current/originalComponentswe have this:This overall takes up 64 bytes to represents
ComponentsNow, without the need to account for
Prefix, we can simplify theComponentsstruct to:This prefix-less
Componentstakes up 24 bytes to represent.Implementation-wise,
Components::nextandComponents::next_backtakes a similar approach to the originalComponentsiterator by subslicing thepathfield to present the left over unconsumed path. However, one of the key differences with thisComponents::nextandComponents::next_backis that we normalize away in between and trailing separator + cur dir bytes after parsing the component (e.g. "foo///bar", when we parse "foo" viaComponents::nextor "bar" viaComponents::next_back", the in between separator bytes will be subsliced from thepathfield in preparation for the nextComponents::next/Components::next_back). Normalizing away these bytes in preparation for the nextComponents::next/Components::next_backwere not done in the original implementation, which meant thatwhile !self.is_finishedloop in the original ran through multiple iterations to actually normalize away those separator bytes and find the next component to parse. Moreover, I relied on iterator methods to make that normalization process really fast, as it appears to be autovectorized and optimized really well by the compiler. Also noteworthy is that the optimization made to normalizing away in between/trailing separator/cur dir bytes using iterator methods enhances the performance ofComponents::as_pathsignificantly.As for
Pathequality andPathcomparison, which largely rely onComponentsunderneath the hood, there are many optimizations we introduce there to increase performance.Starting with
Pathequality, we check for the following:Paths when converted as anOsStrs are equal. If so, we can just return true immediately. Otherwise, we have to hinge onComponents(e.g. of "/foo" is equivalent to "/foo/").Componentsstatefield to determine if we're looking at an empty path (reading 1 byte via theState::Doneenum rather than directly looking at each path's length)Componentspath field and just rely onComponents::next_backto handle the rest on determining whether the two paths are equivalent or not.As for
Pathcomparison, we do the following:Ordering::Equalimmediately.Ordering::GreaterorOrdering::Lessas appropriate).Componentspath field and just rely onComponents::nextto handle the rest on determining whether to returnOrdering::Equal,Ordering::Less, orOrdering::Greater.Componentsimplementation, so this code/idea was just taken from there.All of these enhances the performance of
Pathequality and comparison significantly, which you can see from benchmarking results.Regarding benchmarking, I did all benchmarking and experimentation on this
Componentsimplementation within a separate repository. You can take a look here to see the raw and markdown formatted results and the methodology I chose to go about benchmarking thisComponentsiterator implementation.