Skip to content

feat(cli): add walltime unwinding mode and stack sample size options #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
not-matthias wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(cli): add walltime unwinding mode and stack sample size options #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e2212c
feat(cli): add walltime unwinding mode and stack sample size options
not-matthias Oct 13, 2026
3b356c9
fix(samply): pass stack sample size as --unwinding-stack-size
not-matthias Oct 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/cli/exec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,16 @@ 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_mode: args
.shared
.profiler_run_args
.walltime
.resolve_unwinding_mode(),
walltime_stack_sample_size: args
.shared
.profiler_run_args
.walltime
.walltime_stack_sample_size,
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(),
Expand Down
17 changes: 14 additions & 3 deletions src/cli/run/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -77,8 +77,10 @@ impl RunArgs {
profiler_run_args: ProfilerRunArgs {
enable_profiler: false,
enable_perf: None,
perf: PerfRunArgs {
walltime: WalltimeRunArgs {
walltime_unwinding_mode: None,
perf_unwinding_mode: None,
walltime_stack_sample_size: None,
},
},
experimental: ExperimentalArgs {
Expand Down Expand Up @@ -127,7 +129,16 @@ fn build_orchestrator_config(
targets,
modes,
instruments,
perf_unwinding_mode: args.shared.profiler_run_args.perf.perf_unwinding_mode,
walltime_unwinding_mode: args
.shared
.profiler_run_args
.walltime
.resolve_unwinding_mode(),
walltime_stack_sample_size: args
.shared
.profiler_run_args
.walltime
.walltime_stack_sample_size,
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(),
Expand Down
32 changes: 28 additions & 4 deletions src/cli/shared.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,14 +228,38 @@ pub struct ProfilerRunArgs {
pub enable_perf: Option<bool>,

#[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 struct WalltimeRunArgs {
/// The unwinding mode that the walltime profiler should use to collect the call stack.
#[arg(long, env = "CODSPEED_WALLTIME_UNWINDING_MODE")]
pub walltime_unwinding_mode: Option<UnwindingMode>,

/// Deprecated alias for --walltime-unwinding-mode / CODSPEED_WALLTIME_UNWINDING_MODE.
#[arg(long, env = "CODSPEED_PERF_UNWINDING_MODE", hide = true)]
pub perf_unwinding_mode: Option<UnwindingMode>,

/// Size in bytes of the user stack copied with each sample for DWARF unwinding.
/// Larger values produce deeper call stacks but bigger profiles.
Comment on lines +244 to +245

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could provide the default value per language here if we expect users/agents to use this now

#[arg(long, env = "CODSPEED_WALLTIME_STACK_SAMPLE_SIZE")]
pub walltime_stack_sample_size: Option<u32>,
}

impl WalltimeRunArgs {
/// Resolves the effective unwinding mode, 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_mode(&self) -> Option<UnwindingMode> {
let Some(legacy) = self.perf_unwinding_mode else {
return self.walltime_unwinding_mode;
};
log::warn!(
"CODSPEED_PERF_UNWINDING_MODE / --perf-unwinding-mode is deprecated; use CODSPEED_WALLTIME_UNWINDING_MODE / --walltime-unwinding-mode instead."
);
Comment on lines +258 to +260

@greptile-apps greptile-apps Bot Oct 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Deprecation warning never appears

Both codspeed run and codspeed exec call resolve_unwinding_mode() while building the config, before Orchestrator::new() installs the logger. This drops the warning, so users of --perf-unwinding-mode or CODSPEED_PERF_UNWINDING_MODE never learn about the replacement. Emit it after logger setup or use the existing startup-warning approach.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/cli/shared.rs
Line: 258-260

Comment:
**Deprecation warning never appears**

Both `codspeed run` and `codspeed exec` call `resolve_unwinding_mode()` while building the config, before `Orchestrator::new()` installs the logger. This drops the warning, so users of `--perf-unwinding-mode` or `CODSPEED_PERF_UNWINDING_MODE` never learn about the replacement. Emit it after logger setup or use the existing startup-warning approach.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@not-matthias, this is a legit thing, and it's extremely annoying.
We may want to have a first logger before the full environment parsing at some point, or queue the logs until we've initialized the logger.

We could create a dedicated issue and just eprintln!() here

Some(self.walltime_unwinding_mode.unwrap_or(legacy))
Comment on lines +254 to +261

@greptile-apps greptile-apps Bot Oct 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Option rules lack tests

The new precedence and stack-size overrides have no tests. Add focused cases for the old option alone, both options together, flag/environment combinations, and an explicit size overriding both the default and Python's inferred size. These are the main promises of this change; tests would catch later edits that silently send the wrong value to the profiler.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/cli/shared.rs
Line: 254-261

Comment:
**Option rules lack tests**

The new precedence and stack-size overrides have no tests. Add focused cases for the old option alone, both options together, flag/environment combinations, and an explicit size overriding both the default and Python's inferred size. These are the main promises of this change; tests would catch later edits that silently send the wrong value to the profiler.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we really care for this one

@greptile-apps greptile-apps Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — these tests are useful for regression protection but not essential to this change, so I’m withdrawing the suggestion. No action needed.

}
}

impl ProfilerRunArgs {
Expand Down
18 changes: 12 additions & 6 deletions src/executor/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,10 @@ pub struct OrchestratorConfig {
pub modes: Vec<RunnerMode>,
pub instruments: Instruments,
pub enable_profiler: bool,
/// Stack unwinding mode for perf (if enabled)
pub perf_unwinding_mode: Option<UnwindingMode>,
/// Stack unwinding mode for the walltime profiler (if None, inferred from the command)
pub walltime_unwinding_mode: Option<UnwindingMode>,
/// User stack size in bytes copied per sample for DWARF unwinding (if None, profiler default)
pub walltime_stack_sample_size: Option<u32>,
/// Profiler override for walltime mode (if None, selected based on the platform)
pub walltime_profiler: Option<WalltimeProfiler>,

Expand Down Expand Up @@ -120,8 +122,10 @@ pub struct ExecutorConfig {

pub instruments: Instruments,
pub enable_profiler: bool,
/// Stack unwinding mode for perf (if enabled)
pub perf_unwinding_mode: Option<UnwindingMode>,
/// Stack unwinding mode for the walltime profiler (if None, inferred from the command)
pub walltime_unwinding_mode: Option<UnwindingMode>,
/// User stack size in bytes copied per sample for DWARF unwinding (if None, profiler default)
pub walltime_stack_sample_size: Option<u32>,
Comment on lines +125 to +128

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the separate flags for the cli, but here we could have them grouped? Cause technically the sample size makes no sense for fp etc, so even for clap it could make sense to parse it as an enum with two variants

FramePointer
and
Dwarf(Option<u32>) with the Option being here to see if we have an explicit setting or if we should default


pub simulation_tool: SimulationTool,

Expand Down Expand Up @@ -218,7 +222,8 @@ impl OrchestratorConfig {
command,
instruments: self.instruments.clone(),
enable_profiler: self.enable_profiler,
perf_unwinding_mode: self.perf_unwinding_mode,
walltime_unwinding_mode: self.walltime_unwinding_mode,
walltime_stack_sample_size: self.walltime_stack_sample_size,
simulation_tool: self.simulation_tool,
skip_run: self.skip_run,
skip_setup: self.skip_setup,
Expand Down Expand Up @@ -251,7 +256,8 @@ impl OrchestratorConfig {
}],
modes: vec![RunnerMode::Simulation],
instruments: Instruments::test(),
perf_unwinding_mode: None,
walltime_unwinding_mode: None,
walltime_stack_sample_size: None,
walltime_profiler: None,
enable_profiler: false,
simulation_tool: SimulationTool::default(),
Expand Down
4 changes: 2 additions & 2 deletions src/executor/helpers/introspected_golang/go.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions src/executor/wall_time/profiler/perf/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ impl Profiler for PerfProfiler {
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 {
let (cg_mode, stack_size) = if let Some(mode) = config.walltime_unwinding_mode {

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the python/cargo/go tests:
We should not override specific stack size from cli IMO.
AND if we override it, we should DEFINITELY print a warning, but I think we should just outright take explicit cli over anything else

(mode, None)
} else if command_has_executable(
&config.command,
Expand All @@ -117,7 +117,13 @@ impl Profiler for PerfProfiler {
suppress_go_perf_unwinding_warning();
"fp"
}
UnwindingMode::Dwarf => &format!("dwarf,{}", stack_size.unwrap_or(8192)),
UnwindingMode::Dwarf => &format!(
"dwarf,{}",
config
.walltime_stack_sample_size
.or(stack_size)
.unwrap_or(8192)
),
};
debug!("Using call graph mode: {cg_mode:?}");

Expand Down
46 changes: 31 additions & 15 deletions src/executor/wall_time/profiler/samply/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Samply profiler integration.

use crate::cli::InternalCommands;
#[cfg(target_os = "linux")]
use crate::cli::UnwindingMode;
use crate::cli::samply::SamplyArgs;
use crate::executor::ExecutorConfig;
use crate::executor::helpers::command::CommandBuilder;
Expand Down Expand Up @@ -81,28 +83,42 @@ 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<CommandBuilder> {
let output_path = profile_folder.join(SAMPLY_OUTPUT_FILE_NAME);

// 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<std::ffi::OsString> = 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")]
{
if let Some(mode) = config.walltime_unwinding_mode {
let mode = match mode {
UnwindingMode::FramePointer => "fp",
UnwindingMode::Dwarf => "dwarf",
};
args.extend(["--unwinding-mode".into(), mode.into()]);
}
if let Some(size) = config.walltime_stack_sample_size {
args.extend(["--unwinding-stack-size".into(), size.to_string().into()]);
}
Comment on lines +107 to +116

@GuillaumeLagrange GuillaumeLagrange Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to check the implementation, but I'm 95% sure that in our samply fork we do both, so we should make sure we're not introducing unexpected behavior here

}
#[cfg(not(target_os = "linux"))]
let _ = config;
args.push("--".into());
let samply_builder = InternalCommands::Samply(SamplyArgs { args }).get_command_builder()?;

cmd_builder.wrap_with(samply_builder);

Expand Down
Loading