Add benchmarking for rm
#5187
Add benchmarking for rm
#5187
Conversation
Add benchmarking script and guide for `rm`, mostly copied from `ls` benchmarking guide. Tested `rm` using `jwalk` instead of `walkdir`, and saw a slight performance regression, if any change.
src/uu/rm/BENCHMARKING.md
Outdated
| ```shell | ||
| #!/bin/bash | ||
| cargo build --release --no-default-features --features rm | ||
| perf record target/release/coreutils rm "$@" |
There was a problem hiding this comment.
you might want to try https://github.com/mstange/samply
There was a problem hiding this comment.
That's interesting, just tried samply and rm -rf immediately enters into std::sys::unix::fs::remove_dir_impl::remove_dir_all_recursive, which is then 100% of the rest of the samples.
This is likely why rm -rf performance isn't improved by jwalk at all. We might see improvements if we were manually overriding the remove_dir_all_recursive, which appears to be causing other issues with GNU compatibility.
There was a problem hiding this comment.
This test was again using the linux kernel with rm -rf
Add samply to the sampling options for `rm` benchmarking.
|
Only check failing is spelling due to "samply" and "flamegraphs". Is there a way to add words to a spelling ignore dictionary? |
|
You can add a HTML comment, see for example https://raw.githubusercontent.com/uutils/coreutils/main/CONTRIBUTING.md |
|
Oh cakebaker was just a bit quicker than me 😄 I think we could also add these words to the list here: https://github.com/uutils/coreutils/blob/main/.vscode/cspell.dictionaries/shell.wordlist.txt#L79 We might need them in other files too. |
Add `samply` and `flamegraph` to spell ignore lists.
|
Thank you both! Just added those, hopefully will fix the next run |
|
Looks like the style and lint changes are failing due to files that aren't changed. This could be related to the release of Rust 1.72 which introduced some new clippy warnings. Would it be fine to merge these changes as they're unrelated? |
|
thanks |
Add benchmarking script and guide for
rm, mostly copied fromlsbenchmarking guide. Testedrmusingjwalkinstead ofwalkdir, and saw a slight performance regression, if any change.