From 322478d9a238cee3292131840bfd5e7cdd617f41 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Mon, 26 Apr 2021 09:35:31 +0200 Subject: [PATCH 1/4] ls: document flamegraph --- src/uu/ls/BENCHMARKING.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/uu/ls/BENCHMARKING.md b/src/uu/ls/BENCHMARKING.md index 84a0c3d84..c6568f879 100644 --- a/src/uu/ls/BENCHMARKING.md +++ b/src/uu/ls/BENCHMARKING.md @@ -32,3 +32,19 @@ This can also be used to compare with version of ls built before your changes to - Another thing to look at would be system calls count using strace (on linux) or equivalent on other operating systems. - Example: `strace -c target/release/coreutils ls -al -R tree` + +## Cargo Flamegraph + +With Cargo Flamegraph you can easily make a flamegraph of `ls`: +```bash +cargo flamegraph --cmd coreutils -- ls [additional parameters] +``` + +However, if the `-R` option is given, the output becomes pretty much useless due to recursion. We can fix this by merging all the direct recursive calls with `uniq`, below is a `bash` script that does this. +```bash +#!/bin/bash + +cargo build --release --no-default-features --features ls +perf record target/release/coreutils ls "$@" +perf script | uniq | inferno-collapse-perf | inferno-flamegraph > flamegraph.svg +``` \ No newline at end of file From e4c006949394ae9d98388f3b77bd00e2e6e089ba Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Mon, 26 Apr 2021 09:53:13 +0200 Subject: [PATCH 2/4] ls: remove path strip --- src/uu/ls/src/ls.rs | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 44c644849..b5e76b7d3 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1135,7 +1135,7 @@ fn list(locs: Vec, config: Config) -> i32 { } } sort_entries(&mut files, &config); - display_items(&files, None, &config, &mut out); + display_items(&files, &config, &mut out); sort_entries(&mut dirs, &config); for dir in dirs { @@ -1227,7 +1227,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) entries.append(&mut temp); - display_items(&entries, Some(&dir.p_buf), config, out); + display_items(&entries, config, out); if config.recursive { for e in entries @@ -1264,12 +1264,7 @@ fn pad_left(string: String, count: usize) -> String { format!("{:>width$}", string, width = count) } -fn display_items( - items: &[PathData], - strip: Option<&Path>, - config: &Config, - out: &mut BufWriter, -) { +fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter) { if config.format == Format::Long { let (mut max_links, mut max_size) = (1, 1); for item in items { @@ -1278,12 +1273,10 @@ fn display_items( max_size = size.max(max_size); } for item in items { - display_item_long(item, strip, max_links, max_size, config, out); + display_item_long(item, max_links, max_size, config, out); } } else { - let names = items - .iter() - .filter_map(|i| display_file_name(&i, strip, config)); + let names = items.iter().filter_map(|i| display_file_name(&i, config)); match (&config.format, config.width) { (Format::Columns, Some(width)) => { @@ -1355,7 +1348,6 @@ use uucore::fs::display_permissions; fn display_item_long( item: &PathData, - strip: Option<&Path>, max_links: usize, max_size: usize, config: &Config, @@ -1363,7 +1355,7 @@ fn display_item_long( ) { let md = match item.md() { None => { - let filename = get_file_name(&item.p_buf, strip); + let filename = get_file_name(&item.p_buf); show_error!("could not show file: {}", filename); return; } @@ -1407,7 +1399,7 @@ fn display_item_long( // unwrap is fine because it fails when metadata is not available // but we already know that it is because it's checked at the // start of the function. - display_file_name(&item, strip, config).unwrap().contents, + display_file_name(&item, config).unwrap().contents, ); } @@ -1558,15 +1550,11 @@ fn display_file_type(file_type: FileType) -> char { } } -fn get_file_name(name: &Path, strip: Option<&Path>) -> String { - let mut name = match strip { - Some(prefix) => name.strip_prefix(prefix).unwrap_or(name), - None => name, - }; - if name.as_os_str().is_empty() { - name = Path::new("."); - } - name.to_string_lossy().into_owned() +fn get_file_name(name: &Path) -> String { + name.file_name() + .unwrap_or(name.iter().last().unwrap()) + .to_string_lossy() + .into() } #[cfg(unix)] @@ -1605,8 +1593,8 @@ fn classify_file(path: &PathData) -> Option { } } -fn display_file_name(path: &PathData, strip: Option<&Path>, config: &Config) -> Option { - let mut name = escape_name(get_file_name(&path.p_buf, strip), &config.quoting_style); +fn display_file_name(path: &PathData, config: &Config) -> Option { + let mut name = escape_name(get_file_name(&path.p_buf), &config.quoting_style); #[cfg(unix)] { From 4023e40174485fe7e5bf514c761774d484f8acc2 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Mon, 26 Apr 2021 18:03:56 +0200 Subject: [PATCH 3/4] ls: further reduce OsStr -> String conversions --- src/uu/ls/src/ls.rs | 16 +++++----------- src/uu/ls/src/quoting_style.rs | 10 +++++----- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index b5e76b7d3..1cebeb38a 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1056,7 +1056,9 @@ impl PathData { ) -> Self { let name = p_buf .file_name() - .map_or(String::new(), |s| s.to_string_lossy().into_owned()); + .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) + .to_string_lossy() + .into_owned(); let must_dereference = match &config.dereference { Dereference::All => true, Dereference::Args => command_line, @@ -1355,8 +1357,7 @@ fn display_item_long( ) { let md = match item.md() { None => { - let filename = get_file_name(&item.p_buf); - show_error!("could not show file: {}", filename); + show_error!("could not show file: {}", &item.p_buf.display()); return; } Some(md) => md, @@ -1550,13 +1551,6 @@ fn display_file_type(file_type: FileType) -> char { } } -fn get_file_name(name: &Path) -> String { - name.file_name() - .unwrap_or(name.iter().last().unwrap()) - .to_string_lossy() - .into() -} - #[cfg(unix)] fn file_is_executable(md: &Metadata) -> bool { // Mode always returns u32, but the flags might not be, based on the platform @@ -1594,7 +1588,7 @@ fn classify_file(path: &PathData) -> Option { } fn display_file_name(path: &PathData, config: &Config) -> Option { - let mut name = escape_name(get_file_name(&path.p_buf), &config.quoting_style); + let mut name = escape_name(&path.file_name, &config.quoting_style); #[cfg(unix)] { diff --git a/src/uu/ls/src/quoting_style.rs b/src/uu/ls/src/quoting_style.rs index 49456fc22..01bffa7ec 100644 --- a/src/uu/ls/src/quoting_style.rs +++ b/src/uu/ls/src/quoting_style.rs @@ -171,7 +171,7 @@ impl Iterator for EscapedChar { } } -fn shell_without_escape(name: String, quotes: Quotes, show_control_chars: bool) -> (String, bool) { +fn shell_without_escape(name: &str, quotes: Quotes, show_control_chars: bool) -> (String, bool) { let mut must_quote = false; let mut escaped_str = String::with_capacity(name.len()); @@ -201,7 +201,7 @@ fn shell_without_escape(name: String, quotes: Quotes, show_control_chars: bool) (escaped_str, must_quote) } -fn shell_with_escape(name: String, quotes: Quotes) -> (String, bool) { +fn shell_with_escape(name: &str, quotes: Quotes) -> (String, bool) { // We need to keep track of whether we are in a dollar expression // because e.g. \b\n is escaped as $'\b\n' and not like $'b'$'n' let mut in_dollar = false; @@ -249,7 +249,7 @@ fn shell_with_escape(name: String, quotes: Quotes) -> (String, bool) { (escaped_str, must_quote) } -pub(super) fn escape_name(name: String, style: &QuotingStyle) -> String { +pub(super) fn escape_name(name: &str, style: &QuotingStyle) -> String { match style { QuotingStyle::Literal { show_control } => { if !show_control { @@ -257,7 +257,7 @@ pub(super) fn escape_name(name: String, style: &QuotingStyle) -> String { .flat_map(|c| EscapedChar::new_literal(c).hide_control()) .collect() } else { - name + name.into() } } QuotingStyle::C { quotes } => { @@ -354,7 +354,7 @@ mod tests { fn check_names(name: &str, map: Vec<(&str, &str)>) { assert_eq!( map.iter() - .map(|(_, style)| escape_name(name.to_string(), &get_style(style))) + .map(|(_, style)| escape_name(name, &get_style(style))) .collect::>(), map.iter() .map(|(correct, _)| correct.to_string()) From 35838dc8a9f6f1e8d2dfb65932f92d3b57f2158b Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Mon, 26 Apr 2021 18:36:15 +0200 Subject: [PATCH 4/4] ls: document hyperfine script --- src/uu/ls/BENCHMARKING.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/BENCHMARKING.md b/src/uu/ls/BENCHMARKING.md index c6568f879..852220496 100644 --- a/src/uu/ls/BENCHMARKING.md +++ b/src/uu/ls/BENCHMARKING.md @@ -26,7 +26,15 @@ Example: `hyperfine --warmup 2 "target/release/coreutils ls -al -R tree > /dev/n `hyperfine --warmup 2 "target/release/coreutils ls -al -R tree > /dev/null" "ls -al -R tree > /dev/null"` (This assumes GNU ls is installed as `ls`) -This can also be used to compare with version of ls built before your changes to ensure your change does not regress this +This can also be used to compare with version of ls built before your changes to ensure your change does not regress this. + +Here is a `bash` script for doing this comparison: +```bash +#!/bin/bash +cargo build --no-default-features --features ls --release +args="$@" +hyperfine "ls $args" "target/release/coreutils ls $args" +``` ## Checking system call count @@ -43,7 +51,6 @@ cargo flamegraph --cmd coreutils -- ls [additional parameters] However, if the `-R` option is given, the output becomes pretty much useless due to recursion. We can fix this by merging all the direct recursive calls with `uniq`, below is a `bash` script that does this. ```bash #!/bin/bash - cargo build --release --no-default-features --features ls perf record target/release/coreutils ls "$@" perf script | uniq | inferno-collapse-perf | inferno-flamegraph > flamegraph.svg