diff --git a/crates/samply-codspeed b/crates/samply-codspeed index a8ac4c91c..ad8f2861b 160000 --- a/crates/samply-codspeed +++ b/crates/samply-codspeed @@ -1 +1 @@ -Subproject commit a8ac4c91c2278891c1457e907a69a66ed7909314 +Subproject commit ad8f2861b7f71c285937385c8a66cacf064c027b diff --git a/src/cli/exec/mod.rs b/src/cli/exec/mod.rs index 2302f4839..92983391d 100644 --- a/src/cli/exec/mod.rs +++ b/src/cli/exec/mod.rs @@ -80,7 +80,7 @@ fn build_orchestrator_config( targets: vec![target], modes, instruments: Instruments { mongodb: None }, // exec doesn't support MongoDB - perf_unwinding_mode: args.shared.profiler_run_args.perf.perf_unwinding_mode, + walltime_unwinding: args.shared.profiler_run_args.walltime.resolve_unwinding(), enable_profiler: args.shared.profiler_run_args.resolve_enable_profiler(), walltime_profiler: args.shared.walltime_profiler, simulation_tool: args.shared.simulation_tool.unwrap_or_default(), diff --git a/src/cli/run/mod.rs b/src/cli/run/mod.rs index e6750c1eb..6d25e1470 100644 --- a/src/cli/run/mod.rs +++ b/src/cli/run/mod.rs @@ -48,7 +48,7 @@ impl RunArgs { /// Constructs a new `RunArgs` with default values for testing purposes pub fn test() -> Self { use super::experimental::ExperimentalArgs; - use super::{PerfRunArgs, ProfilerRunArgs}; + use super::{ProfilerRunArgs, WalltimeRunArgs}; use crate::RunnerMode; Self { @@ -77,7 +77,8 @@ impl RunArgs { profiler_run_args: ProfilerRunArgs { enable_profiler: false, enable_perf: None, - perf: PerfRunArgs { + walltime: WalltimeRunArgs { + walltime_unwinding_mode: None, perf_unwinding_mode: None, }, }, @@ -127,7 +128,7 @@ fn build_orchestrator_config( targets, modes, instruments, - perf_unwinding_mode: args.shared.profiler_run_args.perf.perf_unwinding_mode, + walltime_unwinding: args.shared.profiler_run_args.walltime.resolve_unwinding(), enable_profiler: args.shared.profiler_run_args.resolve_enable_profiler(), walltime_profiler: args.shared.walltime_profiler, simulation_tool: args.shared.simulation_tool.unwrap_or_default(), diff --git a/src/cli/shared.rs b/src/cli/shared.rs index 6ea9acd17..0267313a8 100644 --- a/src/cli/shared.rs +++ b/src/cli/shared.rs @@ -1,11 +1,10 @@ use super::experimental::ExperimentalArgs; use crate::VERSION; -use crate::executor::config::{SimulationTool, WalltimeProfiler}; +use crate::executor::config::{SimulationTool, WalltimeProfiler, WalltimeUnwinding}; use crate::prelude::*; use crate::run_environment::interfaces::RepositoryProvider; use crate::runner_mode::{RunnerMode, load_shell_session_mode}; use clap::Args; -use clap::ValueEnum; use std::path::PathBuf; pub(crate) fn show_banner() { @@ -205,17 +204,6 @@ impl ExecAndRunSharedArgs { } } -#[derive(Debug, Copy, Clone, PartialEq, ValueEnum, Default)] -pub enum UnwindingMode { - /// Use the frame pointer for unwinding. Requires the binary to be compiled with frame pointers enabled. - #[clap(name = "fp")] - FramePointer, - - /// Use DWARF unwinding. This does not require any special compilation flags and is enabled by default. - #[default] - Dwarf, -} - #[derive(Args, Debug, Clone)] pub struct ProfilerRunArgs { /// Enable a profiler to collect granular performance data. @@ -228,14 +216,48 @@ pub struct ProfilerRunArgs { pub enable_perf: Option, #[command(flatten)] - pub perf: PerfRunArgs, + pub walltime: WalltimeRunArgs, } #[derive(Args, Debug, Clone)] -pub struct PerfRunArgs { - /// The unwinding mode that should be used with perf to collect the call stack. - #[arg(long, env = "CODSPEED_PERF_UNWINDING_MODE")] - pub perf_unwinding_mode: Option, +pub struct WalltimeRunArgs { + /// Call stack unwinding for the walltime profiler: `fp`, `dwarf`, or `dwarf,`. + /// - `fp` requires binaries compiled with frame pointers. + /// - `dwarf,` sets the user stack size copied per sample; larger values produce + /// deeper call stacks but bigger profiles. + /// + /// Inferred from the benchmark command when unset: + /// - Java: `fp` + /// - Go: `fp` + /// - Python: `dwarf,32768` + /// - Others: `dwarf,8192` + #[arg( + long, + env = "CODSPEED_WALLTIME_UNWINDING_MODE", + value_parser = parse_walltime_unwinding, + verbatim_doc_comment + )] + pub walltime_unwinding_mode: Option, + + /// Deprecated alias for --walltime-unwinding-mode / CODSPEED_WALLTIME_UNWINDING_MODE. + #[arg(long, env = "CODSPEED_PERF_UNWINDING_MODE", hide = true, value_parser = parse_walltime_unwinding)] + pub perf_unwinding_mode: Option, +} + +impl WalltimeRunArgs { + /// Resolves the effective unwinding, honoring the deprecated + /// `--perf-unwinding-mode` / `CODSPEED_PERF_UNWINDING_MODE` flag with a warning. + /// The new flag takes precedence when both are set. + pub fn resolve_unwinding(&self) -> Option { + let Some(legacy) = self.perf_unwinding_mode else { + return self.walltime_unwinding_mode; + }; + // TODO(COD-4063): use warn! once the logger is installed before config resolution. + eprintln!( + "Warning: CODSPEED_PERF_UNWINDING_MODE / --perf-unwinding-mode is deprecated; use CODSPEED_WALLTIME_UNWINDING_MODE / --walltime-unwinding-mode instead." + ); + Some(self.walltime_unwinding_mode.unwrap_or(legacy)) + } } impl ProfilerRunArgs { @@ -256,3 +278,27 @@ impl ProfilerRunArgs { fn parse_version(s: &str) -> Result { semver::Version::parse(s).map_err(|e| format!("Invalid semantic version: {e}")) } + +const FRAME_POINTER_UNWINDING: &str = "fp"; +const DWARF_UNWINDING: &str = "dwarf"; + +/// Parses `fp`, `dwarf`, or `dwarf,`. +fn parse_walltime_unwinding(value: &str) -> Result { + let (mode, stack_size) = match value.split_once(',') { + Some((mode, size)) => { + let size = size + .parse() + .map_err(|e| format!("Invalid stack size `{size}`: {e}"))?; + (mode, Some(size)) + } + None => (value, None), + }; + match (mode, stack_size) { + (DWARF_UNWINDING, stack_size) => Ok(WalltimeUnwinding::Dwarf { stack_size }), + (FRAME_POINTER_UNWINDING, None) => Ok(WalltimeUnwinding::FramePointer), + (FRAME_POINTER_UNWINDING, Some(_)) => { + Err("A stack size only applies to `dwarf` unwinding".into()) + } + _ => Err("Expected `fp`, `dwarf`, or `dwarf,`".into()), + } +} diff --git a/src/executor/config.rs b/src/executor/config.rs index f3ec640c8..98ad7810f 100644 --- a/src/executor/config.rs +++ b/src/executor/config.rs @@ -1,4 +1,3 @@ -use crate::cli::UnwindingMode; use crate::instruments::Instruments; use crate::prelude::*; use crate::run_environment::RepositoryProvider; @@ -51,6 +50,29 @@ pub enum WalltimeProfiler { Samply, } +/// Explicit walltime unwinding settings. An absent setting preserves backend inference. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WalltimeUnwinding { + FramePointer, + /// An absent size preserves the profiler's default stack copy size. + Dwarf { + stack_size: Option, + }, +} + +/// Formats as the `--walltime-unwinding-mode` value. +impl std::fmt::Display for WalltimeUnwinding { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::FramePointer => f.write_str("fp"), + Self::Dwarf { stack_size: None } => f.write_str("dwarf"), + Self::Dwarf { + stack_size: Some(size), + } => write!(f, "dwarf,{size}"), + } + } +} + /// Run-level configuration owned by the orchestrator. /// /// Holds all parameters that are constant across benchmark targets within a run, @@ -68,8 +90,8 @@ pub struct OrchestratorConfig { pub modes: Vec, pub instruments: Instruments, pub enable_profiler: bool, - /// Stack unwinding mode for perf (if enabled) - pub perf_unwinding_mode: Option, + /// Explicit unwinding settings; None preserves the profiler's automatic selection. + pub walltime_unwinding: Option, /// Profiler override for walltime mode (if None, selected based on the platform) pub walltime_profiler: Option, @@ -120,8 +142,8 @@ pub struct ExecutorConfig { pub instruments: Instruments, pub enable_profiler: bool, - /// Stack unwinding mode for perf (if enabled) - pub perf_unwinding_mode: Option, + /// Explicit unwinding settings; None preserves the profiler's automatic selection. + pub walltime_unwinding: Option, pub simulation_tool: SimulationTool, @@ -218,7 +240,7 @@ impl OrchestratorConfig { command, instruments: self.instruments.clone(), enable_profiler: self.enable_profiler, - perf_unwinding_mode: self.perf_unwinding_mode, + walltime_unwinding: self.walltime_unwinding, simulation_tool: self.simulation_tool, skip_run: self.skip_run, skip_setup: self.skip_setup, @@ -251,7 +273,7 @@ impl OrchestratorConfig { }], modes: vec![RunnerMode::Simulation], instruments: Instruments::test(), - perf_unwinding_mode: None, + walltime_unwinding: None, walltime_profiler: None, enable_profiler: false, simulation_tool: SimulationTool::default(), diff --git a/src/executor/helpers/introspected_golang/go.sh b/src/executor/helpers/introspected_golang/go.sh index f3e08dd39..a09866f5f 100755 --- a/src/executor/helpers/introspected_golang/go.sh +++ b/src/executor/helpers/introspected_golang/go.sh @@ -111,9 +111,9 @@ if [ $# -eq 0 ]; then exit $? fi -# On arm64, warn about setting CODSPEED_PERF_UNWINDING_MODE to "fp" for correct flamegraphs +# On arm64, warn about setting CODSPEED_WALLTIME_UNWINDING_MODE to "fp" for correct flamegraphs if [ "$(uname -m)" = "aarch64" ] && [ "${CODSPEED_GO_SUPPRESS_PERF_UNWINDING_MODE_WARNING:-}" != "true" ]; then - echo "::warning::Go profiling on arm64 require frame pointer unwinding. Set CODSPEED_PERF_UNWINDING_MODE=fp for better profiling." >&2 + echo "::warning::Go profiling on arm64 require frame pointer unwinding. Set CODSPEED_WALLTIME_UNWINDING_MODE=fp for better profiling." >&2 fi # Route command based on first argument diff --git a/src/executor/wall_time/profiler/perf/mod.rs b/src/executor/wall_time/profiler/perf/mod.rs index dbc0e464a..9283836e4 100644 --- a/src/executor/wall_time/profiler/perf/mod.rs +++ b/src/executor/wall_time/profiler/perf/mod.rs @@ -1,8 +1,8 @@ #![cfg_attr(not(unix), allow(dead_code, unused_mut))] -use crate::cli::UnwindingMode; use crate::executor::ExecutorConfig; use crate::executor::ToolStatus; +use crate::executor::config::WalltimeUnwinding; use crate::executor::helpers::command::CommandBuilder; use crate::executor::helpers::detect_executable::command_has_executable; use crate::executor::helpers::env::is_codspeed_debug_enabled; @@ -39,6 +39,40 @@ pub mod perf_executable; const PERF_PIPEDATA_FILE_NAME: &str = "perf.pipedata"; +/// Unwinding suggested for the benchmarked runtime, if it has a known best setting. +fn suggested_unwinding(command: &str) -> Option { + if command_has_executable( + command, + &["gradle", "gradlew", "java", "maven", "mvn", "mvnw"], + ) { + return Some(WalltimeUnwinding::FramePointer); + } + if command_has_executable(command, &["go"]) { + return Some(WalltimeUnwinding::FramePointer); + } + if command_has_executable(command, &["pytest", "uv", "python", "python3"]) { + return Some(WalltimeUnwinding::Dwarf { + stack_size: Some(32 * 1024), + }); + } + None +} + +fn resolve_unwinding(command: &str, explicit: Option) -> WalltimeUnwinding { + let suggested = suggested_unwinding(command); + let Some(explicit) = explicit else { + return suggested.unwrap_or(WalltimeUnwinding::Dwarf { stack_size: None }); + }; + if let Some(suggested) = suggested + && suggested != explicit + { + warn!( + "Overriding suggested unwinding `{suggested}` with `{explicit}`, call stacks may be incomplete" + ); + } + explicit +} + pub struct PerfProfiler { /// Set by [`Profiler::wrap_command`]; used by the FIFO hooks to control event /// recording on the live `perf record` process. @@ -89,35 +123,16 @@ impl Profiler for PerfProfiler { let perf_fifo = PerfFifo::new()?; let perf_file_path = profile_folder.join(PERF_PIPEDATA_FILE_NAME); - // Infer the unwinding mode from the benchmark cmd - let (cg_mode, stack_size) = if let Some(mode) = config.perf_unwinding_mode { - (mode, None) - } else if command_has_executable( - &config.command, - &["gradle", "gradlew", "java", "maven", "mvn", "mvnw"], - ) { - // In Java, we must use FP unwinding otherwise we'll have broken call stacks. - (UnwindingMode::FramePointer, None) - } else if command_has_executable(&config.command, &["cargo"]) { - (UnwindingMode::Dwarf, None) - } else if command_has_executable(&config.command, &["go"]) { - (UnwindingMode::FramePointer, None) - } else if command_has_executable(&config.command, &["pytest", "uv", "python", "python3"]) { - // Note that the higher the stack size, the larger the file, although it is mitigated - // by zstd compression - (UnwindingMode::Dwarf, Some(32 * 1024)) - } else { - // Default to dwarf unwinding since it works well with most binaries. - debug!("No call graph mode detected, defaulting to dwarf"); - (UnwindingMode::Dwarf, None) - }; + let unwinding = resolve_unwinding(&config.command, config.walltime_unwinding); - let cg_mode = match cg_mode { - UnwindingMode::FramePointer => { + let cg_mode = match unwinding { + WalltimeUnwinding::FramePointer => { suppress_go_perf_unwinding_warning(); "fp" } - UnwindingMode::Dwarf => &format!("dwarf,{}", stack_size.unwrap_or(8192)), + WalltimeUnwinding::Dwarf { stack_size } => { + &format!("dwarf,{}", stack_size.unwrap_or(8 * 1024)) + } }; debug!("Using call graph mode: {cg_mode:?}"); @@ -310,3 +325,32 @@ impl BenchmarkData<'_> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn explicit_unwinding_overrides_language_inference() { + let explicit = WalltimeUnwinding::Dwarf { + stack_size: Some(16 * 1024), + }; + for (command, inferred) in [ + ( + "python bench.py", + WalltimeUnwinding::Dwarf { + stack_size: Some(32 * 1024), + }, + ), + ("cargo bench", WalltimeUnwinding::Dwarf { stack_size: None }), + ("go test -bench=.", WalltimeUnwinding::FramePointer), + ] { + assert_eq!(resolve_unwinding(command, None), inferred); + assert_eq!(resolve_unwinding(command, Some(explicit)), explicit); + } + assert_eq!( + resolve_unwinding("python bench.py", Some(WalltimeUnwinding::FramePointer)), + WalltimeUnwinding::FramePointer, + ); + } +} diff --git a/src/executor/wall_time/profiler/samply/mod.rs b/src/executor/wall_time/profiler/samply/mod.rs index 394449d17..9dd5c1876 100644 --- a/src/executor/wall_time/profiler/samply/mod.rs +++ b/src/executor/wall_time/profiler/samply/mod.rs @@ -3,6 +3,8 @@ use crate::cli::InternalCommands; use crate::cli::samply::SamplyArgs; use crate::executor::ExecutorConfig; +#[cfg(target_os = "linux")] +use crate::executor::config::WalltimeUnwinding; use crate::executor::helpers::command::CommandBuilder; use crate::executor::helpers::run_with_sudo::wrap_with_sudo; use crate::executor::shared::fifo::FifoBenchmarkData; @@ -81,7 +83,7 @@ impl Profiler for SamplyProfiler { async fn wrap_command( &mut self, mut cmd_builder: CommandBuilder, - _config: &ExecutorConfig, + config: &ExecutorConfig, profile_folder: &Path, requires_sudo: bool, ) -> anyhow::Result { @@ -89,20 +91,38 @@ impl Profiler for SamplyProfiler { // samply is bundled into this binary as the `samply` subcommand; // re-exec ourselves so we don't depend on a system install. - let samply_builder = InternalCommands::Samply(SamplyArgs { - args: vec![ - "record".into(), - "--presymbolicate".into(), - "--no-open".into(), - "--save-only".into(), - "--rate".into(), - SAMPLING_RATE_HZ.to_string().into(), - "-o".into(), - output_path.clone().into(), - "--".into(), - ], - }) - .get_command_builder()?; + let mut args: Vec = vec![ + "record".into(), + "--presymbolicate".into(), + "--no-open".into(), + "--save-only".into(), + "--rate".into(), + SAMPLING_RATE_HZ.to_string().into(), + "-o".into(), + output_path.clone().into(), + ]; + // Unwinding knobs are Linux-only in samply. + #[cfg(target_os = "linux")] + { + match config.walltime_unwinding { + Some(WalltimeUnwinding::FramePointer) => { + args.extend(["--unwinding-mode".into(), "fp".into()]); + } + Some(WalltimeUnwinding::Dwarf { stack_size }) => { + args.extend(["--unwinding-mode".into(), "dwarf".into()]); + if let Some(size) = stack_size { + args.extend(["--unwinding-stack-size".into(), size.to_string().into()]); + } + } + None => {} + } + } + #[cfg(not(target_os = "linux"))] + if config.walltime_unwinding.is_some() { + bail!("Samply unwinding options are only supported on Linux"); + } + args.push("--".into()); + let samply_builder = InternalCommands::Samply(SamplyArgs { args }).get_command_builder()?; cmd_builder.wrap_with(samply_builder); diff --git a/tests/executors.rs b/tests/executors.rs index b45f729b2..c8b8be07d 100644 --- a/tests/executors.rs +++ b/tests/executors.rs @@ -305,3 +305,34 @@ fn memory_forwards_path() { fn memory_forwards_ld_library_path() { memory_forwards_path_like("LD_LIBRARY_PATH", "/custom/test/lib"); } + +#[test] +fn bundled_samply_launches_command_with_explicit_unwinding() { + let _guard = UPROBES.read().unwrap_or_else(PoisonError::into_inner); + let dir = tempfile::tempdir().unwrap(); + let profile = dir.path().join("profile.json"); + codspeed() + .args([ + "samply", + "record", + "--save-only", + "--no-open", + "--unwinding-mode", + "dwarf", + "--unwinding-stack-size", + "16384", + "-o", + ]) + .arg(profile) + .args([ + "--", + "sh", + "-c", + "printf '%s\\n' \"$1\"", + "profiled-command", + "--unwinding-stack-size=application-value", + ]) + .assert() + .success() + .stdout("--unwinding-stack-size=application-value\n"); +}