Merge pull request #2135 from tertsdiepraam/ls/flamegraph

`ls`: file name handling performance improvements

src/uu/ls/BENCHMARKING.md

@@ -26,9 +26,32 @@ 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
 
 - 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 
+```

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,
@@ -1135,7 +1137,7 @@ fn list(locs: Vec<String>, 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 +1229,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>)
 
     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 +1266,7 @@ fn pad_left(string: String, count: usize) -> String {
     format!("{:>width$}", string, width = count)
 }
 
-fn display_items(
+fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout>) {
-    items: &[PathData],
-    strip: Option<&Path>,
-    config: &Config,
-    out: &mut BufWriter<Stdout>,
-) {
     if config.format == Format::Long {
         let (mut max_links, mut max_size) = (1, 1);
         for item in items {
@@ -1278,12 +1275,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
+        let names = items.iter().filter_map(|i| display_file_name(&i, config));
-            .iter()
-            .filter_map(|i| display_file_name(&i, strip, config));
 
         match (&config.format, config.width) {
             (Format::Columns, Some(width)) => {
@@ -1355,7 +1350,6 @@ use uucore::fs::display_permissions;
 
 fn display_item_long(
     item: &PathData,
-    strip: Option<&Path>,
     max_links: usize,
     max_size: usize,
     config: &Config,
@@ -1363,8 +1357,7 @@ fn display_item_long(
 ) {
     let md = match item.md() {
         None => {
-            let filename = get_file_name(&item.p_buf, strip);
+            show_error!("could not show file: {}", &item.p_buf.display());
-            show_error!("could not show file: {}", filename);
             return;
         }
         Some(md) => md,
@@ -1407,7 +1400,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,17 +1551,6 @@ 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()
-}
-
 #[cfg(unix)]
 fn file_is_executable(md: &Metadata) -> bool {
     // Mode always returns u32, but the flags might not be, based on the platform
@@ -1605,8 +1587,8 @@ fn classify_file(path: &PathData) -> Option<char> {
     }
 }
 
-fn display_file_name(path: &PathData, strip: Option<&Path>, config: &Config) -> Option<Cell> {
+fn display_file_name(path: &PathData, config: &Config) -> Option<Cell> {
-    let mut name = escape_name(get_file_name(&path.p_buf, strip), &config.quoting_style);
+    let mut name = escape_name(&path.file_name, &config.quoting_style);
 
     #[cfg(unix)]
     {

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::<Vec<String>>(),
             map.iter()
                 .map(|(correct, _)| correct.to_string())