From dd9e0d205fd332d6be104360a737a7f4224f9849 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Fri, 20 Sep 2024 18:13:09 +0100 Subject: [PATCH] bench_pr: track switch to callgrind/callgrind_annotate --- ci-bench-runner/src/job/bench_pr.rs | 99 ++++++++----------- .../sample | 0 ci-bench-runner/src/test/mod.rs | 17 ++-- 3 files changed, 50 insertions(+), 66 deletions(-) rename ci-bench-runner/src/test/data/{cachegrind_outputs => callgrind_outputs}/sample (100%) diff --git a/ci-bench-runner/src/job/bench_pr.rs b/ci-bench-runner/src/job/bench_pr.rs index be31d2a..f42fcc6 100644 --- a/ci-bench-runner/src/job/bench_pr.rs +++ b/ci-bench-runner/src/job/bench_pr.rs @@ -2,10 +2,9 @@ use std::cmp::Ordering; use std::collections::HashMap; use std::fmt::Write; use std::fs; -use std::fs::File; use std::ops::Deref; use std::path::Path; -use std::process::{Command, Stdio}; +use std::process::Command; use anyhow::{anyhow, bail, Context}; use askama::Template; @@ -554,7 +553,7 @@ fn compare_results( }; let cachegrind_diff = if scenario_kind == ScenarioKind::Icount { - Some(cachegrind_diff(job_output_path, scenario)?) + Some(callgrind_diff(job_output_path, scenario)?) } else { None }; @@ -604,68 +603,56 @@ fn split_on_threshold(diffs: Vec) -> (Vec, Vec anyhow::Result { - // The latest version of valgrind has deprecated cg_diff, which has been superseded by - // cg_annotate. Many systems are running older versions, though, so we are sticking with cg_diff - // for the time being. - - let diffs_path = job_output_path.join("diffs"); - fs::create_dir_all(&diffs_path).context("failed to create dir for cg_diff output")?; - - let baseline_cachegrind_file_path = job_output_path - .join("base/results/cachegrind") - .join(scenario); - let candidate_cachegrind_file_path = job_output_path - .join("candidate/results/cachegrind") - .join(scenario); - let diff_file_path = diffs_path.join(scenario); - - // cg_diff generates a diff between two cachegrind output files in a custom format that is not - // user-friendly - let diff_file = File::create(&diff_file_path).context("cannot create temp file for cg_diff")?; - let cg_diff = Command::new("cg_diff") - // remove per-compilation uniqueness in symbols, eg - // _ZN9hashbrown3raw21RawTable$LT$T$C$A$GT$14reserve_rehash17hc60392f3f3eac4b2E.llvm.9716880419886440089 -> - // _ZN9hashbrown3raw21RawTable$LT$T$C$A$GT$14reserve_rehashE - .arg("--mod-funcname=s/17h[0-9a-f]+E\\.llvm\\.\\d+/E/") - // remove the leading path, which is unique for each checkout of the repository (we replace it by `rustls`) - .arg("--mod-filename=s/.+\\/(target\\/release\\/build.+)/rustls\\/\\1/") - .arg(baseline_cachegrind_file_path) - .arg(candidate_cachegrind_file_path) - .stdout(Stdio::from(diff_file)) - .spawn() - .context("cannot spawn cg_diff subprocess")? - .wait() - .context("error waiting for cg_diff to finish")?; - - if !cg_diff.success() { - bail!( - "cg_diff finished with an error (code = {:?})", - cg_diff.code() +pub fn callgrind_diff(job_output_path: &Path, scenario: &str) -> anyhow::Result { + // callgrind_annotate formats the callgrind output file, suitable for comparison with + // callgrind_differ + let callgrind_annotate_base = Command::new("callgrind_annotate") + .arg( + job_output_path + .join("base/results/callgrind") + .join(scenario), ) - } - - // cg_annotate transforms the output of cg_diff into something a user can understand - let cg_annotate = Command::new("cg_annotate") - .arg(diff_file_path) + // do not annotate source, to keep output compact .arg("--auto=no") .output() - .context("error waiting for cg_annotate to finish")?; + .context("error waiting for callgrind_annotate to finish")?; - let stdout = - String::from_utf8(cg_annotate.stdout).context("cg_annotate produced invalid UTF8")?; + let callgrind_annotate_candidate = Command::new("callgrind_annotate") + .arg( + job_output_path + .join("candidate/results/callgrind") + .join(scenario), + ) + // do not annotate source, to keep output compact + .arg("--auto=no") + .output() + .context("error waiting for callgrind_annotate to finish")?; - if !cg_annotate.status.success() { - let stderr = - String::from_utf8(cg_annotate.stderr).context("cg_annotate produced invalid UTF8")?; + if !callgrind_annotate_base.status.success() { + anyhow::bail!( + "callgrind_annotate for base finished with an error (code = {:?})", + callgrind_annotate_base.status.code() + ) + } - bail!( - "cg_annotate finished with an error (code = {:?}). Stdout:\n{stdout}\nStderr:\n{stderr}", - cg_annotate.status.code() + if !callgrind_annotate_candidate.status.success() { + anyhow::bail!( + "callgrind_annotate for candidate finished with an error (code = {:?})", + callgrind_annotate_candidate.status.code() ) } - Ok(stdout) + let string_base = String::from_utf8(callgrind_annotate_base.stdout) + .context("callgrind_annotate produced invalid UTF8")?; + let string_candidate = String::from_utf8(callgrind_annotate_candidate.stdout) + .context("callgrind_annotate produced invalid UTF8")?; + + // TODO: reinstate actual diffing, using `callgrind_differ` crate + Ok(format!( + "Base output:\n{string_base}\n\ + =====\n\n\ + Candidate output:\n{string_candidate}\n" + )) } #[derive(Template)] diff --git a/ci-bench-runner/src/test/data/cachegrind_outputs/sample b/ci-bench-runner/src/test/data/callgrind_outputs/sample similarity index 100% rename from ci-bench-runner/src/test/data/cachegrind_outputs/sample rename to ci-bench-runner/src/test/data/callgrind_outputs/sample diff --git a/ci-bench-runner/src/test/mod.rs b/ci-bench-runner/src/test/mod.rs index 1a0d5aa..81b9250 100644 --- a/ci-bench-runner/src/test/mod.rs +++ b/ci-bench-runner/src/test/mod.rs @@ -79,8 +79,8 @@ mod webhook { } } -mod cachegrind { - pub static SAMPLE_OUTPUT: &str = include_str!("data/cachegrind_outputs/sample"); +mod callgrind { + pub static SAMPLE_OUTPUT: &str = include_str!("data/callgrind_outputs/sample"); } struct MockBenchRunner { @@ -134,14 +134,11 @@ impl BenchRunner for MockBenchRunner { // Fake icounts fs::write(results_dir.join("icounts.csv"), "fake_bench,12345")?; - // Fake cachegrind output - let cachegrind_dir = results_dir.join("cachegrind"); - fs::create_dir(&cachegrind_dir)?; - fs::write( - cachegrind_dir.join("calibration"), - cachegrind::SAMPLE_OUTPUT, - )?; - fs::write(cachegrind_dir.join("fake_bench"), cachegrind::SAMPLE_OUTPUT)?; + // Fake callgrind output + let callgrind_dir = results_dir.join("callgrind"); + fs::create_dir(&callgrind_dir)?; + fs::write(callgrind_dir.join("calibration"), callgrind::SAMPLE_OUTPUT)?; + fs::write(callgrind_dir.join("fake_bench"), callgrind::SAMPLE_OUTPUT)?; // Fake walltimes fs::write(