From a301550f1d916e014f6759b7fdf971b763f16899 Mon Sep 17 00:00:00 2001 From: Luc Varoqui Date: Thu, 1 Oct 2026 16:50:27 +0200 Subject: [PATCH] feat(instrument-hooks): declare benchmarks run in another process Add set_executed_benchmark_for_pid, which passes an explicit pid to instrument_hooks_set_executed_benchmark instead of the calling process' own. set_executed_benchmark keeps its behavior and delegates to it. Bump instrument-hooks, whose valgrind instrument now writes a "Benchmark pid: " desc line in the dump part when that pid is not the calling process'. This also brings thread-safe C API exports and the callgrind_toggle_collect helper. Refs COD-3722 Co-Authored-By: Claude Opus 5.5 --- crates/exec-harness/src/analysis/mod.rs | 11 +++-- .../instrument-hooks | 2 +- crates/instrument-hooks-bindings/src/lib.rs | 18 +++++++- src/binary_pins.rs | 10 ++--- src/executor/valgrind/measure.rs | 7 +++ src/executor/valgrind/setup.rs | 8 ++-- tests/docker/run.sh | 2 +- tests/executors.rs | 45 +++++++++++++++++++ 8 files changed, 87 insertions(+), 16 deletions(-) diff --git a/crates/exec-harness/src/analysis/mod.rs b/crates/exec-harness/src/analysis/mod.rs index ec6132ca6..b4e41e6e6 100644 --- a/crates/exec-harness/src/analysis/mod.rs +++ b/crates/exec-harness/src/analysis/mod.rs @@ -18,15 +18,20 @@ pub fn perform(commands: Vec) -> Result<()> { cmd.args(&benchmark_cmd.command[1..]); hooks.start_benchmark().unwrap(); - let status = cmd.status(); + let result = cmd.spawn().and_then(|mut child| { + let pid = child.id(); + child.wait().map(|status| (pid, status)) + }); hooks.stop_benchmark().unwrap(); - let status = status.context("Failed to execute command")?; + let (pid, status) = result.context("Failed to execute command")?; if !status.success() { bail!("Command exited with non-zero status: {status}"); } - hooks.set_executed_benchmark(&name_and_uri.uri).unwrap(); + hooks + .set_executed_benchmark_for_pid(pid, &name_and_uri.uri) + .unwrap(); } Ok(()) diff --git a/crates/instrument-hooks-bindings/instrument-hooks b/crates/instrument-hooks-bindings/instrument-hooks index b9ddb5bc6..d38f44cbc 160000 --- a/crates/instrument-hooks-bindings/instrument-hooks +++ b/crates/instrument-hooks-bindings/instrument-hooks @@ -1 +1 @@ -Subproject commit b9ddb5bc654b2e6fa13eb18efcd3a45e7ecda0bb +Subproject commit d38f44cbc992b58920440d8c80f6c1222d6657e2 diff --git a/crates/instrument-hooks-bindings/src/lib.rs b/crates/instrument-hooks-bindings/src/lib.rs index 8ea620cb8..09a031dd7 100644 --- a/crates/instrument-hooks-bindings/src/lib.rs +++ b/crates/instrument-hooks-bindings/src/lib.rs @@ -58,10 +58,15 @@ mod linux_impl { #[inline(always)] pub fn set_executed_benchmark(&self, uri: &str) -> Result<(), u8> { - let pid = std::process::id() as i32; + self.set_executed_benchmark_for_pid(std::process::id(), uri) + } + + /// Declares a benchmark that ran in the process `pid` rather than in this one. + #[inline(always)] + pub fn set_executed_benchmark_for_pid(&self, pid: u32, uri: &str) -> Result<(), u8> { let c_uri = CString::new(uri).map_err(|_| 1u8)?; let result = unsafe { - ffi::instrument_hooks_set_executed_benchmark(self.0, pid, c_uri.as_ptr()) + ffi::instrument_hooks_set_executed_benchmark(self.0, pid as i32, c_uri.as_ptr()) }; if result == 0 { Ok(()) } else { Err(result) } } @@ -184,6 +189,10 @@ mod other_impl { Ok(()) } + pub fn set_executed_benchmark_for_pid(&self, _pid: u32, _uri: &str) -> Result<(), u8> { + Ok(()) + } + pub fn set_integration(&self, _name: &str, _version: &str) -> Result<(), u8> { Ok(()) } @@ -213,6 +222,11 @@ mod tests { let hooks = InstrumentHooks::instance("test_integration", "1.0.0"); assert!(!hooks.is_instrumented() || hooks.start_benchmark().is_ok()); assert!(hooks.set_executed_benchmark("test_uri").is_ok()); + assert!( + hooks + .set_executed_benchmark_for_pid(std::process::id() + 1, "test_uri") + .is_ok() + ); assert!(hooks.set_integration("test_integration", "1.0.0").is_ok()); let start = InstrumentHooks::current_timestamp(); let end = start + 1_000_000; // Simulate 1ms later diff --git a/src/binary_pins.rs b/src/binary_pins.rs index 355d725e8..6bfc4458c 100644 --- a/src/binary_pins.rs +++ b/src/binary_pins.rs @@ -13,7 +13,7 @@ pub const VALGRIND_CODSPEED_VERSION: Version = Version::new(3, 26, 0); /// the .deb is repackaged without a new upstream valgrind release. Appears in /// the .deb package version (`3.26.0-0codspeed3`) and in `valgrind --version` /// output (`valgrind-3.26.0.codspeed3`). -pub const VALGRIND_CODSPEED_ITERATION: u32 = 7; +pub const VALGRIND_CODSPEED_ITERATION: u32 = 8; /// Suffix appended to `VALGRIND_CODSPEED_VERSION` to form the .deb package version. static VALGRIND_DEB_REV: LazyLock = LazyLock::new(|| format!("0codspeed{VALGRIND_CODSPEED_ITERATION}")); @@ -93,16 +93,16 @@ impl ValgrindTarget { fn sha256(self) -> &'static str { match (self.distro_version, self.arch) { (DistroVersion::Ubuntu2204, Arch::Amd64) => { - "658a64049b6a1f5bec9c038b8036f9a61fee83011a909b649e32a82edab4ba99" + "9836998817cd6d12e86bf60a230634f9f11594a1d21fcbd580a47b693ec5b6d1" } (DistroVersion::Ubuntu2404, Arch::Amd64) => { - "ea1788e43cfd75b8af84dd496eb265307c6ab6d048cb7bc8ba4bc564dbbd5aba" + "edee86d0d61b46435ea3f4a73ffb222fcf8d605a9e3901638f280197c3c3d602" } (DistroVersion::Ubuntu2204, Arch::Arm64) => { - "6db07f15ce23e3cfda00bda85aac0ea3b8a2261c00d529ed856ceb7f3ba207f6" + "453e2455a3a924c2b2e93201336fb9b7904f1154bf9304bf910da884cee27052" } (DistroVersion::Ubuntu2404, Arch::Arm64) => { - "0bccdaf2da7202ff4fe15e8301641c3a0abd46879c03713150cb5f21d6c9baaf" + "1884667fe9da4f991476b97108d021341b0456ebc5185430a1748e57f4cccc52" } } } diff --git a/src/executor/valgrind/measure.rs b/src/executor/valgrind/measure.rs index 36fbdc0d6..e48405e26 100644 --- a/src/executor/valgrind/measure.rs +++ b/src/executor/valgrind/measure.rs @@ -63,6 +63,13 @@ fn get_valgrind_args(tool: &SimulationTool, config: &ExecutorConfig) -> Vec /dev/null"]) + .assert() + .success(); + + let profiles: Vec<(String, String)> = std::fs::read_dir(profile_folder.path()) + .unwrap() + .map(|entry| entry.unwrap().path()) + .filter(|path| path.extension().is_some_and(|ext| ext == "out")) + .map(|path| { + let pid = path.file_stem().unwrap().to_string_lossy().into_owned(); + (pid, std::fs::read_to_string(&path).unwrap()) + }) + .collect(); + + let declarations: Vec<(&str, &str)> = profiles + .iter() + .flat_map(|(pid, content)| { + content + .lines() + .filter_map(|line| line.strip_prefix("desc: Benchmark pid: ")) + .map(move |benchmark_pid| (pid.as_str(), benchmark_pid)) + }) + .collect(); + let [(harness_pid, benchmark_pid)] = declarations.as_slice() else { + panic!("expected exactly one benchmark pid declaration, got {declarations:?}"); + }; + + assert_ne!(harness_pid, benchmark_pid); + assert!( + profiles.iter().any(|(pid, _)| pid == benchmark_pid), + "benchmark pid {benchmark_pid} has no profile among {:?}", + profiles.iter().map(|(pid, _)| pid).collect::>() + ); +} + /// Prepends `prefix` to `var` and checks the benchmark still sees it. fn memory_forwards_path_like(var: &str, prefix: &str) { let value = match std::env::var(var).unwrap_or_default() {