Repository: rustls/rustls (language: rust)

Issue:
CI benchmarks: use cachegrind "client requests" for more precise instruction counts
From [this article](https://kobzol.github.io/rust/rustc/2023/09/23/rustc-runtime-benchmarks.html):

> Valgrind has support for [client requests](https://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq), which allow the profiled program (amongst other things) to selectively enable and disable instrumentation for parts of the program.

They are not yet available for cachegrind, but it looks like support is coming in the next release. Using client requests would greatly improve our benchmarking harness, making it possible to ignore setup code when counting instructions. The functionality should be accessible through the [crabgrind](https://github.com/2dav/crabgrind) crate.

The current workaround to discard the instruction counts originating from setup code is to run the setup code in isolation and subtract the measured icount from the full benchmark's icount. This works well enough in practice, but gets confusing when you are investigating a regression, because the cachegrind diff does include the benchmark's setup code.


Discussion / hints:
Update: valgrind 3.22 (31 October 2023) ships with client request support in cachegrind

Fix patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index a14209f6107..127991c36c9 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -439,6 +439,11 @@ jobs:
         uses: actions/checkout@v4
         with:
           persist-credentials: false
+
+      - name: Install valgrind
+        if: runner.os == 'Linux'
+        run: sudo apt-get update && sudo apt-get install -y valgrind
+
       - name: Install rust toolchain
         uses: dtolnay/rust-toolchain@stable
         with:
@@ -458,6 +463,11 @@ jobs:
         uses: actions/checkout@v4
         with:
           persist-credentials: false
+
+      - name: Install valgrind
+        if: runner.os == 'Linux'
+        run: sudo apt-get update && sudo apt-get install -y valgrind
+
       - name: Install rust toolchain
         uses: dtolnay/rust-toolchain@nightly
         with:
diff --git a/Cargo.lock b/Cargo.lock
index b321c1f7e1f..c8b83e6b123 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -656,6 +656,15 @@ dependencies = [
  "libc",
 ]
 
+[[package]]
+name = "crabgrind"
+version = "0.1.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bdbd43e4f32a9681a504577db2d4ea7d3f7b1bf2e97955561af98501ab600508"
+dependencies = [
+ "cc",
+]
+
 [[package]]
 name = "crossbeam-deque"
 version = "0.8.5"
@@ -2144,6 +2153,7 @@ dependencies = [
  "async-trait",
  "byteorder",
  "clap",
+ "crabgrind",
  "fxhash",
  "itertools 0.13.0",
  "rayon",
diff --git a/ci-bench/Cargo.toml b/ci-bench/Cargo.toml
index c1322f132f8..51b70528f52 100644
--- a/ci-bench/Cargo.toml
+++ b/ci-bench/Cargo.toml
@@ -20,3 +20,6 @@ rustls-pemfile = "2"
 
 [target.'cfg(not(target_env = "msvc"))'.dependencies]
 tikv-jemallocator = "0.6"
+
+[target.'cfg(target_os = "linux")'.dependencies]
+crabgrind = "=0.1.9" # compatible with valgrind package on GHA ubuntu-latest
diff --git a/ci-bench/README.md b/ci-bench/README.md
index 15117186665..8cab18e0231 100644
--- a/ci-bench/README.md
+++ b/ci-bench/README.md
@@ -2,7 +2,7 @@
 
 This crate is meant for CI benchmarking. It has two modes of operation:
 
-1. Measure CPU instructions using `cachegrind`.
+1. Measure CPU instructions using `callgrind`.
 2. Measure wall-time (runs each benchmark multiple times, leaving it to the caller to do statistical
    analysis).
 
@@ -35,10 +35,10 @@ handshake_no_resume_1.3_rsa_aes_client,4212770
 ...
 ```
 
-In the `cachegrind` subdirectory you will find output files emitted by the `cachegrind` tool, which
+In the `callgrind` subdirectory you will find output files emitted by the `callgrind` tool, which
 are useful to report detailed instruction count differences when comparing two benchmark runs. This
-subdirectory also contains log information from cachegrind itself (in `.log` files), which can be
-used to diagnose unexpected cachegrind crashes.
+subdirectory also contains log information from callgrind itself (in `.log` files), which can be
+used to diagnose unexpected callgrind crashes.
 
 ### Running all benchmarks in wall-time mode
 
@@ -86,27 +86,23 @@ here are some high-level considerations that can help you hack on the crate.
 ### Environment configuration
 
 An important goal of this benchmarking setup is that it should run with minimal noise. Measuring CPU
-instructions using `cachegrind` yields excellent results, regardless of your environment. The
+instructions using `callgrind` yields excellent results, regardless of your environment. The
 wall-time benchmarks, however, require a more elaborate benchmarking environment: running them on a
 laptop is too noisy, but running them on a carefully configured bare-metal server yields accurate
 measurements (up to 1% resolution, according to our tests).
 
 ### Instruction count mode
 
-Using `cachegrind` has some architectural consequences because it operates at the process level
-(i.e. it can count CPU instructions for a whole process, but not for a single function). The most
-important consequences when running in instruction count mode are:
-
-- Since we want to measure server and client instruction counts separately, the benchmark runner
-  spawns two child processes for each benchmark (one for the client, one for the server) and pipes
-  their stdio to each other for communication (i.e. stdio acts as the transport layer).
-- There is a no-op "benchmark" that measures the overhead of starting up the child process, so
-  we can subtract it from the instruction count of the real benchmarks and reduce noise.
-- Since we want to measure individual portions of code (e.g. data transfer after the handshake),
-  there is a mechanism to subtract the instructions that are part of a benchmark's setup.
-  Specifically, a benchmark can be configured to have another benchmark's instruction count
-  subtracted from it. We are currently using this to subtract the handshake instructions from the
-  data transfer benchmark.
+Instruction counting is done with `callgrind`, and is precise in the sense that only operations
+we want to measure are included.  We tell `callgrind` to start with collection disabled,
+with `--collect-atstart=no`.  Then we use
+[client requests](https://valgrind.org/docs/manual/cl-manual.html#cl-manual.clientrequests) via
+the [crabgrind crate](https://docs.rs/crabgrind/latest/crabgrind/) to enable and disable collection
+(see `callgrind::CountInstructions`).
+
+Since we want to measure server and client instruction counts separately, the benchmark runner
+spawns two child processes for each benchmark (one for the client, one for the server) and pipes
+their stdio to each other for communication (i.e. stdio acts as the transport layer).
 
 If you need to debug benchmarks in instruction count mode, here are a few tricks that might help:
 
diff --git a/ci-bench/src/benchmark.rs b/ci-bench/src/benchmark.rs
index b7c25bbc997..6434df68a54 100644
--- a/ci-bench/src/benchmark.rs
+++ b/ci-bench/src/benchmark.rs
@@ -1,9 +1,9 @@
 use std::sync::Arc;
 
-use fxhash::{FxHashMap, FxHashSet};
+use fxhash::FxHashMap;
 use itertools::Itertools;
 
-use crate::cachegrind::InstructionCounts;
+use crate::callgrind::InstructionCounts;
 use crate::util::KeyType;
 use crate::Side;
 
@@ -12,7 +12,6 @@ use crate::Side;
 /// Benchmarks can be invalid because of the following reasons:
 ///
 /// - Re-using an already defined benchmark name.
-/// - Referencing a non-existing benchmark in [`ReportingMode::AllInstructionsExceptSetup`].
 pub fn validate_benchmarks(benchmarks: &[Benchmark]) -> anyhow::Result<()> {
     // Detect duplicate definitions
     let duplicate_names: Vec<_> = benchmarks
@@ -27,55 +26,15 @@ pub fn validate_benchmarks(benchmarks: &[Benchmark]) -> anyhow::Result<()> {
         );
     }
 
-    // Detect dangling benchmark references
-    let all_names: FxHashSet<_> = benchmarks
-        .iter()
-        .map(|b| b.name.as_str())
-        .collect();
-    let referenced_names: FxHashSet<_> = benchmarks
-        .iter()
-        .flat_map(|b| match &b.reporting_mode {
-            ReportingMode::AllInstructions => None,
-            ReportingMode::AllInstructionsExceptSetup(name) => Some(name.as_str()),
-        })
-        .collect();
-
-    let undefined_names: Vec<_> = referenced_names
-        .difference(&all_names)
-        .cloned()
-        .collect();
-    if !undefined_names.is_empty() {
-        anyhow::bail!("The following benchmark names are referenced, but have no corresponding benchmarks: {}",
-            undefined_names.join(", "));
-    }
-
     Ok(())
 }
 
-/// Specifies how the results of a particular benchmark should be reported
-pub enum ReportingMode {
-    /// All instructions are reported
-    AllInstructions,
-    /// All instructions are reported, after subtracting the instructions of the setup code
-    ///
-    /// The instruction count of the setup code is obtained by running a benchmark containing only
-    /// that code. The string parameter corresponds to the name of that benchmark.
-    AllInstructionsExceptSetup(String),
-}
-
 /// Get the reported instruction counts for the provided benchmark
 pub fn get_reported_instr_count(
     bench: &Benchmark,
     results: &FxHashMap<&str, InstructionCounts>,
 ) -> InstructionCounts {
-    match bench.reporting_mode() {
-        ReportingMode::AllInstructions => results[&bench.name()],
-        ReportingMode::AllInstructionsExceptSetup(setup_name) => {
-            let bench_results = results[&bench.name()];
-            let setup_results = results[setup_name.as_str()];
-            bench_results - setup_results
-        }
-    }
+    results[&bench.name()]
 }
 
 /// Specifies which functionality is being benchmarked
@@ -167,26 +126,12 @@ pub struct Benchmark {
     pub kind: BenchmarkKind,
     /// The benchmark's parameters
     pub params: BenchmarkParams,
-    /// The way instruction counts should be reported for this benchmark
-    pub reporting_mode: ReportingMode,
 }
 
 impl Benchmark {
     /// Create a new benchmark
     pub fn new(name: String, kind: BenchmarkKind, params: BenchmarkParams) -> Self {
-        Self {
-            name,
-            kind,
-            params,
-            reporting_mode: ReportingMode::AllInstructions,
-        }
-    }
-
-    /// Configure this benchmark to subtract the instruction count of the referenced benchmark when
-    /// reporting results
-    pub fn exclude_setup_instructions(mut self, name: String) -> Self {
-        self.reporting_mode = ReportingMode::AllInstructionsExceptSetup(name);
-        self
+        Self { name, kind, params }
     }
 
     /// Returns the benchmark's unique name
@@ -198,9 +143,4 @@ impl Benchmark {
     pub fn name_with_side(&self, side: Side) -> String {
         format!("{}_{}", self.name, side.as_str())
     }
-
-    /// Returns the benchmark's reporting mode
-    pub fn reporting_mode(&self) -> &ReportingMode {
-        &self.reporting_mode
-    }
 }
diff --git a/ci-bench/src/cachegrind.rs b/ci-bench/src/cachegrind.rs
deleted file mode 100644
index 6f945bbad01..00000000000
--- a/ci-bench/src/cachegrind.rs
+++ /dev/null
@@ -1,293 +0,0 @@
-use std::fs::{self, File};
-use std::io::{BufRead, BufReader};
-use std::ops::Sub;
-use std::path::{Path, PathBuf};
-use std::process::{Child, Command, Stdio};
-
-use anyhow::Context;
-
-use crate::benchmark::Benchmark;
-use crate::Side;
-
-/// The subdirectory in which the cachegrind output should be stored
-const CACHEGRIND_OUTPUT_SUBDIR: &str = "cachegrind";
-
-/// A cachegrind-based benchmark runner
-pub struct CachegrindRunner {
-    /// The path to the ci-bench executable
-    ///
-    /// This is necessary because the cachegrind runner works by spawning child processes
-    executable: String,
-    /// The directory where the cachegrind output will be stored
-    output_dir: PathBuf,
-    /// The amount of instructions that are executed upon startup of the child process, before
-    /// actually running one of the benchmarks
-    ///
-    /// This count is subtracted from benchmark results, to reduce noise
-    overhead_instructions: u64,
-}
-
-impl CachegrindRunner {
-    /// Returns a new cachegrind-based benchmark runner
-    pub fn new(executable: String, output_dir: PathBuf) -> anyhow::Result<Self> {
-        Self::ensure_cachegrind_available()?;
-
-        let cachegrind_output_dir = output_dir.join(CACHEGRIND_OUTPUT_SUBDIR);
-        std::fs::create_dir_all(&cachegrind_output_dir)
-            .context("Failed to create cachegrind output directory")?;
-
-        // We don't care about the side here, so let's use `Server` just to choose something
-        let overhead_instructions = Self::run_bench_side(
-            &executable,
-            u32::MAX,
-            Side::Server,
-            "calibration",
-            Stdio::piped(),
-            Stdio::piped(),
-            &cachegrind_output_dir,
-        )?
-        .wait_and_get_instr_count()
-        .context("Unable to count overhead instructions")?;
-
-        Ok(CachegrindRunner {
-            executable,
-            output_dir: cachegrind_output_dir,
-            overhead_instructions,
-        })
-    }
-
-    /// Runs the benchmark at the specified index and returns the instruction counts for each side
-    pub fn run_bench(
-        &self,
-        benchmark_index: u32,
-        bench: &Benchmark,
-    ) -> anyhow::Result<InstructionCounts> {
-        // The server and client are started as child processes, and communicate with each other
-        // through stdio.
-
-        let mut server = Self::run_bench_side(
-            &self.executable,
-            benchmark_index,
-            Side::Server,
-            &bench.name_with_side(Side::Server),
-            Stdio::piped(),
-            Stdio::piped(),
-            &self.output_dir,
-        )
-        .context("server side bench crashed")?;
-
-        let client = Self::run_bench_side(
-            &self.executable,
-            benchmark_index,
-            Side::Client,
-            &bench.name_with_side(Side::Client),
-            Stdio::from(server.process.stdout.take().unwrap()),
-            Stdio::from(server.process.stdin.take().unwrap()),
-            &self.output_dir,
-        )
-        .context("client side bench crashed")?;
-
-        let counts = InstructionCounts {
-            server: server.wait_and_get_instr_count()?,
-            client: client.wait_and_get_instr_count()?,
-        };
-
-        let overhead_counts = InstructionCounts {
-            server: self.overhead_instructions,
-            client: self.overhead_instructions,
-        };
-
-        Ok(counts - overhead_counts)
-    }
-
-    /// Returns an error if cachegrind is not available
-    fn ensure_cachegrind_available() -> anyhow::Result<()> {
-        let result = Command::new("valgrind")
-            .arg("--tool=cachegrind")
-            .arg("--version")
-            .stdout(Stdio::null())
-            .stderr(Stdio::null())
-            .status();
-
-        match result {
-            Err(e) => anyhow::bail!("Unexpected error while launching cachegrind. Error: {}", e),
-            Ok(status) => {
-                if status.success() {
-                    Ok(())
-                } else {
-                    anyhow::bail!("Failed to launch cachegrind. Error: {}. Please ensure that valgrind is installed and on the $PATH.", status)
-                }
-            }
-        }
-    }
-
-    /// See docs for [`Self::run_bench`]
-    fn run_bench_side(
-        executable: &str,
-        benchmark_index: u32,
-        side: Side,
-        name: &str,
-        stdin: Stdio,
-        stdout: Stdio,
-        output_dir: &Path,
-    ) -> anyhow::Result<BenchSubprocess> {
-        let cachegrind_output_file = output_dir.join(name);
-        let cachegrind_log_file = output_dir.join(format!("{name}.log"));
-
-        // Run under setarch to disable ASLR, to reduce noise
-        let mut cmd = Command::new("setarch");
-        let child = cmd
-            .arg("-R")
-            .arg("valgrind")
-            .arg("--tool=cachegrind")
-            // Disable the cache simulation, since we are only interested in instruction counts
-            .arg("--cache-sim=no")
-            // Save cachegrind's logs, which would otherwise be printed to stderr (we want to
-            // keep stderr free of noise, to see any errors from the child process)
-            .arg(format!("--log-file={}", cachegrind_log_file.display()))
-            // The file where the instruction counts will be stored
-            .arg(format!(
-                "--cachegrind-out-file={}",
-                cachegrind_output_file.display()
-            ))
-            .arg(executable)
-            .arg("run-single")
-            .arg(benchmark_index.to_string())
-            .arg(side.as_str())
-            .stdin(stdin)
-            .stdout(stdout)
-            .stderr(Stdio::inherit())
-            .spawn()
-            .context("Failed to run benchmark in cachegrind")?;
-
-        Ok(BenchSubprocess {
-            process: child,
-            cachegrind_output_file,
-        })
-    }
-}
-
-/// A running subprocess for one of the sides of the benchmark (client or server)
-struct BenchSubprocess {
-    /// The benchmark's child process, running under cachegrind
-    process: Child,
-    /// Cachegrind's output file for this benchmark
-    cachegrind_output_file: PathBuf,
-}
-
-impl BenchSubprocess {
-    /// Waits for the process to finish and returns the measured instruction count
-    fn wait_and_get_instr_count(mut self) -> anyhow::Result<u64> {
-        let status = self
-            .process
-            .wait()
-            .context("Failed to run benchmark in cachegrind")?;
-        if !status.success() {
-            anyhow::bail!(
-                "Failed to run benchmark in cachegrind. Exit code: {:?}",
-                status.code()
-            );
-        }
-
-        let instruction_count = parse_cachegrind_output(&self.cachegrind_output_file)?;
-        Ok(instruction_count)
-    }
-}
-
-/// Returns the instruction count, extracted from the cachegrind output file at the provided path
-fn parse_cachegrind_output(file: &Path) -> anyhow::Result<u64> {
-    let file_in = File::open(file).context("Unable to open cachegrind output file")?;
-
-    for line in BufReader::new(file_in).lines() {
-        let line = line.context("Error reading cachegrind output file")?;
-        if let Some(line) = line.strip_prefix("summary: ") {
-            let instr_count = line
-                .trim()
-                .parse()
-                .context("Unable to parse instruction counts from cachegrind output file")?;
-
-            return Ok(instr_count);
-        }
-    }
-
-    anyhow::bail!("`summary` section not found in cachegrind output file")
-}
-
-/// The instruction counts, for each side, after running a benchmark
-#[derive(Copy, Clone)]
-pub struct InstructionCounts {
-    pub client: u64,
-    pub server: u64,
-}
-
-impl Sub for InstructionCounts {
-    type Output = InstructionCounts;
-
-    fn sub(self, rhs: Self) -> Self::Output {
-        InstructionCounts {
-            client: self.client - rhs.client,
-            server: self.server - rhs.server,
-        }
-    }
-}
-
-/// Returns the detailed instruction diff between the baseline and the candidate
-pub fn diff(baseline: &Path, candidate: &Path, scenario: &str) -> anyhow::Result<String> {
-    // 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 tmp_path = Path::new("ci-bench-tmp");
-    let tmp = File::create(tmp_path).context("cannot create temp file for cg_diff")?;
-
-    // cg_diff generates a diff between two cachegrind output files in a custom format that is not
-    // user-friendly
-    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/")
-        .arg(
-            baseline
-                .join(CACHEGRIND_OUTPUT_SUBDIR)
-                .join(scenario),
-        )
-        .arg(
-            candidate
-                .join(CACHEGRIND_OUTPUT_SUBDIR)
-                .join(scenario),
-        )
-        .stdout(Stdio::from(tmp))
-        .spawn()
-        .context("cannot spawn cg_diff subprocess")?
-        .wait()
-        .context("error waiting for cg_diff to finish")?;
-
-    if !cg_diff.success() {
-        anyhow::bail!(
-            "cg_diff finished with an error (code = {:?})",
-            cg_diff.code()
-        )
-    }
-
-    // cg_annotate transforms the output of cg_diff into something a user can understand
-    let cg_annotate = Command::new("cg_annotate")
-        .arg(tmp_path)
-        .arg("--auto=no")
-        .output()
-        .context("error waiting for cg_annotate to finish")?;
-
-    if !cg_annotate.status.success() {
-        anyhow::bail!(
-            "cg_annotate finished with an error (code = {:?})",
-            cg_annotate.status.code()
-        )
-    }
-
-    let diff =
-        String::from_utf8(cg_annotate.stdout).context("cg_annotate produced invalid UTF8")?;
-
-    fs::remove_file(tmp_path).ok();
-
-    Ok(diff)
-}
diff --git a/ci-bench/src/callgrind.rs b/ci-bench/src/callgrind.rs
new file mode 100644
index 00000000000..2dba776762d
--- /dev/null
+++ b/ci-bench/src/callgrind.rs
@@ -0,0 +1,288 @@
+use std::fs::File;
+use std::io::{BufRead, BufReader};
+use std::ops::Sub;
+use std::path::{Path, PathBuf};
+use std::process::{Child, Command, Stdio};
+
+use anyhow::Context;
+
+use crate::benchmark::Benchmark;
+use crate::Side;
+
+/// The subdirectory in which the callgrind output should be stored
+const CALLGRIND_OUTPUT_SUBDIR: &str = "callgrind";
+
+/// A callgrind-based benchmark runner
+pub struct CallgrindRunner {
+    /// The path to the ci-bench executable
+    ///
+    /// This is necessary because the callgrind runner works by spawning child processes
+    executable: String,
+    /// The directory where the callgrind output will be stored
+    output_dir: PathBuf,
+}
+
+impl CallgrindRunner {
+    /// Returns a new callgrind-based benchmark runner
+    pub fn new(executable: String, output_dir: PathBuf) -> anyhow::Result<Self> {
+        Self::ensure_callgrind_available()?;
+
+        let callgrind_output_dir = output_dir.join(CALLGRIND_OUTPUT_SUBDIR);
+        std::fs::create_dir_all(&callgrind_output_dir)
+            .context("Failed to create callgrind output directory")?;
+
+        Ok(CallgrindRunner {
+            executable,
+            output_dir: callgrind_output_dir,
+        })
+    }
+
+    /// Runs the benchmark at the specified index and returns the instruction counts for each side
+    pub fn run_bench(
+        &self,
+        benchmark_index: u32,
+        bench: &Benchmark,
+    ) -> anyhow::Result<InstructionCounts> {
+        // The server and client are started as child processes, and communicate with each other
+        // through stdio.
+
+        let mut server = Self::run_bench_side(
+            &self.executable,
+            benchmark_index,
+            Side::Server,
+            &bench.name_with_side(Side::Server),
+            Stdio::piped(),
+            Stdio::piped(),
+            &self.output_dir,
+        )
+        .context("server side bench crashed")?;
+
+        let client = Self::run_bench_side(
+            &self.executable,
+            benchmark_index,
+            Side::Client,
+            &bench.name_with_side(Side::Client),
+            Stdio::from(server.process.stdout.take().unwrap()),
+            Stdio::from(server.process.stdin.take().unwrap()),
+            &self.output_dir,
+        )
+        .context("client side bench crashed")?;
+
+        Ok(InstructionCounts {
+            server: server.wait_and_get_instr_count()?,
+            client: client.wait_and_get_instr_count()?,
+        })
+    }
+
+    /// Returns an error if callgrind is not available
+    fn ensure_callgrind_available() -> anyhow::Result<()> {
+        let result = Command::new("valgrind")
+            .arg("--tool=callgrind")
+            .arg("--version")
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .status();
+
+        match result {
+            Err(e) => anyhow::bail!("Unexpected error while launching callgrind. Error: {}", e),
+            Ok(status) => {
+                if status.success() {
+                    Ok(())
+                } else {
+                    anyhow::bail!("Failed to launch callgrind. Error: {}. Please ensure that valgrind is installed and on the $PATH.", status)
+                }
+            }
+        }
+    }
+
+    /// See docs for [`Self::run_bench`]
+    fn run_bench_side(
+        executable: &str,
+        benchmark_index: u32,
+        side: Side,
+        name: &str,
+        stdin: Stdio,
+        stdout: Stdio,
+        output_dir: &Path,
+    ) -> anyhow::Result<BenchSubprocess> {
+        let callgrind_output_file = output_dir.join(name);
+        let callgrind_log_file = output_dir.join(format!("{name}.log"));
+
+        // Run under setarch to disable ASLR, to reduce noise
+        let mut cmd = Command::new("setarch");
+        let child = cmd
+            .arg("-R")
+            .arg("valgrind")
+            .arg("--tool=callgrind")
+            // Do not count instructions from the start, instead this is controlled by `CountInstructions`
+            .arg("--collect-atstart=no")
+            // Disable the cache simulation, since we are only interested in instruction counts
+            .arg("--cache-sim=no")
+            // Save callgrind's logs, which would otherwise be printed to stderr (we want to
+            // keep stderr free of noise, to see any errors from the child process)
+            .arg(format!("--log-file={}", callgrind_log_file.display()))
+            // The file where the instruction counts will be stored
+            .arg(format!(
+                "--callgrind-out-file={}",
+                callgrind_output_file.display()
+            ))
+            .arg(executable)
+            .arg("run-single")
+            .arg(benchmark_index.to_string())
+            .arg(side.as_str())
+            .stdin(stdin)
+            .stdout(stdout)
+            .stderr(Stdio::inherit())
+            .spawn()
+            .context("Failed to run benchmark in callgrind")?;
+
+        Ok(BenchSubprocess {
+            process: child,
+            callgrind_output_file,
+        })
+    }
+}
+
+/// A running subprocess for one of the sides of the benchmark (client or server)
+struct BenchSubprocess {
+    /// The benchmark's child process, running under callgrind
+    process: Child,
+    /// Callgrind's output file for this benchmark
+    callgrind_output_file: PathBuf,
+}
+
+impl BenchSubprocess {
+    /// Waits for the process to finish and returns the measured instruction count
+    fn wait_and_get_instr_count(mut self) -> anyhow::Result<u64> {
+        let status = self
+            .process
+            .wait()
+            .context("Failed to run benchmark in callgrind")?;
+        if !status.success() {
+            anyhow::bail!(
+                "Failed to run benchmark in callgrind. Exit code: {:?}",
+                status.code()
+            );
+        }
+
+        let instruction_count = parse_callgrind_output(&self.callgrind_output_file)?;
+        Ok(instruction_count)
+    }
+}
+
+/// Returns the instruction count, extracted from the callgrind output file at the provided path
+fn parse_callgrind_output(file: &Path) -> anyhow::Result<u64> {
+    let file_in = File::open(file).context("Unable to open callgrind output file")?;
+
+    for line in BufReader::new(file_in).lines() {
+        let line = line.context("Error reading callgrind output file")?;
+        if let Some(line) = line.strip_prefix("summary: ") {
+            let instr_count = line
+                .trim()
+                .parse()
+                .context("Unable to parse instruction counts from callgrind output file")?;
+
+            return Ok(instr_count);
+        }
+    }
+
+    anyhow::bail!("`summary` section not found in callgrind output file")
+}
+
+/// The instruction counts, for each side, after running a benchmark
+#[derive(Copy, Clone)]
+pub struct InstructionCounts {
+    pub client: u64,
+    pub server: u64,
+}
+
+impl Sub for InstructionCounts {
+    type Output = InstructionCounts;
+
+    fn sub(self, rhs: Self) -> Self::Output {
+        InstructionCounts {
+            client: self.client - rhs.client,
+            server: self.server - rhs.server,
+        }
+    }
+}
+
+/// Returns the detailed instruction diff between the baseline and the candidate
+pub fn diff(baseline: &Path, candidate: &Path, scenario: &str) -> anyhow::Result<String> {
+    // callgrind_annotate formats the callgrind output file, suitable for comparison with
+    // callgrind_differ
+    let callgrind_annotate_base = Command::new("callgrind_annotate")
+        .arg(
+            baseline
+                .join(CALLGRIND_OUTPUT_SUBDIR)
+                .join(scenario),
+        )
+        // do not annotate source, to keep output compact
+        .arg("--auto=no")
+        .output()
+        .context("error waiting for callgrind_annotate to finish")?;
+
+    let callgrind_annotate_candidate = Command::new("callgrind_annotate")
+        .arg(
+            candidate
+                .join(CALLGRIND_OUTPUT_SUBDIR)
+                .join(scenario),
+        )
+        // do not annotate source, to keep output compact
+        .arg("--auto=no")
+        .output()
+        .context("error waiting for callgrind_annotate to finish")?;
+
+    if !callgrind_annotate_base.status.success() {
+        anyhow::bail!(
+            "callgrind_annotate for base finished with an error (code = {:?})",
+            callgrind_annotate_base.status.code()
+        )
+    }
+
+    if !callgrind_annotate_candidate
+        .status
+        .success()
+    {
+        anyhow::bail!(
+            "callgrind_annotate for candidate finished with an error (code = {:?})",
+            callgrind_annotate_candidate
+                .status
+                .code()
+        )
+    }
+
+    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"
+    ))
+}
+
+/// A RAII-like object for enabling callgrind instruction counting.
+///
+/// Warning: must not be nested.
+///
+/// Instructions outside the scope of these objects are not counted.
+pub(crate) struct CountInstructions;
+
+impl CountInstructions {
+    pub(crate) fn start() -> Self {
+        #[cfg(target_os = "linux")]
+        crabgrind::callgrind::toggle_collect();
+        CountInstructions
+    }
+}
+
+impl Drop for CountInstructions {
+    fn drop(&mut self) {
+        #[cfg(target_os = "linux")]
+        crabgrind::callgrind::toggle_collect();
+    }
+}
diff --git a/ci-bench/src/main.rs b/ci-bench/src/main.rs
index b045e05b4ce..dcd879f6b5d 100644
--- a/ci-bench/src/main.rs
+++ b/ci-bench/src/main.rs
@@ -16,18 +16,18 @@ use itertools::Itertools;
 use rayon::iter::Either;
 use rayon::prelude::*;
 use rustls::client::Resumption;
-use rustls::crypto::{aws_lc_rs, ring};
+use rustls::crypto::{aws_lc_rs, ring, CryptoProvider, GetRandomFailed, SecureRandom};
 use rustls::server::{NoServerSessionStorage, ServerSessionMemoryCache, WebPkiClientVerifier};
 use rustls::{
-    CipherSuite, ClientConfig, ClientConnection, ProtocolVersion, RootCertStore, ServerConfig,
-    ServerConnection,
+    CipherSuite, ClientConfig, ClientConnection, HandshakeKind, ProtocolVersion, RootCertStore,
+    ServerConfig, ServerConnection,
 };
 
 use crate::benchmark::{
     get_reported_instr_count, validate_benchmarks, Benchmark, BenchmarkKind, BenchmarkParams,
     ResumptionKind,
 };
-use crate::cachegrind::CachegrindRunner;
+use crate::callgrind::{CallgrindRunner, CountInstructions};
 use crate::util::async_io::{self, AsyncRead, AsyncWrite};
 use crate::util::transport::{
     read_handshake_message, read_plaintext_to_end_bounded, send_handshake_message,
@@ -36,7 +36,7 @@ use crate::util::transport::{
 use crate::util::KeyType;
 
 mod benchmark;
-mod cachegrind;
+mod callgrind;
 mod util;
 
 /// The size in bytes of the plaintext sent in the transfer benchmark
@@ -297,13 +297,13 @@ fn all_benchmarks_params() -> Vec<BenchmarkParams> {
 
     for (provider, suites, ticketer, provider_name) in [
         (
-            ring::default_provider(),
+            derandomize(ring::default_provider()),
             ring::ALL_CIPHER_SUITES,
             &(ring_ticketer as fn() -> Arc<dyn rustls::server::ProducesTickets>),
             "ring",
         ),
         (
-            aws_lc_rs::default_provider(),
+            derandomize(aws_lc_rs::default_provider()),
             aws_lc_rs::ALL_CIPHER_SUITES,
             &(aws_lc_rs_ticketer as fn() -> Arc<dyn rustls::server::ProducesTickets>),
             "aws_lc_rs",
@@ -384,6 +384,23 @@ fn aws_lc_rs_ticketer() -> Arc<dyn rustls::server::ProducesTickets> {
     aws_lc_rs::Ticketer::new().unwrap()
 }
 
+fn derandomize(base: CryptoProvider) -> CryptoProvider {
+    CryptoProvider {
+        secure_random: &NotRandom,
+        ..base
+    }
+}
+
+#[derive(Debug)]
+struct NotRandom;
+
+impl SecureRandom for NotRandom {
+    fn fill(&self, buf: &mut [u8]) -> Result<(), GetRandomFailed> {
+        buf.fill(0x5a);
+        Ok(())
+    }
+}
+
 /// Adds a group of benchmarks for the specified parameters
 ///
 /// The benchmarks in the group are:
@@ -403,41 +420,29 @@ fn add_benchmark_group(benchmarks: &mut Vec<Benchmark>, params: BenchmarkParams)
             params.clone(),
         );
 
-        let handshake_bench = if resumption_param != ResumptionKind::No {
-            // Since resumed handshakes include a first non-resumed handshake, we need to subtract
-            // the non-resumed handshake's instructions
-            handshake_bench
-                .exclude_setup_instructions(format!("handshake_no_resume_{params_label}"))
-        } else {
-            handshake_bench
-        };
-
         benchmarks.push(handshake_bench);
     }
 
     // Benchmark data transfer
-    benchmarks.push(
-        Benchmark::new(
-            format!("transfer_no_resume_{params_label}"),
-            BenchmarkKind::Transfer,
-            params.clone(),
-        )
-        .exclude_setup_instructions(format!("handshake_no_resume_{params_label}")),
-    );
+    benchmarks.push(Benchmark::new(
+        format!("transfer_no_resume_{params_label}"),
+        BenchmarkKind::Transfer,
+        params.clone(),
+    ));
 }
 
-/// Run all the provided benches under cachegrind to retrieve their instruction count
+/// Run all the provided benches under callgrind to retrieve their instruction count
 pub fn run_all(
     executable: String,
     output_dir: PathBuf,
     benches: &[Benchmark],
 ) -> anyhow::Result<Vec<(String, u64)>> {
     // Run the benchmarks in parallel
-    let cachegrind = CachegrindRunner::new(executable, output_dir)?;
+    let runner = CallgrindRunner::new(executable, output_dir)?;
     let results: Vec<_> = benches
         .par_iter()
         .enumerate()
-        .map(|(i, bench)| (bench, cachegrind.run_bench(i as u32, bench)))
+        .map(|(i, bench)| (bench, runner.run_bench(i as u32, bench)))
         .collect();
 
     // Report possible errors
@@ -476,6 +481,7 @@ trait BenchStepper {
     async fn handshake(&mut self) -> anyhow::Result<Self::Endpoint>;
     async fn sync_before_resumed_handshake(&mut self) -> anyhow::Result<()>;
     async fn transmit_data(&mut self, endpoint: &mut Self::Endpoint) -> anyhow::Result<()>;
+    fn handshake_kind(&self, endpoint: &Self::Endpoint) -> HandshakeKind;
 }
 
 /// Stepper fields necessary for IO
@@ -503,7 +509,7 @@ impl ClientSideStepper<'_> {
         );
 
         let mut cfg = ClientConfig::builder_with_provider(
-            rustls::crypto::CryptoProvider {
+            CryptoProvider {
                 cipher_suites: vec![params.ciphersuite],
                 ..params.provider.clone()
             }
@@ -566,6 +572,10 @@ impl BenchStepper for ClientSideStepper<'_> {
         assert_eq!(total_plaintext_read, TRANSFER_PLAINTEXT_SIZE);
         Ok(())
     }
+
+    fn handshake_kind(&self, endpoint: &Self::Endpoint) -> HandshakeKind {
+        endpoint.handshake_kind().unwrap()
+    }
 }
 
 /// A benchmark stepper for the server-side of the connection
@@ -624,21 +634,28 @@ impl BenchStepper for ServerSideStepper<'_> {
         write_all_plaintext_bounded(endpoint, self.io.writer, TRANSFER_PLAINTEXT_SIZE).await?;
         Ok(())
     }
+
+    fn handshake_kind(&self, endpoint: &Self::Endpoint) -> HandshakeKind {
+        endpoint.handshake_kind().unwrap()
+    }
 }
 
 /// Runs the benchmark using the provided stepper
 async fn run_bench<T: BenchStepper>(mut stepper: T, kind: BenchmarkKind) -> anyhow::Result<()> {
-    let mut endpoint = stepper.handshake().await?;
-
     match kind {
         BenchmarkKind::Handshake(ResumptionKind::No) => {
-            // Nothing else to do here, since the handshake already happened
-            black_box(endpoint);
+            // Just count instructions for one handshake.
+            let _count = CountInstructions::start();
+            black_box(stepper.handshake().await?);
         }
         BenchmarkKind::Handshake(_) => {
-            // The handshake performed above was non-resumed, because the client didn't have a
-            // session ID / ticket; from now on we can perform resumed handshakes. We do it multiple
+            // The first handshake performed is non-resumed, because the client didn't have a
+            // session ID / ticket.  This is not measured.
+            stepper.handshake().await?;
+
+            // From now on we can perform resumed handshakes. We do it multiple
             // times, for reasons explained in the comments to `RESUMED_HANDSHAKE_RUNS`.
+            let _count = CountInstructions::start();
             for _ in 0..RESUMED_HANDSHAKE_RUNS {
                 // Wait for the endpoints to sync (i.e. the server must have discarded the previous
                 // connection and be ready for a new handshake, otherwise the client will start a
@@ -647,10 +664,14 @@ async fn run_bench<T: BenchStepper>(mut stepper: T, kind: BenchmarkKind) -> anyh
                 stepper
                     .sync_before_resumed_handshake()
                     .await?;
-                stepper.handshake().await?;
+                let endpoint = stepper.handshake().await?;
+                assert_eq!(stepper.handshake_kind(&endpoint), HandshakeKind::Resumed);
             }
         }
         BenchmarkKind::Transfer => {
+            // Measurement includes the transfer, but not the handshake.
+            let mut endpoint = stepper.handshake().await?;
+            let _count = CountInstructions::start();
             stepper
                 .transmit_data(&mut endpoint)
                 .await?;
@@ -664,10 +685,8 @@ async fn run_bench<T: BenchStepper>(mut stepper: T, kind: BenchmarkKind) -> anyh
 struct CompareResult {
     /// Results for benchmark scenarios we know are fairly deterministic.
     ///
-    /// The string is a detailed diff between the instruction counts obtained from cachegrind.
+    /// The string is a detailed diff between the instruction counts obtained from callgrind.
     diffs: Vec<(Diff, String)>,
-    /// Results for benchmark scenarios we know are extremely non-deterministic
-    known_noisy: Vec<Diff>,
     /// Benchmark scenarios present in the candidate but missing in the baseline
     missing_in_baseline: Vec<String>,
 }
@@ -720,7 +739,6 @@ fn compare_results(
 ) -> anyhow::Result<CompareResult> {
     let mut diffs = Vec::new();
     let mut missing = Vec::new();
-    let mut known_noisy = Vec::new();
 
     for (scenario, &instr_count) in candidate {
         let Some(&baseline_instr_count) = baseline.get(scenario) else {
@@ -738,10 +756,7 @@ fn compare_results(
             diff_ratio,
         };
 
-        match is_known_noisy(scenario) {
-            true => known_noisy.push(diff),
-            false => diffs.push(diff),
-        };
+        diffs.push(diff);
     }
 
     diffs.sort_by(|diff1, diff2| {
@@ -751,35 +766,18 @@ fn compare_results(
             .total_cmp(&diff1.diff_ratio.abs())
     });
 
-    let mut diffs_with_cachegrind_diff = Vec::new();
+    let mut diffs_with_callgrind_diff = Vec::new();
     for diff in diffs {
-        let detailed_diff = cachegrind::diff(baseline_dir, candidate_dir, &diff.scenario)?;
-        diffs_with_cachegrind_diff.push((diff, detailed_diff));
+        let detailed_diff = callgrind::diff(baseline_dir, candidate_dir, &diff.scenario)?;
+        diffs_with_callgrind_diff.push((diff, detailed_diff));
     }
 
     Ok(CompareResult {
-        diffs: diffs_with_cachegrind_diff,
+        diffs: diffs_with_callgrind_diff,
         missing_in_baseline: missing,
-        known_noisy,
     })
 }
 
-fn is_known_noisy(scenario_name: &str) -> bool {
-    // aws-lc-rs RSA key validation is non-deterministic, and expensive in relative terms for
-    // "cheaper" tests, and only done for server-side tests.  Exclude these tests
-    // from comparison.
-    //
-    // Better solutions for this include:
-    // - https://github.com/rustls/rustls/issues/1494: exclude key validation in these tests.
-    //   Key validation is benchmarked separately elsewhere, and mostly amortised into
-    //   insignificance in real-world scenarios.
-    // - Find a way to make aws-lc-rs deterministic, such as by replacing its RNG with a
-    //   test-only one.
-    scenario_name.contains("_aws_lc_rs_")
-        && scenario_name.contains("_rsa_")
-        && scenario_name.ends_with("_server")
-}
-
 /// Prints a report of the comparison to stdout, using GitHub-flavored markdown
 fn print_report(result: &CompareResult) {
     println!("# Benchmark results");
@@ -815,14 +813,6 @@ fn print_report(result: &CompareResult) {
         }
         println!("</details>\n")
     }
-
-    if !result.known_noisy.is_empty() {
-        println!("### ‼️ Caution: ignored noisy benchmarks");
-        println!("<details>");
-        println!("<summary>Click to expand</summary>\n");
-        table(result.known_noisy.iter(), false);
-        println!("</details>\n")
-    }
 }
 
 /// Renders the diffs as a markdown table
diff --git a/rustls/src/manual/howto.rs b/rustls/src/manual/howto.rs
index 826f5a51364..f3a8219cdf0 100644
--- a/rustls/src/manual/howto.rs
+++ b/rustls/src/manual/howto.rs
@@ -17,21 +17,21 @@ It's a TODO to make these and other extension points async.)
 
 Once you have these two pieces, configuring a server to use them involves, briefly:
 
-- packaging your `sign::SigningKey` with the matching certificate chain into a [`sign::CertifiedKey`][certified_key]
-- making a [`ResolvesServerCertUsingSni`][cert_using_sni] and feeding in your `sign::CertifiedKey` for all SNI hostnames you want to use it for,
+- packaging your [`sign::SigningKey`][signing_key] with the matching certificate chain into a [`sign::CertifiedKey`][certified_key]
+- making a [`ResolvesServerCertUsingSni`][cert_using_sni] and feeding in your [`sign::CertifiedKey`][certified_key] for all SNI hostnames you want to use it for,
 - setting that as your `ServerConfig`'s [`cert_resolver`][cert_resolver]
 
-For a complete example of implementing a custom `sign::SigningKey` and `sign::Signer` see
-the [rustls-cng] crate.
-
-[signing_key]: ../../sign/trait.SigningKey.html
-[choose_scheme]: ../../sign/trait.SigningKey.html#tymethod.choose_scheme
-[sig_scheme]: ../../enum.SignatureScheme.html
-[signer]: ../../sign/trait.Signer.html
-[sign_method]: ../../sign/trait.Signer.html#tymethod.sign
-[certified_key]: ../../sign/struct.CertifiedKey.html
-[cert_using_sni]: ../../struct.ResolvesServerCertUsingSni.html
-[cert_resolver]: ../../struct.ServerConfig.html#structfield.cert_resolver
+For a complete example of implementing a custom [`sign::SigningKey`][signing_key]
+and [`sign::Signer`][signer] see the [rustls-cng] crate.
+
+[signing_key]: crate::crypto::signer::SigningKey
+[choose_scheme]: crate::crypto::signer::SigningKey.choose_scheme()
+[sig_scheme]: crate::SignatureScheme
+[signer]: crate::crypto::signer::Signer
+[sign_method]: crate::crypto::Signer.sign()
+[certified_key]: crate::crypto::signer::CertifiedKey
+[cert_using_sni]: crate::server::ResolvesServerCertUsingSni
+[cert_resolver]: crate::ServerConfig::cert_resolver
 [rustls-cng]: https://github.com/rustls/rustls-cng/blob/dev/src/signer.rs
 
 [^1]: For PKCS#8 it does not support password encryption -- there's not a meaningful threat


Test patch: