From a9b0e5607e0847f83ac2b4783a357f77297f48b8 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 13 Jan 2025 14:58:03 +0000 Subject: [PATCH 1/5] Undo some clippy --fix verbosity --- collector/src/bin/collector.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index 2f5e7fa9e..bfeb5ec98 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -1158,18 +1158,15 @@ fn main_result() -> anyhow::Result { 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()); - } + if diffs.len() == 1 { + 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()); + } else { + eprintln!("Diffs:"); + for diff in diffs { + eprintln!("{}", diff.to_string_lossy()); } } } From 2d993543f3c1f2cd9d905e6408d79c7c0309515b Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 13 Jan 2025 14:58:56 +0000 Subject: [PATCH 2/5] Prefer slice patterns over length check and indexing --- collector/src/bin/collector.rs | 6 +++--- collector/src/compare.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index bfeb5ec98..7227ec7d4 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -1158,10 +1158,10 @@ fn main_result() -> anyhow::Result { scenarios, &mut errors, ); - if diffs.len() == 1 { + if let [diff] = &diffs[..] { 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()); + 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:"); diff --git a/collector/src/compare.rs b/collector/src/compare.rs index b7b21305b..5327860b4 100644 --- a/collector/src/compare.rs +++ b/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 From 95c68deafdd4865b16135baf093be7ad03a5bdf7 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 13 Jan 2025 15:27:02 +0000 Subject: [PATCH 3/5] Fix crate version numbers causing everything from them on to be replaced with the extension --- collector/src/compile/execute/profiler.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/collector/src/compile/execute/profiler.rs b/collector/src/compile/execute/profiler.rs index 8054eedf6..8398c367d 100644 --- a/collector/src/compile/execute/profiler.rs +++ b/collector/src/compile/execute/profiler.rs @@ -371,13 +371,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)?; From a9cc22f1e429ffea35978a67892537f937d60009 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 13 Jan 2025 15:01:16 +0000 Subject: [PATCH 4/5] Avoid `Command::output` if it's just going to be written to a file anyway --- collector/src/compile/execute/profiler.rs | 22 ++++++++++-------- collector/src/toolchain.rs | 7 +++--- collector/src/utils/cachegrind.rs | 28 ++++++++++------------- collector/src/utils/mod.rs | 8 +++++-- 4 files changed, 34 insertions(+), 31 deletions(-) diff --git a/collector/src/compile/execute/profiler.rs b/collector/src/compile/execute/profiler.rs index 8398c367d..1c27617b4 100644 --- a/collector/src/compile/execute/profiler.rs +++ b/collector/src/compile/execute/profiler.rs @@ -3,6 +3,7 @@ use crate::utils; use crate::utils::cachegrind::cachegrind_annotate; use anyhow::Context; use std::collections::HashMap; +use std::fs::File; use std::future::Future; use std::io::BufRead; use std::io::Write; @@ -143,10 +144,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,8 +197,9 @@ 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 @@ -207,8 +207,9 @@ impl<'a> Processor for ProfileProcessor<'a> { .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 +249,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 diff --git a/collector/src/toolchain.rs b/collector/src/toolchain.rs index 503fa2d5d..42ba7ff0f 100644 --- a/collector/src/toolchain.rs +++ b/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")?; diff --git a/collector/src/utils/cachegrind.rs b/collector/src/utils/cachegrind.rs index d4e9966b1..fe7584758 100644 --- a/collector/src/utils/cachegrind.rs +++ b/collector/src/utils/cachegrind.rs @@ -1,6 +1,7 @@ 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}; @@ -61,8 +62,9 @@ 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(()) } @@ -81,38 +83,32 @@ 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") + 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") + 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(()) } diff --git a/collector/src/utils/mod.rs b/collector/src/utils/mod.rs index ff728156f..bf98cee07 100644 --- a/collector/src/utils/mod.rs +++ b/collector/src/utils/mod.rs @@ -1,5 +1,5 @@ use std::future::Future; -use std::process::Command; +use std::process::{Command, Stdio}; pub mod cachegrind; pub mod fs; @@ -17,5 +17,9 @@ pub fn wait_for_future, 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() } From 0892746aa9475a667c95a8b12d977dd2bb145143 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 13 Jan 2025 15:01:16 +0000 Subject: [PATCH 5/5] Make it easy to add diffing of output of arbitrary profilers (demonstrate for dep-graph) --- collector/src/bin/collector.rs | 73 +++++++++++------------ collector/src/compile/execute/profiler.rs | 41 ++++++++++++- collector/src/utils/cachegrind.rs | 9 +-- collector/src/utils/diff.rs | 23 +++++++ collector/src/utils/mod.rs | 9 +++ 5 files changed, 111 insertions(+), 44 deletions(-) create mode 100644 collector/src/utils/diff.rs diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index 7227ec7d4..20e412987 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -132,14 +132,8 @@ 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, @@ -147,6 +141,7 @@ fn generate_cachegrind_diffs( profiles: &[Profile], scenarios: &[Scenario], errors: &mut BenchmarkErrors, + profiler: &Profiler, ) -> Vec { 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 { 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 { diff --git a/collector/src/compile/execute/profiler.rs b/collector/src/compile/execute/profiler.rs index 1c27617b4..ce6a4562f 100644 --- a/collector/src/compile/execute/profiler.rs +++ b/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> { diff --git a/collector/src/utils/cachegrind.rs b/collector/src/utils/cachegrind.rs index fe7584758..682740af8 100644 --- a/collector/src/utils/cachegrind.rs +++ b/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) diff --git a/collector/src/utils/diff.rs b/collector/src/utils/diff.rs new file mode 100644 index 000000000..7a5b70c11 --- /dev/null +++ b/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(()) +} diff --git a/collector/src/utils/mod.rs b/collector/src/utils/mod.rs index bf98cee07..02684e305 100644 --- a/collector/src/utils/mod.rs +++ b/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(()) +}