Make it easy to add diffing of output of arbitrary profilers (demonstrate for dep-graph)

Author: oli-obk

collector/src/bin/collector.rs

@@ -132,21 +132,16 @@ fn check_measureme_installed() -> Result<(), String> {
     }
 }
 
-fn check_installed(name: &str) -> anyhow::Result<()> {
-    if !is_installed(name) {
-        anyhow::bail!("`{}` is not installed but must be", name);
-    }
-    Ok(())
-}
-
-fn generate_cachegrind_diffs(
+#[allow(clippy::too_many_arguments)]
+fn generate_diffs(
     id1: &str,
     id2: &str,
     out_dir: &Path,
     benchmarks: &[Benchmark],
     profiles: &[Profile],
     scenarios: &[Scenario],
     errors: &mut BenchmarkErrors,
+    profiler: &Profiler,
 ) -> Vec<PathBuf> {
     let mut annotated_diffs = Vec::new();
     for benchmark in benchmarks {
@@ -166,22 +161,28 @@ fn generate_cachegrind_diffs(
             }) {
                 let filename = |prefix, id| {
                     format!(
-                        "{}-{}-{}-{:?}-{}",
-                        prefix, id, benchmark.name, profile, scenario
+                        "{}-{}-{}-{:?}-{}{}",
+                        prefix,
+                        id,
+                        benchmark.name,
+                        profile,
+                        scenario,
+                        profiler.postfix()
                     )
                 };
                 let id_diff = format!("{}-{}", id1, id2);
-                let cgout1 = out_dir.join(filename("cgout", id1));
-                let cgout2 = out_dir.join(filename("cgout", id2));
-                let cgann_diff = out_dir.join(filename("cgann-diff", &id_diff));
+                let prefix = profiler.prefix();
+                let left = out_dir.join(filename(prefix, id1));
+                let right = out_dir.join(filename(prefix, id2));
+                let output = out_dir.join(filename(&format!("{prefix}-diff"), &id_diff));
 
-                if let Err(e) = cachegrind_diff(&cgout1, &cgout2, &cgann_diff) {
+                if let Err(e) = profiler.diff(&left, &right, &output) {
                     errors.incr();
                     eprintln!("collector error: {:?}", e);
                     continue;
                 }
 
-                annotated_diffs.push(cgann_diff);
+                annotated_diffs.push(output);
             }
         }
     }
@@ -1145,29 +1146,25 @@ fn main_result() -> anyhow::Result<i32> {
                 let id1 = get_toolchain_and_profile(local.rustc.as_str(), "1")?;
                 let id2 = get_toolchain_and_profile(rustc2.as_str(), "2")?;
 
-                if profiler == Profiler::Cachegrind {
-                    check_installed("valgrind")?;
-                    check_installed("cg_annotate")?;
-
-                    let diffs = generate_cachegrind_diffs(
-                        &id1,
-                        &id2,
-                        &out_dir,
-                        &benchmarks,
-                        profiles,
-                        scenarios,
-                        &mut errors,
-                    );
-                    if let [diff] = &diffs[..] {
-                        let short = out_dir.join("cgann-diff-latest");
-                        std::fs::copy(diff, &short).expect("copy to short path");
-                        eprintln!("Original diff at: {}", diff.to_string_lossy());
-                        eprintln!("Short path: {}", short.to_string_lossy());
-                    } else {
-                        eprintln!("Diffs:");
-                        for diff in diffs {
-                            eprintln!("{}", diff.to_string_lossy());
-                        }
+                let diffs = generate_diffs(
+                    &id1,
+                    &id2,
+                    &out_dir,
+                    &benchmarks,
+                    profiles,
+                    scenarios,
+                    &mut errors,
+                    &profiler,
+                );
+                if let [diff] = &diffs[..] {
+                    let short = out_dir.join(format!("{}-diff-latest", profiler.prefix()));
+                    std::fs::copy(diff, &short).expect("copy to short path");
+                    eprintln!("Original diff at: {}", diff.to_string_lossy());
+                    eprintln!("Short path: {}", short.to_string_lossy());
+                } else {
+                    eprintln!("Diffs:");
+                    for diff in diffs {
+                        eprintln!("{}", diff.to_string_lossy());
                     }
                 }
             } else {

collector/src/compile/execute/profiler.rs

@@ -1,6 +1,7 @@
 use crate::compile::execute::{PerfTool, ProcessOutputData, Processor, Retry};
-use crate::utils;
-use crate::utils::cachegrind::cachegrind_annotate;
+use crate::utils::cachegrind::{cachegrind_annotate, cachegrind_diff};
+use crate::utils::diff::run_diff;
+use crate::utils::{self};
 use anyhow::Context;
 use std::collections::HashMap;
 use std::fs::File;
@@ -51,6 +52,42 @@ impl Profiler {
                 | Profiler::DepGraph
         )
     }
+
+    /// A file prefix added to all files of this profiler.
+    pub fn prefix(&self) -> &'static str {
+        use Profiler::*;
+        match self {
+            Cachegrind => "cgout",
+            DepGraph => "dep-graph",
+
+            SelfProfile | PerfRecord | Oprofile | Samply | Callgrind | Dhat | DhatCopy | Massif
+            | Bytehound | Eprintln | LlvmLines | MonoItems | LlvmIr => "",
+        }
+    }
+
+    /// A postfix added to the file that gets diffed.
+    pub fn postfix(&self) -> &'static str {
+        use Profiler::*;
+        match self {
+            Cachegrind => "",
+            DepGraph => ".txt",
+
+            SelfProfile | PerfRecord | Oprofile | Samply | Callgrind | Dhat | DhatCopy | Massif
+            | Bytehound | Eprintln | LlvmLines | MonoItems | LlvmIr => "",
+        }
+    }
+
+    /// Actually perform the diff
+    pub fn diff(&self, left: &Path, right: &Path, output: &Path) -> anyhow::Result<()> {
+        use Profiler::*;
+        match self {
+            Cachegrind => cachegrind_diff(left, right, output),
+            DepGraph => run_diff(left, right, output),
+
+            SelfProfile | PerfRecord | Oprofile | Samply | Callgrind | Dhat | DhatCopy | Massif
+            | Bytehound | Eprintln | LlvmLines | MonoItems | LlvmIr => Ok(()),
+        }
+    }
 }
 
 pub struct ProfileProcessor<'a> {

collector/src/utils/cachegrind.rs

@@ -1,4 +1,3 @@
-use crate::utils::is_installed;
 use crate::utils::mangling::demangle_file;
 use anyhow::Context;
 use std::fs::File;
@@ -7,6 +6,8 @@ use std::path::Path;
 use std::process::{Command, Stdio};
 use std::{fs, io};
 
+use super::check_installed;
+
 /// Annotate and demangle the output of Cachegrind using the `cg_annotate` tool.
 pub fn cachegrind_annotate(
     input: &Path,
@@ -70,6 +71,7 @@ pub fn cachegrind_annotate(
 
 /// Creates a diff between two `cgout` files, and annotates the diff.
 pub fn cachegrind_diff(cgout_a: &Path, cgout_b: &Path, output: &Path) -> anyhow::Result<()> {
+    check_installed("valgrind")?;
     let cgout_diff = tempfile::NamedTempFile::new()?.into_temp_path();
 
     run_cg_diff(cgout_a, cgout_b, &cgout_diff).context("Cannot run cg_diff")?;
@@ -80,9 +82,7 @@ pub fn cachegrind_diff(cgout_a: &Path, cgout_b: &Path, output: &Path) -> anyhow:
 
 /// Compares two Cachegrind output files using `cg_diff` and writes the result to `path`.
 fn run_cg_diff(cgout1: &Path, cgout2: &Path, path: &Path) -> anyhow::Result<()> {
-    if !is_installed("cg_diff") {
-        anyhow::bail!("`cg_diff` not installed.");
-    }
+    check_installed("cg_diff")?;
     let status = Command::new("cg_diff")
         .arg(r"--mod-filename=s/\/rustc\/[^\/]*\///")
         .arg("--mod-funcname=s/[.]llvm[.].*//")
@@ -100,6 +100,7 @@ fn run_cg_diff(cgout1: &Path, cgout2: &Path, path: &Path) -> anyhow::Result<()>
 
 /// Postprocess Cachegrind output file and writes the result to `path`.
 fn annotate_diff(cgout: &Path, path: &Path) -> anyhow::Result<()> {
+    check_installed("cg_annotate")?;
     let status = Command::new("cg_annotate")
         .arg("--show-percs=no")
         .arg(cgout)

collector/src/utils/diff.rs

@@ -0,0 +1,23 @@
+use std::{
+    fs::File,
+    path::Path,
+    process::{Command, Stdio},
+};
+
+use anyhow::Context as _;
+
+use super::check_installed;
+
+/// Compares two text files using `diff` and writes the result to `path`.
+pub fn run_diff(left: &Path, right: &Path, out_file: &Path) -> anyhow::Result<()> {
+    check_installed("diff")?;
+    Command::new("diff")
+        .arg(left)
+        .arg(right)
+        .stderr(Stdio::inherit())
+        .stdout(File::create(out_file)?)
+        .status()
+        .context("failed to run `diff`")?;
+
+    Ok(())
+}

collector/src/utils/mod.rs

@@ -2,6 +2,7 @@ use std::future::Future;
 use std::process::{Command, Stdio};
 
 pub mod cachegrind;
+pub mod diff;
 pub mod fs;
 pub mod git;
 pub mod mangling;
@@ -23,3 +24,11 @@ pub fn is_installed(name: &str) -> bool {
         .status()
         .is_ok()
 }
+
+/// Checks if the given binary can be executed and bails otherwise.
+pub fn check_installed(name: &str) -> anyhow::Result<()> {
+    if !is_installed(name) {
+        anyhow::bail!("`{}` is not installed but must be", name);
+    }
+    Ok(())
+}