Merge pull request #2031 from oli-obk/push-wtzmtssxzmxz

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

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,32 +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,
-                    );
-                    match diffs.len().cmp(&1) {
-                        Ordering::Equal => {
-                            let short = out_dir.join("cgann-diff-latest");
-                            std::fs::copy(&diffs[0], &short).expect("copy to short path");
-                            eprintln!("Original diff at: {}", diffs[0].to_string_lossy());
-                            eprintln!("Short path: {}", short.to_string_lossy());
-                        }
-                        _ => {
-                            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/compare.rs

@@ -80,8 +80,8 @@ pub async fn compare_artifacts(
         None => select_artifact_id("base", &aids)?.to_string(),
     };
     aids.retain(|id| id != &base);
-    let modified = if aids.len() == 1 {
-        let new_modified = aids[0].clone();
+    let modified = if let [new_modified] = &aids[..] {
+        let new_modified = new_modified.clone();
         println!(
             "Only 1 artifact remains, automatically selecting: {}",
             new_modified

collector/src/compile/execute/profiler.rs

@@ -1,8 +1,10 @@
 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;
 use std::future::Future;
 use std::io::BufRead;
 use std::io::Write;
@@ -50,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> {
@@ -143,10 +181,8 @@ impl<'a> Processor for ProfileProcessor<'a> {
                     // Run `summarize`.
                     let mut summarize_cmd = Command::new("summarize");
                     summarize_cmd.arg("summarize").arg(&zsp_files_prefix);
-                    fs::write(
-                        summarize_file,
-                        summarize_cmd.output().context("summarize")?.stdout,
-                    )?;
+                    summarize_cmd.stdout(File::create(summarize_file)?);
+                    summarize_cmd.status().context("summarize")?;
 
                     // Run `flamegraph`.
                     let mut flamegraph_cmd = Command::new("flamegraph");
@@ -198,17 +234,19 @@ impl<'a> Processor for ProfileProcessor<'a> {
                         .arg("--debug-info")
                         .arg("--threshold")
                         .arg("0.5")
-                        .arg(&session_dir_arg);
-                    fs::write(oprep_file, op_report_cmd.output()?.stdout)?;
+                        .arg(&session_dir_arg)
+                        .stdout(File::create(oprep_file)?);
+                    op_report_cmd.status()?;
 
                     let mut op_annotate_cmd = Command::new("opannotate");
                     // Other possibly useful args: --assembly
                     op_annotate_cmd
                         .arg("--source")
                         .arg("--threshold")
                         .arg("0.5")
-                        .arg(&session_dir_arg);
-                    fs::write(opann_file, op_annotate_cmd.output()?.stdout)?;
+                        .arg(&session_dir_arg)
+                        .stdout(File::create(opann_file)?);
+                    op_annotate_cmd.status()?;
                 }
 
                 // Samply produces (via rustc-fake) a data file called
@@ -248,8 +286,9 @@ impl<'a> Processor for ProfileProcessor<'a> {
                     clg_annotate_cmd
                         .arg("--auto=yes")
                         .arg("--show-percs=yes")
-                        .arg(&clgout_file);
-                    fs::write(clgann_file, clg_annotate_cmd.output()?.stdout)?;
+                        .arg(&clgout_file)
+                        .stdout(File::create(clgann_file)?);
+                    clg_annotate_cmd.status()?;
                 }
 
                 // DHAT produces (via rustc-fake) a data file called `dhout`. We
@@ -371,13 +410,13 @@ impl<'a> Processor for ProfileProcessor<'a> {
                 Profiler::DepGraph => {
                     let tmp_file = filepath(data.cwd, "dep_graph.txt");
                     let output =
-                        filepath(self.output_dir, &out_file("dep-graph")).with_extension("txt");
+                        filepath(self.output_dir, &format!("{}.txt", out_file("dep-graph")));
 
                     fs::copy(tmp_file, output)?;
 
                     let tmp_file = filepath(data.cwd, "dep_graph.dot");
                     let output =
-                        filepath(self.output_dir, &out_file("dep-graph")).with_extension("dot");
+                        filepath(self.output_dir, &format!("{}.dot", out_file("dep-graph")));
 
                     // May not exist if not incremental, but then that's OK.
                     fs::copy(tmp_file, output)?;

collector/src/toolchain.rs

@@ -418,9 +418,10 @@ pub fn get_local_toolchain(
             .output()
             .context("failed to run `rustup which rustc`")?;
 
-        if !output.status.success() {
-            anyhow::bail!("did not manage to obtain toolchain {}", toolchain);
-        }
+        anyhow::ensure!(
+            output.status.success(),
+            "did not manage to obtain toolchain {toolchain}"
+        );
 
         let s = String::from_utf8(output.stdout)
             .context("failed to convert `rustup which rustc` output to utf8")?;

collector/src/utils/cachegrind.rs

@@ -1,11 +1,13 @@
-use crate::utils::is_installed;
 use crate::utils::mangling::demangle_file;
 use anyhow::Context;
+use std::fs::File;
 use std::io::{BufRead, Write};
 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,
@@ -61,13 +63,15 @@ pub fn cachegrind_annotate(
     cg_annotate_cmd
         .arg("--auto=yes")
         .arg("--show-percs=yes")
-        .arg(cgout_output);
-    fs::write(cgann_output, cg_annotate_cmd.output()?.stdout)?;
+        .arg(cgout_output)
+        .stdout(File::create(cgann_output)?);
+    cg_annotate_cmd.status()?;
     Ok(())
 }
 
 /// 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")?;
@@ -78,41 +82,34 @@ 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.");
-    }
-    let output = Command::new("cg_diff")
+    check_installed("cg_diff")?;
+    let status = Command::new("cg_diff")
         .arg(r"--mod-filename=s/\/rustc\/[^\/]*\///")
         .arg("--mod-funcname=s/[.]llvm[.].*//")
         .arg(cgout1)
         .arg(cgout2)
         .stderr(Stdio::inherit())
-        .output()
+        .stdout(File::create(path)?)
+        .status()
         .context("failed to run `cg_diff`")?;
 
-    if !output.status.success() {
-        anyhow::bail!("failed to generate cachegrind diff");
-    }
-
-    fs::write(path, output.stdout).context("failed to write `cg_diff` output")?;
+    anyhow::ensure!(status.success(), "failed to generate cachegrind diff");
 
     Ok(())
 }
 
 /// Postprocess Cachegrind output file and writes the result to `path`.
 fn annotate_diff(cgout: &Path, path: &Path) -> anyhow::Result<()> {
-    let output = Command::new("cg_annotate")
+    check_installed("cg_annotate")?;
+    let status = Command::new("cg_annotate")
         .arg("--show-percs=no")
         .arg(cgout)
         .stderr(Stdio::inherit())
-        .output()
+        .stdout(File::create(path)?)
+        .status()
         .context("failed to run `cg_annotate`")?;
 
-    if !output.status.success() {
-        anyhow::bail!("failed to annotate cachegrind output");
-    }
-
-    fs::write(path, output.stdout).context("failed to write `cg_annotate` output")?;
+    anyhow::ensure!(status.success(), "failed to annotate cachegrind output");
 
     Ok(())
 }

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

@@ -1,7 +1,8 @@
 use std::future::Future;
-use std::process::Command;
+use std::process::{Command, Stdio};
 
 pub mod cachegrind;
+pub mod diff;
 pub mod fs;
 pub mod git;
 pub mod mangling;
@@ -17,5 +18,17 @@ pub fn wait_for_future<F: Future<Output = R>, R>(f: F) -> R {
 
 /// Checks if the given binary can be executed.
 pub fn is_installed(name: &str) -> bool {
-    Command::new(name).output().is_ok()
+    Command::new(name)
+        .stdout(Stdio::null())
+        .stderr(Stdio::null())
+        .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(())
 }