Add support for --reverse option

src/bin/flamegraph.rs

@@ -119,14 +119,11 @@ struct Opt {
     /// Don't sort the input lines.
     /// If you set this flag you need to be sure your
     /// input stack lines are already sorted.
-    #[structopt(long = "no-sort")]
+    #[structopt(name = "no-sort", long = "no-sort")]
     no_sort: bool,
 
     /// Generate stack-reversed flame graph.
-    /// Note that stack lines must always be sorted
-    /// after reversing the stacks so the no-sort
-    /// flag will be ignored.
-    #[structopt(long = "reverse")]
+    #[structopt(long = "reverse", conflicts_with = "no-sort")]
     reverse: bool,
 
     /// Don't include static JavaScript in flame graph.

src/flamegraph/merge.rs

@@ -199,33 +199,58 @@ where
 
 // Parse and remove the number of samples from the end of a line.
 fn parse_nsamples(line: &mut &str, stripped_fractional_samples: &mut bool) -> Option<usize> {
-    let samplesi = line.rfind(' ')?;
-    let mut samples = &line[(samplesi + 1)..];
-
-    // Strip fractional part (if any);
-    // foobar 1.klwdjlakdj
-    //
-    // The Perl version keeps the fractional part but this can be problematic
-    // because of cumulative floating point errors. Instead we recommend to
-    // use the --factor option. See https://github.com/brendangregg/FlameGraph/pull/18
-    if let Some(doti) = samples.find('.') {
-        if !samples[..doti]
-            .chars()
-            .chain(samples[doti + 1..].chars())
-            .all(|c| c.is_digit(10))
-        {
-            return None;
-        }
+    if let Some((samplesi, doti)) = rfind_samples(line) {
+        let mut samples = &line[samplesi..];
+        // Strip fractional part (if any);
+        // foobar 1.klwdjlakdj
+        //
+        // The Perl version keeps the fractional part but this can be problematic
+        // because of cumulative floating point errors. Instead we recommend to
+        // use the --factor option. See https://github.com/brendangregg/FlameGraph/pull/18
+        //
         // Warn if we're stripping a non-zero fractional part, but only the first time.
-        if !*stripped_fractional_samples && !samples[doti + 1..].chars().all(|c| c == '0') {
+        if !*stripped_fractional_samples
+            && doti < samples.len() - 1
+            && !samples[doti + 1..].chars().all(|c| c == '0')
+        {
             *stripped_fractional_samples = true;
             warn!("The input data has fractional sample counts that will be truncated to integers. If you need to retain the extra precision you can scale up the sample data and use the --factor option to scale it back down.");
         }
         samples = &samples[..doti];
+        let nsamples = samples.parse::<usize>().ok()?;
+        // remove nsamples part we just parsed from line
+        *line = line[..samplesi].trim_end();
+        Some(nsamples)
+    } else {
+        None
     }
+}
 
-    let nsamples = samples.parse::<usize>().ok()?;
-    // remove nsamples part we just parsed from line
-    *line = line[..samplesi].trim_end();
-    Some(nsamples)
+// Tries to find a sample count at the end of a line.
+//
+// On success, the first value of the returned tuple will be the index to the sample count.
+// If the sample count is fractional, the second value will be the offset of the dot within
+// the sample count.
+// If the sample count is not fractional, the second value returned is the offset
+// to the last digit in the sample count.
+//
+// If no sample count is found, `None` will be returned.
+pub(super) fn rfind_samples(line: &str) -> Option<(usize, usize)> {
+    let samplesi = line.rfind(' ')? + 1;
+    let samples = &line[samplesi..];
+    if let Some(doti) = samples.find('.') {
+        if samples[..doti]
+            .chars()
+            .chain(samples[doti + 1..].chars())
+            .all(|c| c.is_digit(10))
+        {
+            Some((samplesi, doti))
+        } else {
+            None
+        }
+    } else if !samples.chars().all(|c| c.is_digit(10)) {
+        None
+    } else {
+        Some((samplesi, line.len() - samplesi))
+    }
 }

src/flamegraph/mod.rs

@@ -157,7 +157,7 @@ pub struct Options<'a> {
 
     /// Generate stack-reversed flame graph.
     ///
-    /// Note that stack lines must always be sorted after reversing the stacks so the `no-sort`
+    /// Note that stack lines must always be sorted after reversing the stacks so the `no_sort`
     /// option will be ignored.
     pub reverse_stack_order: bool,
 
@@ -345,21 +345,20 @@ where
     W: Write,
 {
     let mut reversed = StrStack::new();
-    let (mut frames, time, ignored, delta_max) = if !opt.no_sort && !opt.reverse_stack_order {
-        // Sort lines by default.
-        let mut lines: Vec<&str> = lines.into_iter().collect();
-        lines.sort_unstable();
-        merge::frames(lines)?
-    } else if opt.reverse_stack_order {
+    let (mut frames, time, ignored, delta_max) = if opt.reverse_stack_order {
         if opt.no_sort {
-            warn!("Input lines are always sorted when using --reverse. The --no-sort flag is being ignored.");
+            warn!("Input lines are always sorted when `reverse_stack_order` is `true`. The `no_sort` option is being ignored.");
         }
         // Reverse order of stacks and sort.
         let mut stack = String::new();
         for line in lines {
             stack.clear();
-            let samples_idx = rfind_samples(line).unwrap_or_else(|| line.len());
-            let samples_idx = rfind_samples(&line[..samples_idx - 1]).unwrap_or(samples_idx);
+            let samples_idx = merge::rfind_samples(line)
+                .map(|(i, _)| i)
+                .unwrap_or_else(|| line.len());
+            let samples_idx = merge::rfind_samples(&line[..samples_idx - 1])
+                .map(|(i, _)| i)
+                .unwrap_or(samples_idx);
             for (i, func) in line[..samples_idx].trim().split(';').rev().enumerate() {
                 if i != 0 {
                     stack.push(';');
@@ -373,9 +372,14 @@ where
         let mut reversed: Vec<&str> = reversed.iter().collect();
         reversed.sort_unstable();
         merge::frames(reversed)?
-    } else {
+    } else if opt.no_sort {
         // Lines don't need sorting.
         merge::frames(lines)?
+    } else {
+        // Sort lines by default.
+        let mut lines: Vec<&str> = lines.into_iter().collect();
+        lines.sort_unstable();
+        merge::frames(lines)?
     };
 
     if ignored != 0 {
@@ -764,22 +768,3 @@ fn filled_rectangle<W: Write>(
     }
     svg.write_event(&cache_rect)
 }
-
-// Tries to find a sample count at the end of a line and returns its index.
-fn rfind_samples(line: &str) -> Option<usize> {
-    let samplesi = line.rfind(' ')?;
-    let samples = &line[samplesi + 1..];
-    if let Some(doti) = samples.find('.') {
-        if !samples[..doti]
-            .chars()
-            .chain(samples[doti + 1..].chars())
-            .all(|c| c.is_digit(10))
-        {
-            return None;
-        }
-    } else if !samples.chars().all(|c| c.is_digit(10)) {
-        return None;
-    }
-
-    Some(samplesi + 1)
-}

tests/flamegraph.rs

@@ -683,7 +683,7 @@ fn flamegraph_should_warn_about_no_sort_when_reversing_stack_ordering() {
         |captured_logs| {
             let nwarnings = captured_logs
             .into_iter()
-            .filter(|log| log.body == "Input lines are always sorted when using --reverse. The --no-sort flag is being ignored." && log.level == Level::Warn)
+            .filter(|log| log.body == "Input lines are always sorted when `reverse_stack_order` is `true`. The `no_sort` option is being ignored." && log.level == Level::Warn)
             .count();
             assert_eq!(
                 nwarnings, 1,