From e7fa40405768cc63d1b4195b6ce124cb2b510454 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Tue, 23 Sep 2025 15:29:07 +0200 Subject: [PATCH 1/3] fix: cargo clippy lints --- src/api_client.rs | 6 +++--- src/config.rs | 2 +- src/run/config.rs | 2 +- src/run/helpers/download_file.rs | 13 ++++--------- src/run/helpers/get_env_var.rs | 2 +- src/run/helpers/parse_git_remote.rs | 5 +---- src/run/instruments/mod.rs | 2 +- src/run/run_environment/gitlab_ci/provider.rs | 2 +- src/run/run_environment/local/provider.rs | 5 +---- src/run/runner/valgrind/measure.rs | 13 +++++-------- src/run/runner/wall_time/executor.rs | 9 ++++----- 11 files changed, 23 insertions(+), 38 deletions(-) diff --git a/src/api_client.rs b/src/api_client.rs index 611798c9..7439c4be 100644 --- a/src/api_client.rs +++ b/src/api_client.rs @@ -170,7 +170,7 @@ impl CodSpeedAPIClient { .await; match response { Ok(response) => Ok(response.create_login_session), - Err(err) => bail!("Failed to create login session: {}", err), + Err(err) => bail!("Failed to create login session: {err}"), } } @@ -189,7 +189,7 @@ impl CodSpeedAPIClient { .await; match response { Ok(response) => Ok(response.consume_login_session), - Err(err) => bail!("Failed to use login session: {}", err), + Err(err) => bail!("Failed to use login session: {err}"), } } @@ -212,7 +212,7 @@ impl CodSpeedAPIClient { Err(err) if err.contains_error_code("UNAUTHENTICATED") => { bail!("Your session has expired, please login again using `codspeed auth login`") } - Err(err) => bail!("Failed to fetch local run report: {}", err), + Err(err) => bail!("Failed to fetch local run report: {err}"), } } } diff --git a/src/config.rs b/src/config.rs index bac05e85..729c5915 100644 --- a/src/config.rs +++ b/src/config.rs @@ -56,7 +56,7 @@ impl CodSpeedConfig { debug!("Config file not found at {}", config_path.display()); CodSpeedConfig::default() } - Err(e) => bail!("Failed to load config: {}", e), + Err(e) => bail!("Failed to load config: {e}"), }; if let Some(oauth_token) = oauth_token_override { diff --git a/src/run/config.rs b/src/run/config.rs index ffa7e003..cc71fd88 100644 --- a/src/run/config.rs +++ b/src/run/config.rs @@ -70,7 +70,7 @@ impl TryFrom for Config { let instruments = Instruments::try_from(&args)?; let raw_upload_url = args.upload_url.unwrap_or_else(|| DEFAULT_UPLOAD_URL.into()); let upload_url = Url::parse(&raw_upload_url) - .map_err(|e| anyhow!("Invalid upload URL: {}, {}", raw_upload_url, e))?; + .map_err(|e| anyhow!("Invalid upload URL: {raw_upload_url}, {e}"))?; Ok(Self { upload_url, diff --git a/src/run/helpers/download_file.rs b/src/run/helpers/download_file.rs index e07c2b19..ce9df1d8 100644 --- a/src/run/helpers/download_file.rs +++ b/src/run/helpers/download_file.rs @@ -9,7 +9,7 @@ pub async fn download_file(url: &Url, path: &Path) -> Result<()> { .get(url.clone()) .send() .await - .map_err(|e| anyhow!("Failed to download file: {}", e))?; + .map_err(|e| anyhow!("Failed to download file: {e}"))?; if !response.status().is_success() { bail!("Failed to download file: {}", response.status()); } @@ -18,13 +18,8 @@ pub async fn download_file(url: &Url, path: &Path) -> Result<()> { let content = response .bytes() .await - .map_err(|e| anyhow!("Failed to read response: {}", e))?; - std::io::copy(&mut content.as_ref(), &mut file).map_err(|e| { - anyhow!( - "Failed to write to file: {}, {}", - path.display(), - e.to_string() - ) - })?; + .map_err(|e| anyhow!("Failed to read response: {e}"))?; + std::io::copy(&mut content.as_ref(), &mut file) + .map_err(|e| anyhow!("Failed to write to file: {}, {}", path.display(), e))?; Ok(()) } diff --git a/src/run/helpers/get_env_var.rs b/src/run/helpers/get_env_var.rs index 5380c7c7..fb61274a 100644 --- a/src/run/helpers/get_env_var.rs +++ b/src/run/helpers/get_env_var.rs @@ -3,7 +3,7 @@ use anyhow::anyhow; use std::env; pub fn get_env_variable(name: &str) -> Result { - env::var(name).map_err(|_| anyhow!("{} environment variable not found", name)) + env::var(name).map_err(|_| anyhow!("{name} environment variable not found")) } #[cfg(test)] diff --git a/src/run/helpers/parse_git_remote.rs b/src/run/helpers/parse_git_remote.rs index 5447ec34..027fcbd6 100644 --- a/src/run/helpers/parse_git_remote.rs +++ b/src/run/helpers/parse_git_remote.rs @@ -17,10 +17,7 @@ pub struct GitRemote { pub fn parse_git_remote(remote: &str) -> Result { let captures = REMOTE_REGEX.captures(remote).ok_or_else(|| { - anyhow!( - "Could not extract owner and repository from remote url: {}", - remote - ) + anyhow!("Could not extract owner and repository from remote url: {remote}") })?; let domain = captures.name("domain").unwrap().as_str(); diff --git a/src/run/instruments/mod.rs b/src/run/instruments/mod.rs index 48b60d37..da34e06e 100644 --- a/src/run/instruments/mod.rs +++ b/src/run/instruments/mod.rs @@ -47,7 +47,7 @@ impl TryFrom<&RunArgs> for Instruments { for instrument_name in &args.instruments { match instrument_name.as_str() { "mongodb" => validated_instrument_names.insert(InstrumentName::MongoDB), - _ => bail!("Invalid instrument name: {}", instrument_name), + _ => bail!("Invalid instrument name: {instrument_name}"), }; } diff --git a/src/run/run_environment/gitlab_ci/provider.rs b/src/run/run_environment/gitlab_ci/provider.rs index b3e2017e..86b6b3c1 100644 --- a/src/run/run_environment/gitlab_ci/provider.rs +++ b/src/run/run_environment/gitlab_ci/provider.rs @@ -101,7 +101,7 @@ impl TryFrom<&Config> for GitLabCIProvider { None, ), - _ => bail!("Event {} is not supported by CodSpeed", ci_pipeline_source), + _ => bail!("Event {ci_pipeline_source} is not supported by CodSpeed"), }; let run_id = get_env_variable("CI_JOB_ID")?; diff --git a/src/run/run_environment/local/provider.rs b/src/run/run_environment/local/provider.rs index 85999fab..783c5cf8 100644 --- a/src/run/run_environment/local/provider.rs +++ b/src/run/run_environment/local/provider.rs @@ -191,10 +191,7 @@ fn extract_provider_owner_and_repository_from_remote_url( let repository_provider = match domain.as_str() { "github.com" => RepositoryProvider::GitHub, "gitlab.com" => RepositoryProvider::GitLab, - domain => bail!( - "Repository provider {} is not supported by CodSpeed", - domain - ), + domain => bail!("Repository provider {domain} is not supported by CodSpeed"), }; Ok(( diff --git a/src/run/runner/valgrind/measure.rs b/src/run/runner/valgrind/measure.rs index 262e471b..460010a2 100644 --- a/src/run/runner/valgrind/measure.rs +++ b/src/run/runner/valgrind/measure.rs @@ -93,10 +93,10 @@ pub async fn measure( format!( "{}:{}:{}", introspected_nodejs::setup() - .map_err(|e| anyhow!("failed to setup NodeJS introspection. {}", e))? + .map_err(|e| anyhow!("failed to setup NodeJS introspection. {e}"))? .to_string_lossy(), introspected_golang::setup() - .map_err(|e| anyhow!("failed to setup Go introspection. {}", e))? + .map_err(|e| anyhow!("failed to setup Go introspection. {e}"))? .to_string_lossy(), env::var("PATH").unwrap_or_default(), ), @@ -136,7 +136,7 @@ pub async fn measure( debug!("cmd: {cmd:?}"); let status = run_command_with_log_pipe(cmd) .await - .map_err(|e| anyhow!("failed to execute the benchmark process. {}", e))?; + .map_err(|e| anyhow!("failed to execute the benchmark process. {e}"))?; debug!( "Valgrind exit code = {:?}, Valgrind signal = {:?}", status.code(), @@ -157,14 +157,11 @@ pub async fn measure( let content = std::fs::read_to_string(&cmd_status_path)?; content .parse::() - .map_err(|e| anyhow!("unable to retrieve the program exit code. {}", e))? + .map_err(|e| anyhow!("unable to retrieve the program exit code. {e}"))? }; debug!("Program exit code = {cmd_status}"); if cmd_status != 0 { - bail!( - "failed to execute the benchmark process, exit code: {}", - cmd_status - ); + bail!("failed to execute the benchmark process, exit code: {cmd_status}"); } Ok(()) diff --git a/src/run/runner/wall_time/executor.rs b/src/run/runner/wall_time/executor.rs index f89539e4..8c6d9854 100644 --- a/src/run/runner/wall_time/executor.rs +++ b/src/run/runner/wall_time/executor.rs @@ -113,10 +113,10 @@ impl WallTimeExecutor { let path_env = format!( "export PATH={}:{}:{}", introspected_nodejs::setup() - .map_err(|e| anyhow!("failed to setup NodeJS introspection. {}", e))? + .map_err(|e| anyhow!("failed to setup NodeJS introspection. {e}"))? .to_string_lossy(), introspected_golang::setup() - .map_err(|e| anyhow!("failed to setup Go introspection. {}", e))? + .map_err(|e| anyhow!("failed to setup Go introspection. {e}"))? .to_string_lossy(), path_env ); @@ -200,12 +200,11 @@ impl Executor for WallTimeExecutor { } }; - let status = - status.map_err(|e| anyhow!("failed to execute the benchmark process. {}", e))?; + let status = status.map_err(|e| anyhow!("failed to execute the benchmark process. {e}"))?; debug!("cmd exit status: {:?}", status); if !status.success() { - bail!("failed to execute the benchmark process: {}", status); + bail!("failed to execute the benchmark process: {status}"); } Ok(()) From 664c01085c1ff0dcb380aabdb36245601da7016f Mon Sep 17 00:00:00 2001 From: not-matthias Date: Tue, 23 Sep 2025 15:34:41 +0200 Subject: [PATCH 2/3] feat: add runner-shared crate --- Cargo.lock | 37 +++++++-- Cargo.toml | 6 +- crates/runner-shared/Cargo.toml | 9 +++ crates/runner-shared/src/lib.rs | 3 + .../runner-shared/src}/metadata.rs | 0 crates/runner-shared/src/unwind_data.rs | 49 ++++++++++++ src/run/runner/wall_time/perf/fifo.rs | 3 +- src/run/runner/wall_time/perf/helpers.rs | 71 ----------------- src/run/runner/wall_time/perf/jit_dump.rs | 3 +- src/run/runner/wall_time/perf/shared.rs | 15 ---- src/run/runner/wall_time/perf/unwind_data.rs | 77 +++++-------------- 11 files changed, 119 insertions(+), 154 deletions(-) create mode 100644 crates/runner-shared/Cargo.toml create mode 100644 crates/runner-shared/src/lib.rs rename {src/run/runner/wall_time/perf => crates/runner-shared/src}/metadata.rs (100%) create mode 100644 crates/runner-shared/src/unwind_data.rs delete mode 100644 src/run/runner/wall_time/perf/helpers.rs delete mode 100644 src/run/runner/wall_time/perf/shared.rs diff --git a/Cargo.lock b/Cargo.lock index 2879f846..f2da33cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -93,9 +93,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.97" +version = "1.0.100" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dcfed56ad506cb2c684a14971b8861fdc3baaaae314b9e5f9bb532cbe3ba7a4f" +checksum = "a23eb6b1614318a8071c9b2521f36b424b2c83db5eb3a0fead4a6c0809af6e61" [[package]] name = "async-compression" @@ -317,6 +317,7 @@ dependencies = [ "reqwest-retry", "rstest", "rstest_reuse", + "runner-shared", "serde", "serde_json", "serde_yaml", @@ -1891,6 +1892,15 @@ dependencies = [ "syn 2.0.96", ] +[[package]] +name = "runner-shared" +version = "0.1.0" +dependencies = [ + "anyhow", + "serde", + "serde_json", +] + [[package]] name = "rustc-demangle" version = "0.1.24" @@ -1995,18 +2005,28 @@ checksum = "56e6fa9c48d24d85fb3de5ad847117517440f6beceb7798af16b4a87d616b8d0" [[package]] name = "serde" -version = "1.0.219" +version = "1.0.225" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fd6c24dee235d0da097043389623fb913daddf92c76e9f5a1db88607a0bcbd1d" +dependencies = [ + "serde_core", + "serde_derive", +] + +[[package]] +name = "serde_core" +version = "1.0.225" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f0e2c6ed6606019b4e29e69dbaba95b11854410e5347d525002456dbbb786b6" +checksum = "659356f9a0cb1e529b24c01e43ad2bdf520ec4ceaf83047b83ddcc2251f96383" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.219" +version = "1.0.225" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b0276cf7f2c73365f7157c8123c21cd9a50fbbd844757af28ca1f5925fc2a00" +checksum = "0ea936adf78b1f766949a4977b91d2f5595825bd6ec079aa9543ad2685fc4516" dependencies = [ "proc-macro2", "quote", @@ -2015,15 +2035,16 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.140" +version = "1.0.145" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "20068b6e96dc6c9bd23e01df8827e6c7e1f2fddd43c21810382803c136b99373" +checksum = "402a6f66d8c709116cf22f558eab210f5a50187f702eb4d7e5ef38d9a7f1c79c" dependencies = [ "indexmap", "itoa", "memchr", "ryu", "serde", + "serde_core", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index ccd18597..80cac1b7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,8 +53,9 @@ object = "0.36.7" linux-perf-data = "0.11.0" debugid = "0.8.0" memmap2 = "0.9.5" -nix = { version = "0.29.0", features = ["fs", "user"] } +nix = { version = "0.29.0", features = ["fs", "time", "user"] } futures = "0.3.31" +runner-shared = { path = "crates/runner-shared" } [target.'cfg(target_os = "linux")'.dependencies] procfs = "0.17.0" @@ -67,6 +68,9 @@ rstest = { version = "0.25.0", default-features = false } rstest_reuse = "0.7.0" shell-quote = "0.7.2" +[workspace] +members = ["crates/runner-shared"] + [workspace.metadata.release] sign-tag = true sign-commit = true diff --git a/crates/runner-shared/Cargo.toml b/crates/runner-shared/Cargo.toml new file mode 100644 index 00000000..1c9cb96e --- /dev/null +++ b/crates/runner-shared/Cargo.toml @@ -0,0 +1,9 @@ +[package] +name = "runner-shared" +version = "0.1.0" +edition = "2024" + +[dependencies] +anyhow = "1.0.100" +serde = { version = "1.0.225", features = ["derive"] } +serde_json = "1.0.145" diff --git a/crates/runner-shared/src/lib.rs b/crates/runner-shared/src/lib.rs new file mode 100644 index 00000000..3118fd02 --- /dev/null +++ b/crates/runner-shared/src/lib.rs @@ -0,0 +1,3 @@ +pub mod fifo; +pub mod metadata; +pub mod unwind_data; diff --git a/src/run/runner/wall_time/perf/metadata.rs b/crates/runner-shared/src/metadata.rs similarity index 100% rename from src/run/runner/wall_time/perf/metadata.rs rename to crates/runner-shared/src/metadata.rs diff --git a/crates/runner-shared/src/unwind_data.rs b/crates/runner-shared/src/unwind_data.rs new file mode 100644 index 00000000..4c40cfeb --- /dev/null +++ b/crates/runner-shared/src/unwind_data.rs @@ -0,0 +1,49 @@ +use core::{ + fmt::Debug, + hash::{Hash, Hasher}, +}; +use serde::{Deserialize, Serialize}; +use std::{hash::DefaultHasher, ops::Range}; + +/// Unwind data for a single module. +#[derive(Serialize, Deserialize)] +pub struct UnwindData { + pub path: String, + + pub avma_range: Range, + pub base_avma: u64, + + pub eh_frame_hdr: Vec, + pub eh_frame_hdr_svma: Range, + + pub eh_frame: Vec, + pub eh_frame_svma: Range, +} + +impl Debug for UnwindData { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let eh_frame_hdr_hash = { + let mut hasher = DefaultHasher::new(); + self.eh_frame_hdr.hash(&mut hasher); + hasher.finish() + }; + let eh_frame_hash = { + let mut hasher = DefaultHasher::new(); + self.eh_frame.hash(&mut hasher); + hasher.finish() + }; + + f.debug_struct("UnwindData") + .field("path", &self.path) + .field("avma_range", &format_args!("{:x?}", self.avma_range)) + .field("base_avma", &format_args!("{:x}", self.base_avma)) + .field( + "eh_frame_hdr_svma", + &format_args!("{:x?}", self.eh_frame_hdr_svma), + ) + .field("eh_frame_hdr_hash", &format_args!("{eh_frame_hdr_hash:x}")) + .field("eh_frame_hash", &format_args!("{eh_frame_hash:x}")) + .field("eh_frame_svma", &format_args!("{:x?}", self.eh_frame_svma)) + .finish() + } +} diff --git a/src/run/runner/wall_time/perf/fifo.rs b/src/run/runner/wall_time/perf/fifo.rs index f4b418da..996b9827 100644 --- a/src/run/runner/wall_time/perf/fifo.rs +++ b/src/run/runner/wall_time/perf/fifo.rs @@ -1,4 +1,5 @@ -use super::{FifoCommand, RUNNER_ACK_FIFO, RUNNER_CTL_FIFO}; +use super::FifoCommand; +use runner_shared::fifo::{RUNNER_ACK_FIFO, RUNNER_CTL_FIFO}; use std::path::PathBuf; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::unix::pipe::OpenOptions as TokioPipeOpenOptions; diff --git a/src/run/runner/wall_time/perf/helpers.rs b/src/run/runner/wall_time/perf/helpers.rs deleted file mode 100644 index 2eb8777f..00000000 --- a/src/run/runner/wall_time/perf/helpers.rs +++ /dev/null @@ -1,71 +0,0 @@ -use crate::prelude::*; -use linux_perf_data::{PerfFileReader, PerfFileRecord, linux_perf_event_reader::EventRecord}; -use std::collections::HashMap; - -/// Tries to find the pid of the sampled process within a perf.data file. -pub fn find_pid>(perf_file: P) -> anyhow::Result { - let content = std::fs::read(perf_file.as_ref())?; - let reader = std::io::Cursor::new(content); - - let PerfFileReader { - mut record_iter, - mut perf_file, - } = PerfFileReader::parse_file(reader)?; - - let mut pid_freq = HashMap::new(); - - // Only consider the first N events to reduce the performance impact. Certain benchmark libraries can generate - // more than 100k for each benchmark, which can slow down the runner a lot. The highest chance of finding - // different pids is in the first few events, where there's a possible overlap. - const COUNT_FIRST_N: usize = 1000; - let mut i = 0; - - while let Some(record) = record_iter.next_record(&mut perf_file)? { - let PerfFileRecord::EventRecord { record, .. } = record else { - continue; - }; - - let Ok(parsed_record) = record.parse() else { - continue; - }; - - let EventRecord::Sample(event) = parsed_record else { - continue; - }; - - // Ignore kernel events - if event.pid == Some(-1) { - continue; - } - - if let Some(pid) = event.pid { - *pid_freq.entry(pid).or_insert(0) += 1; - - i += 1; - if i >= COUNT_FIRST_N { - break; - } - } - } - debug!("Pid frequency: {pid_freq:?}"); - - // Choose the pid with the highest frequency. However, we can only use a pid if more than N% of the - // events are from that pid. - let total_count = pid_freq.values().sum::(); - let (pid, pid_count) = pid_freq - .iter() - .max_by_key(|&(_, count)| count) - .ok_or_else(|| anyhow!("Couldn't find pid in perf.data"))?; - log::debug!("Pid frequency: {pid_freq:?}"); - - let pid_percentage = (*pid_count as f64 / total_count as f64) * 100.0; - if pid_percentage < 75.0 { - bail!( - "Most common pid {} only has {:.2}% of total events", - pid, - pid_percentage - ); - } - - Ok(*pid) -} diff --git a/src/run/runner/wall_time/perf/jit_dump.rs b/src/run/runner/wall_time/perf/jit_dump.rs index 32edf2b2..15dbe687 100644 --- a/src/run/runner/wall_time/perf/jit_dump.rs +++ b/src/run/runner/wall_time/perf/jit_dump.rs @@ -2,10 +2,11 @@ use crate::{ prelude::*, run::runner::wall_time::perf::{ perf_map::{ModuleSymbols, Symbol}, - unwind_data::UnwindData, + unwind_data::UnwindDataExt, }, }; use linux_perf_data::jitdump::{JitDumpReader, JitDumpRecord}; +use runner_shared::unwind_data::UnwindData; use std::{ collections::HashSet, path::{Path, PathBuf}, diff --git a/src/run/runner/wall_time/perf/shared.rs b/src/run/runner/wall_time/perf/shared.rs deleted file mode 100644 index 8afe630b..00000000 --- a/src/run/runner/wall_time/perf/shared.rs +++ /dev/null @@ -1,15 +0,0 @@ -//! WARNING: Has to be in sync with `codspeed-rust/codspeed`. - -pub const RUNNER_CTL_FIFO: &str = "/tmp/runner.ctl.fifo"; -pub const RUNNER_ACK_FIFO: &str = "/tmp/runner.ack.fifo"; - -#[derive(serde::Serialize, serde::Deserialize, Debug, PartialEq)] -pub enum Command { - CurrentBenchmark { pid: u32, uri: String }, - StartBenchmark, - StopBenchmark, - Ack, - PingPerf, - SetIntegration { name: String, version: String }, - Err, -} diff --git a/src/run/runner/wall_time/perf/unwind_data.rs b/src/run/runner/wall_time/perf/unwind_data.rs index c7a1be7d..76a0537b 100644 --- a/src/run/runner/wall_time/perf/unwind_data.rs +++ b/src/run/runner/wall_time/perf/unwind_data.rs @@ -1,60 +1,29 @@ //! WARNING: This file has to be in sync with perf-parser! use anyhow::{Context, bail}; -use core::{ - fmt::Debug, - hash::{Hash, Hasher}, -}; use debugid::CodeId; -use serde::{Deserialize, Serialize}; -use std::{hash::DefaultHasher, ops::Range}; +use object::{Object, ObjectSection}; +use runner_shared::unwind_data::UnwindData; +use std::ops::Range; -/// Unwind data for a single module. -#[derive(Serialize, Deserialize)] -pub struct UnwindData { - pub path: String, - - pub avma_range: Range, - pub base_avma: u64, - - pub eh_frame_hdr: Vec, - pub eh_frame_hdr_svma: Range, - - pub eh_frame: Vec, - pub eh_frame_svma: Range, -} - -impl Debug for UnwindData { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let eh_frame_hdr_hash = { - let mut hasher = DefaultHasher::new(); - self.eh_frame_hdr.hash(&mut hasher); - hasher.finish() - }; - let eh_frame_hash = { - let mut hasher = DefaultHasher::new(); - self.eh_frame.hash(&mut hasher); - hasher.finish() - }; +pub trait UnwindDataExt { + fn new( + path_slice: &[u8], + mapping_start_file_offset: u64, + mapping_start_avma: u64, + mapping_size: u64, + build_id: Option<&[u8]>, + ) -> anyhow::Result + where + Self: Sized; - f.debug_struct("UnwindData") - .field("path", &self.path) - .field("avma_range", &format_args!("{:x?}", self.avma_range)) - .field("base_avma", &format_args!("{:x}", self.base_avma)) - .field( - "eh_frame_hdr_svma", - &format_args!("{:x?}", self.eh_frame_hdr_svma), - ) - .field("eh_frame_hdr_hash", &format_args!("{eh_frame_hdr_hash:x}")) - .field("eh_frame_hash", &format_args!("{eh_frame_hash:x}")) - .field("eh_frame_svma", &format_args!("{:x?}", self.eh_frame_svma)) - .finish() - } + fn save_to>(&self, folder: P, pid: u32) -> anyhow::Result<()>; + fn to_file>(&self, path: P) -> anyhow::Result<()>; } -impl UnwindData { +impl UnwindDataExt for UnwindData { // Based on this: https://github.com/mstange/linux-perf-stuff/blob/22ca6531b90c10dd2a4519351c843b8d7958a451/src/main.rs#L747-L893 - pub fn new( + fn new( path_slice: &[u8], mapping_start_file_offset: u64, mapping_start_avma: u64, @@ -80,18 +49,12 @@ impl UnwindData { let file_build_id = CodeId::from_binary(file_build_id); let expected_build_id = CodeId::from_binary(build_id); bail!( - "File {:?} has non-matching build ID {} (expected {})", - path, - file_build_id, - expected_build_id + "File {path:?} has non-matching build ID {file_build_id} (expected {expected_build_id})" ); } } (Some(_), Err(_)) | (Some(_), Ok(None)) => { - bail!( - "File {:?} does not contain a build ID, but we expected it to have one", - path - ); + bail!("File {path:?} does not contain a build ID, but we expected it to have one"); } _ => { // No build id to check @@ -146,7 +109,7 @@ impl UnwindData { }) } - pub fn save_to>(&self, folder: P, pid: u32) -> anyhow::Result<()> { + fn save_to>(&self, folder: P, pid: u32) -> anyhow::Result<()> { let unwind_data_path = folder.as_ref().join(format!( "{}_{:x}_{:x}.unwind", pid, self.avma_range.start, self.avma_range.end From 4c990fde2aa705147b21d77842c2d2a177a26ebf Mon Sep 17 00:00:00 2001 From: not-matthias Date: Tue, 23 Sep 2025 15:39:16 +0200 Subject: [PATCH 3/3] feat: add perf v2 support --- crates/runner-shared/src/fifo.rs | 33 +++ crates/runner-shared/src/metadata.rs | 26 ++- src/run/runner/valgrind/helpers/perf_maps.rs | 5 +- src/run/runner/wall_time/perf/mod.rs | 202 +++++++++---------- src/run/runner/wall_time/perf/perf_map.rs | 7 +- src/run/runner/wall_time/perf/unwind_data.rs | 6 +- 6 files changed, 154 insertions(+), 125 deletions(-) create mode 100644 crates/runner-shared/src/fifo.rs diff --git a/crates/runner-shared/src/fifo.rs b/crates/runner-shared/src/fifo.rs new file mode 100644 index 00000000..b45bbeb7 --- /dev/null +++ b/crates/runner-shared/src/fifo.rs @@ -0,0 +1,33 @@ +//! WARNING: Has to be in sync with `instrument-hooks`. + +pub const RUNNER_CTL_FIFO: &str = "/tmp/runner.ctl.fifo"; +pub const RUNNER_ACK_FIFO: &str = "/tmp/runner.ack.fifo"; + +pub const CURRENT_PROTOCOL_VERSION: u64 = 1; + +/// The different markers that can be set in the perf.data. +/// +/// `SampleStart/End`: Marks the start and end of a sampling period. This is used to differentiate between benchmarks. +/// `BenchmarkStart/End`: Marks the start and end of a benchmark. This is used to measure the duration of a benchmark, without the benchmark harness code. +#[derive( + serde::Serialize, serde::Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Copy, Clone, +)] +pub enum MarkerType { + SampleStart(u64), + SampleEnd(u64), + BenchmarkStart(u64), + BenchmarkEnd(u64), +} + +#[derive(serde::Serialize, serde::Deserialize, Debug, PartialEq)] +pub enum Command { + CurrentBenchmark { pid: i32, uri: String }, + StartBenchmark, + StopBenchmark, + Ack, + PingPerf, + SetIntegration { name: String, version: String }, + Err, + AddMarker { pid: i32, marker: MarkerType }, + SetVersion(u64), +} diff --git a/crates/runner-shared/src/metadata.rs b/crates/runner-shared/src/metadata.rs index e46441f8..61c98d5d 100644 --- a/crates/runner-shared/src/metadata.rs +++ b/crates/runner-shared/src/metadata.rs @@ -1,26 +1,32 @@ -// !!!!!!!!!!!!!!!!!!!!!!!! -// !! DO NOT TOUCH BELOW !! -// !!!!!!!!!!!!!!!!!!!!!!!! -// Has to be in sync with `perf-parser`. -// - -use std::{collections::HashMap, path::Path}; - +use anyhow::Context; use serde::{Deserialize, Serialize}; +use std::path::Path; + +use crate::fifo::MarkerType; #[derive(Serialize, Deserialize)] pub struct PerfMetadata { + /// The version of this metadata format. + pub version: u64, + /// Name and version of the integration pub integration: (String, String), - /// The URIs of the benchmarks in the order they were executed. - pub bench_order_by_pid: HashMap>, + /// The URIs of the benchmarks with the timestamps they were executed at. + pub uri_by_ts: Vec<(u64, String)>, /// Modules that should be ignored and removed from the folded trace and callgraph (e.g. python interpreter) pub ignored_modules: Vec<(String, u64, u64)>, + + /// Marker for certain regions in the profiling data + pub markers: Vec, } impl PerfMetadata { + pub fn from_reader(reader: R) -> anyhow::Result { + serde_json::from_reader(reader).context("Could not parse perf metadata from JSON") + } + pub fn save_to>(&self, path: P) -> anyhow::Result<()> { let file = std::fs::File::create(path.as_ref().join("perf.metadata"))?; serde_json::to_writer(file, self)?; diff --git a/src/run/runner/valgrind/helpers/perf_maps.rs b/src/run/runner/valgrind/helpers/perf_maps.rs index 796ab4c1..9631995a 100644 --- a/src/run/runner/valgrind/helpers/perf_maps.rs +++ b/src/run/runner/valgrind/helpers/perf_maps.rs @@ -21,7 +21,10 @@ pub async fn harvest_perf_maps(profile_folder: &Path) -> Result<()> { harvest_perf_maps_for_pids(profile_folder, &pids).await } -pub async fn harvest_perf_maps_for_pids(profile_folder: &Path, pids: &HashSet) -> Result<()> { +pub async fn harvest_perf_maps_for_pids( + profile_folder: &Path, + pids: &HashSet, +) -> Result<()> { let perf_maps = pids .iter() .map(|pid| format!("perf-{pid}.map")) diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index e0db17e1..4415127f 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -9,35 +9,34 @@ use crate::run::runner::helpers::setup::run_with_sudo; use crate::run::runner::valgrind::helpers::ignored_objects_path::get_objects_path_to_ignore; use crate::run::runner::valgrind::helpers::perf_maps::harvest_perf_maps_for_pids; use crate::run::runner::wall_time::perf::jit_dump::harvest_perf_jit_for_pids; +use crate::run::runner::wall_time::perf::unwind_data::UnwindDataExt; use anyhow::Context; use fifo::{PerfFifo, RunnerFifo}; -use futures::stream::FuturesUnordered; -use metadata::PerfMetadata; +use libc::pid_t; +use nix::sys::time::TimeValLike; +use nix::time::clock_gettime; use perf_map::ProcessSymbols; -use shared::Command as FifoCommand; +use runner_shared::fifo::Command as FifoCommand; +use runner_shared::fifo::MarkerType; +use runner_shared::metadata::PerfMetadata; +use runner_shared::unwind_data::UnwindData; use std::collections::HashSet; use std::path::PathBuf; use std::process::Command; use std::time::Duration; use std::{cell::OnceCell, collections::HashMap, process::ExitStatus}; -use tempfile::TempDir; -use unwind_data::UnwindData; mod jit_dump; -mod metadata; mod setup; -mod shared; -pub use shared::*; pub mod fifo; -pub mod helpers; pub mod perf_map; pub mod unwind_data; -const PERF_DATA_PREFIX: &str = "perf.data."; +const PERF_METADATA_CURRENT_VERSION: u64 = 1; +const PERF_DATA_PATH: &str = "/tmp/perf.data"; pub struct PerfRunner { - perf_dir: TempDir, benchmark_data: OnceCell, } @@ -72,7 +71,6 @@ impl PerfRunner { pub fn new() -> Self { Self { - perf_dir: tempfile::tempdir().expect("Failed to create temporary directory"), benchmark_data: OnceCell::new(), } } @@ -86,13 +84,6 @@ impl PerfRunner { let perf_fifo = PerfFifo::new()?; let runner_fifo = RunnerFifo::new()?; - // We have to pass a file to perf, which will create `perf.data.` files - // when the output is split. - let perf_file = tempfile::Builder::new() - .keep(true) - .prefix(PERF_DATA_PREFIX) - .tempfile_in(&self.perf_dir)?; - // Infer the unwinding mode from the benchmark cmd let (cg_mode, stack_size) = if let Some(mode) = config.perf_unwinding_mode { (mode, None) @@ -115,22 +106,36 @@ impl PerfRunner { }; debug!("Using call graph mode: {cg_mode:?}"); - let quiet_flag = if is_codspeed_debug_enabled() { + let quiet_flag = if !is_codspeed_debug_enabled() { "--quiet" } else { "" }; - cmd.args([ - "sh", - "-c", + let perf_args = [ + "perf", + "record", + quiet_flag, + "--timestamp", + // Required for matching the markers and URIs to the samples. + "-k", + "CLOCK_MONOTONIC", + "--freq=999", + "--delay=-1", + "-g", + "--user-callchains", + &format!("--call-graph={cg_mode}"), &format!( - "perf record {quiet_flag} --user-callchains --freq=999 --switch-output --control=fifo:{},{} --delay=-1 -g --call-graph={cg_mode} --output={} -- {bench_cmd}", + "--control=fifo:{},{}", perf_fifo.ctl_fifo_path.to_string_lossy(), - perf_fifo.ack_fifo_path.to_string_lossy(), - perf_file.path().to_string_lossy(), + perf_fifo.ack_fifo_path.to_string_lossy() ), - ]); + &format!("--output={PERF_DATA_PATH}"), + "--", + bench_cmd, + ]; + + cmd.args(["sh", "-c", &perf_args.join(" ")]); debug!("cmd: {cmd:?}"); let on_process_started = async |_| -> anyhow::Result<()> { @@ -145,7 +150,7 @@ impl PerfRunner { pub async fn save_files_to(&self, profile_folder: &PathBuf) -> anyhow::Result<()> { let start = std::time::Instant::now(); - // We ran perf with sudo, so we have to change the ownership of the perf data files + // We ran perf with sudo, so we have to change the ownership of the perf.data run_with_sudo(&[ "chown", "-R", @@ -154,63 +159,24 @@ impl PerfRunner { nix::unistd::Uid::current(), nix::unistd::Gid::current() ), - self.perf_dir.path().to_str().unwrap(), + PERF_DATA_PATH, ])?; - // Copy the perf data files to the profile folder - let copy_tasks = std::fs::read_dir(&self.perf_dir)? - .filter_map(|entry| entry.ok()) - .map(|entry| entry.path().to_path_buf()) - .filter(|path| { - path.file_name() - .map(|name| name.to_string_lossy().starts_with(PERF_DATA_PREFIX)) - .unwrap_or(false) - }) - .sorted_by_key(|path| path.file_name().unwrap().to_string_lossy().to_string()) - // The first perf.data will only contain metadata that is not relevant to the benchmarks. We - // capture the symbols and unwind data separately. - .skip(1) - .map(|src_path| { - let profile_folder = profile_folder.clone(); - tokio::task::spawn(async move { - let pid = helpers::find_pid(&src_path)?; - - let dst_file_name = format!( - "{}_{}.perf", - pid, - src_path.file_name().unwrap_or_default().to_string_lossy(), - ); - let dst_path = profile_folder.join(dst_file_name); - tokio::fs::copy(src_path, dst_path).await?; - - Ok::<_, anyhow::Error>(pid) - }) - }) - .collect::>(); + // Copy the perf data to the profile folder + let perf_data_dest = profile_folder.join("perf.data"); + std::fs::copy(PERF_DATA_PATH, &perf_data_dest) + .with_context(|| format!("Failed to copy perf data to {perf_data_dest:?}",))?; let bench_data = self .benchmark_data .get() .expect("Benchmark order is not available"); - assert_eq!( - copy_tasks.len(), - bench_data.bench_count(), - "Benchmark count mismatch" - ); // Harvest the perf maps generated by python. This will copy the perf // maps from /tmp to the profile folder. We have to write our own perf // maps to these files AFTERWARDS, otherwise it'll be overwritten! - let bench_pids = futures::future::try_join_all(copy_tasks) - .await? - .into_iter() - .filter_map(|result| { - debug!("Copy task result: {result:?}"); - result.ok() - }) - .collect::>(); - harvest_perf_maps_for_pids(profile_folder, &bench_pids).await?; - harvest_perf_jit_for_pids(profile_folder, &bench_pids).await?; + harvest_perf_maps_for_pids(profile_folder, &bench_data.bench_pids).await?; + harvest_perf_jit_for_pids(profile_folder, &bench_data.bench_pids).await?; // Append perf maps, unwind info and other metadata if let Err(BenchmarkDataSaveError::MissingIntegration) = bench_data.save_to(profile_folder) @@ -228,9 +194,9 @@ impl PerfRunner { #[cfg(target_os = "linux")] fn process_memory_mappings( - pid: u32, - symbols_by_pid: &mut HashMap, - unwind_data_by_pid: &mut HashMap>, + pid: pid_t, + symbols_by_pid: &mut HashMap, + unwind_data_by_pid: &mut HashMap>, ) -> anyhow::Result<()> { use procfs::process::MMPermissions; @@ -302,13 +268,21 @@ impl PerfRunner { mut runner_fifo: RunnerFifo, mut perf_fifo: PerfFifo, ) -> anyhow::Result { - let mut bench_order_by_pid = HashMap::>::new(); - let mut symbols_by_pid = HashMap::::new(); - let mut unwind_data_by_pid = HashMap::>::new(); + let mut bench_order_by_timestamp = Vec::<(u64, String)>::new(); + let mut bench_pids = HashSet::::new(); + let mut symbols_by_pid = HashMap::::new(); + let mut unwind_data_by_pid = HashMap::>::new(); + let mut markers = Vec::::new(); + let mut integration = None; let mut perf_ping_timeout = 5; - let perf_pid = OnceCell::new(); + let current_time = || { + clock_gettime(nix::time::ClockId::CLOCK_MONOTONIC) + .unwrap() + .num_nanoseconds() as u64 + }; + loop { let perf_ping = tokio::time::timeout(Duration::from_secs(perf_ping_timeout), perf_fifo.ping()) @@ -328,7 +302,8 @@ impl PerfRunner { match cmd { FifoCommand::CurrentBenchmark { pid, uri } => { - bench_order_by_pid.entry(pid).or_default().push(uri); + bench_order_by_timestamp.push((current_time(), uri)); + bench_pids.insert(pid); #[cfg(target_os = "linux")] if !symbols_by_pid.contains_key(&pid) && !unwind_data_by_pid.contains_key(&pid) @@ -343,26 +318,14 @@ impl PerfRunner { runner_fifo.send_cmd(FifoCommand::Ack).await?; } FifoCommand::StartBenchmark => { - let perf_pid = perf_pid.get_or_init(|| { - let output = std::process::Command::new("sh") - .arg("-c") - .arg("pidof -s perf") - .output() - .expect("Failed to run pidof command"); - - String::from_utf8_lossy(&output.stdout) - .trim() - .parse::() - .expect("Failed to parse perf pid") - }); - - // Split the perf.data file - run_with_sudo(&["kill", "-USR2", &perf_pid.to_string()])?; + markers.push(MarkerType::SampleStart(current_time())); perf_fifo.start_events().await?; runner_fifo.send_cmd(FifoCommand::Ack).await?; } FifoCommand::StopBenchmark => { + markers.push(MarkerType::SampleEnd(current_time())); + perf_fifo.stop_events().await?; runner_fifo.send_cmd(FifoCommand::Ack).await?; } @@ -377,16 +340,39 @@ impl PerfRunner { integration = Some((name, version)); runner_fifo.send_cmd(FifoCommand::Ack).await?; } - FifoCommand::Ack => unreachable!(), - FifoCommand::Err => unreachable!(), + FifoCommand::AddMarker { marker, .. } => { + markers.push(marker); + runner_fifo.send_cmd(FifoCommand::Ack).await?; + } + FifoCommand::SetVersion(protocol_version) => { + if protocol_version < runner_shared::fifo::CURRENT_PROTOCOL_VERSION { + panic!( + "Integration is using an incompatible protocol version ({protocol_version} < {}). Please update the integration to the latest version.", + runner_shared::fifo::CURRENT_PROTOCOL_VERSION + ) + } else if protocol_version > runner_shared::fifo::CURRENT_PROTOCOL_VERSION { + panic!( + "Runner is using an incompatible protocol version ({} < {protocol_version}). Please update the runner to the latest version.", + runner_shared::fifo::CURRENT_PROTOCOL_VERSION + ) + }; + + runner_fifo.send_cmd(FifoCommand::Ack).await?; + } + _ => { + warn!("Received unexpected command: {cmd:?}"); + runner_fifo.send_cmd(FifoCommand::Err).await?; + } } } Ok(BenchmarkData { integration, - bench_order_by_pid, + uri_by_ts: bench_order_by_timestamp, + bench_pids, symbols_by_pid, unwind_data_by_pid, + markers, }) } } @@ -395,9 +381,11 @@ pub struct BenchmarkData { /// Name and version of the integration integration: Option<(String, String)>, - bench_order_by_pid: HashMap>, - symbols_by_pid: HashMap, - unwind_data_by_pid: HashMap>, + uri_by_ts: Vec<(u64, String)>, + bench_pids: HashSet, + symbols_by_pid: HashMap, + unwind_data_by_pid: HashMap>, + markers: Vec, } #[derive(Debug)] @@ -421,11 +409,12 @@ impl BenchmarkData { } let metadata = PerfMetadata { + version: PERF_METADATA_CURRENT_VERSION, integration: self .integration .clone() .ok_or(BenchmarkDataSaveError::MissingIntegration)?, - bench_order_by_pid: self.bench_order_by_pid.clone(), + uri_by_ts: self.uri_by_ts.clone(), ignored_modules: { let mut to_ignore = vec![]; @@ -471,13 +460,10 @@ impl BenchmarkData { to_ignore }, + markers: self.markers.clone(), }; metadata.save_to(&path).unwrap(); Ok(()) } - - pub fn bench_count(&self) -> usize { - self.bench_order_by_pid.values().map(|v| v.len()).sum() - } } diff --git a/src/run/runner/wall_time/perf/perf_map.rs b/src/run/runner/wall_time/perf/perf_map.rs index 9dcc0cec..63b32794 100644 --- a/src/run/runner/wall_time/perf/perf_map.rs +++ b/src/run/runner/wall_time/perf/perf_map.rs @@ -1,4 +1,5 @@ use crate::prelude::*; +use libc::pid_t; use object::{Object, ObjectSegment, ObjectSymbol, ObjectSymbolTable}; use std::{ collections::HashMap, @@ -169,13 +170,13 @@ impl ModuleSymbols { /// Represents all the modules inside a process and their symbols. pub struct ProcessSymbols { - pid: u32, + pid: pid_t, module_mappings: HashMap>, modules: HashMap, } impl ProcessSymbols { - pub fn new(pid: u32) -> Self { + pub fn new(pid: pid_t) -> Self { Self { pid, module_mappings: HashMap::new(), @@ -185,7 +186,7 @@ impl ProcessSymbols { pub fn add_mapping>( &mut self, - pid: u32, + pid: pid_t, module_path: P, start_addr: u64, end_addr: u64, diff --git a/src/run/runner/wall_time/perf/unwind_data.rs b/src/run/runner/wall_time/perf/unwind_data.rs index 76a0537b..500aa98b 100644 --- a/src/run/runner/wall_time/perf/unwind_data.rs +++ b/src/run/runner/wall_time/perf/unwind_data.rs @@ -2,7 +2,7 @@ use anyhow::{Context, bail}; use debugid::CodeId; -use object::{Object, ObjectSection}; +use libc::pid_t; use runner_shared::unwind_data::UnwindData; use std::ops::Range; @@ -17,7 +17,7 @@ pub trait UnwindDataExt { where Self: Sized; - fn save_to>(&self, folder: P, pid: u32) -> anyhow::Result<()>; + fn save_to>(&self, folder: P, pid: pid_t) -> anyhow::Result<()>; fn to_file>(&self, path: P) -> anyhow::Result<()>; } @@ -109,7 +109,7 @@ impl UnwindDataExt for UnwindData { }) } - fn save_to>(&self, folder: P, pid: u32) -> anyhow::Result<()> { + fn save_to>(&self, folder: P, pid: pid_t) -> anyhow::Result<()> { let unwind_data_path = folder.as_ref().join(format!( "{}_{:x}_{:x}.unwind", pid, self.avma_range.start, self.avma_range.end