Introduce a specialization trait for ReadDir for most platforms

[asder8215]

For most platforms, we can achieve a zero cost way of constructing a ReadDir via std::fs::read_dir when passing in a PathBuf or something that can be converted into a PathBuf (in a zero cost fashion) through a specialization trait (as std::fs::read_dir takes in an AsRef<Path>). We can use a specialization trait via what's offered by min_specialization feature.

The only platforms, as far as I can tell, that cannot achieve a zero cost way (at least conveniently) of constructing a ReadDir with an owned path is uefi since it seems like it needs to create an absolute path from a path reference (which creates and allocates for a PathBuf anyways). On Windows, this specialization trait gets rid of let root = p.into_path_buf(), reducing one PathBuf allocation, but there's already another PathBuf allocation that occurs through pushing a "*" on the path. Of course, on unsupported and vexos, since readdir is unsupported on those platforms, I didn't introduce the trait there.

I also tried out making a mock ReadDir with the min_specialization feature on rust playground and can see that an owned path and a path reference uses different functions of the specialization trait accordingly.

r? @Mark-Simulacrum since this goes hand in hand with the remove_dir_all_recursive PR I made that he's a reviewer of.

[asder8215]

This seems to be unnecessary since internally uefi_fs::File::from_path already uses crate::path::absolute(p) on the given path reference.

View changes since the review

[bjorn3]

Is this allocation actually that expensive compared to the allocation that need to be done for every returned directory entry to warrant a ton of extra code?

[asder8215]

Is this allocation actually that expensive compared to the allocation that need to be done for every returned directory entry to warrant a ton of extra code?

You're right to point this out since each DirEntry produced by ReadDir already performs allocation to store the entry name; in most cases, I'd presume that this DirEntry allocation dominates over constructing a ReadDir on each nested directory.

However, I guess I've been thinking about why not just potentially improve the performance and usage of building ReadDir even if it's slightly (though there is code maintenance and churn here to think about of course)? If someone's building a ReadDir iterator using a PathBuf be it initially or through a nested directory, I don't know why they should pay the cost of allocating for another PathBuf if they gave one directly. I think about this blog post I read before on how it's an anti-pattern to use AsRef if you're doing something like let x = param.as_ref().to_owned();, which should use Into instead, and I agreed with its point here. I just thought it was a little disappointing that a function like this, which allocates for a given path regardless if it's owned or not, exists in the standard library (which I assume we can't turn this into Into due to breaking changes). When I read about specialization being possibly usable here from the libs-team notes on my ACP for ReadDir, I just figured it was an opportunity to make this similar to how Into would work for this function.

The effects of how big this change makes for performance is something that I should benchmark. The remove_dir_all_recursive for unix/uefi (less so on uefi since I couldn't introduce the trait there) is one of those functions that I noticed could have been improved by this change since it could've used less memory and avoid an allocation on each recursive call (and it seems to be used pretty often within the rust repo).

[Mark-Simulacrum]

I don't think this complexity is warranted, especially given it needs specialization which carries some correctness risk too. But I'll r? @the8472 if you want to take review here.

[the8472]

So I get the motivation, but the question is how much of a win is this? So yeah, benchmarks needed. I suspect the difference would be small, but I could be wrong (maybe for long paths?).

But there's another angle. There's work on directory handles (#120426) which means we'll soon have to think about a ReadDir without a path being immediately available.
So as part of that effort it might make sense to make the PathBuf inside ReadDir optional. Then ReadDir instances constructed from a Dir wouldn't pay the allocation cost, and those could be used in performance-sensitive code.
And the given example (remove_dir_all_recursive) should be using Dir too.

[asder8215]

So I get the motivation, but the question is how much of a win is this? So yeah, benchmarks needed. I suspect the difference would be small, but I could be wrong (maybe for long paths?).

I wrote some benchmark code for this in this repo. Some of the error-related stuff I extracted or implemented manually inside spec_read_dir.rs (may need a bit of verification if all of this looks correct or can be used to provide accurate measurements). I benchmarked the time it takes to recursively open the current and nested directories and check each dir entry.

What I benchmarked were the following (num of nested directories and files were found through: find "path" -type d | wc -l//find "path" -type f | wc -l

This is from my /home/asder8215/games_file directory that has 2 nested directories and total of 25 files. (max depth 2)

Spec Read Dir           time:   [20.048 µs 20.056 µs 20.065 µs]
Found 11 outliers among 100 measurements (11.00%)
  8 (8.00%) high mild
  3 (3.00%) high severe

Std Read Dir            time:   [20.475 µs 20.516 µs 20.563 µs]
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe

This is from my /home/asder8215/Androids directory which contains 16239 directories and a total of 51343 (max depth of 16)

Spec Read Dir           time:   [143.39 ms 144.02 ms 144.68 ms]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Std Read Dir            time:   [145.47 ms 146.32 ms 147.24 ms]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

This is from my home directory (though I set the Criterion sampling size to 10 since it takes a while to run this), which it contains 278652 directories, and 2515101 files at this current time.

Spec Read Dir
                        time:   [2.7354 s 2.7414 s 2.7483 s]
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild

Std Read Dir
                        time:   [2.7959 s 2.8005 s 2.8058 s]

Yeah, the performance gain is pretty small (~2.3% diff on games_file and home directory, and ~1.6% diff for my Android folder). Not certain if that small improvement is considered reasonable to introduce spec on ReadDir.

[asder8215]

But there's another angle. There's work on directory handles (#120426) which means we'll soon have to think about a ReadDir without a path being immediately available.
So as part of that effort it might make sense to make the PathBuf inside ReadDir optional. Then ReadDir instances constructed from a Dir wouldn't pay the allocation cost, and those could be used in performance-sensitive code.
And the given example (remove_dir_all_recursive) should be using Dir too.

Got you. I'm going to need a bit of time to look at the PR and understand the context behind this. However, that makes sense to make PathBuf optional to avoid paying the allocation cost.