Skip to content

feat: enable cycle estimation and allocation exclusion by default #461

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 1 commit into
base: main
Choose a base branch
from
+32 −32
Open

feat: enable cycle estimation and allocation exclusion by default #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/cli/exec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ fn build_orchestrator_config(
poll_results_options,
extra_env: HashMap::new(),
fair_sched: args.shared.experimental.experimental_fair_sched,
cycle_estimation: args.shared.experimental.experimental_cycle_estimation,
exclude_allocations: args.shared.experimental.experimental_exclude_allocations,
cycle_estimation: args.shared.cycle_estimation,
exclude_allocations: args.shared.exclude_allocations,
})
}

Expand Down
24 changes: 0 additions & 24 deletions src/cli/experimental.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,6 @@ pub struct ExperimentalArgs {
env = "CODSPEED_EXPERIMENTAL_FAIR_SCHED"
)]
pub experimental_fair_sched: bool,

/// Enable Valgrind cycle estimation (--cycle-estimation) in simulation mode.
#[arg(
long,
default_value_t = false,
help_heading = "Experimental",
env = "CODSPEED_EXPERIMENTAL_CYCLE_ESTIMATION"
)]
pub experimental_cycle_estimation: bool,

/// Exclude memory allocation time from simulation results.
#[arg(
long,
default_value_t = false,
help_heading = "Experimental",
env = "CODSPEED_EXPERIMENTAL_EXCLUDE_ALLOCATIONS"
)]
pub experimental_exclude_allocations: bool,
}

impl ExperimentalArgs {
Expand All @@ -43,12 +25,6 @@ impl ExperimentalArgs {
if self.experimental_fair_sched {
flags.push("--experimental-fair-sched");
}
if self.experimental_cycle_estimation {
flags.push("--experimental-cycle-estimation");
}
if self.experimental_exclude_allocations {
flags.push("--experimental-exclude-allocations");
}
flags
}

Expand Down
8 changes: 4 additions & 4 deletions src/cli/run/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ impl RunArgs {
go_runner_version: None,
show_full_output: false,
base: None,
cycle_estimation: true,
exclude_allocations: true,
profiler_run_args: ProfilerRunArgs {
enable_profiler: false,
enable_perf: None,
Expand All @@ -78,8 +80,6 @@ impl RunArgs {
},
experimental: ExperimentalArgs {
experimental_fair_sched: false,
experimental_cycle_estimation: false,
experimental_exclude_allocations: false,
},
},
instruments: vec![],
Expand Down Expand Up @@ -129,8 +129,8 @@ fn build_orchestrator_config(
poll_results_options,
extra_env: HashMap::new(),
fair_sched: args.shared.experimental.experimental_fair_sched,
cycle_estimation: args.shared.experimental.experimental_cycle_estimation,
exclude_allocations: args.shared.experimental.experimental_exclude_allocations,
cycle_estimation: args.shared.cycle_estimation,
exclude_allocations: args.shared.exclude_allocations,
})
}

Expand Down
24 changes: 24 additions & 0 deletions src/cli/shared.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,30 @@ pub struct ExecAndRunSharedArgs {
#[arg(long)]
pub base: Option<String>,

/// Enable Valgrind cycle estimation (--cycle-estimation) in simulation mode.
#[arg(
long,
env = "CODSPEED_CYCLE_ESTIMATION",
default_value_t = true,
default_missing_value = "true",
num_args = 0..=1,
require_equals = true,
action = clap::ArgAction::Set
)]
pub cycle_estimation: bool,
Comment on lines +117 to +126

@greptile-apps greptile-apps Bot Jul 20, 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.

P1 Old Cycle Flag Breaks

Existing jobs that still pass --experimental-cycle-estimation now fail argument parsing because the old field was removed without an alias or hidden compatibility option. Existing jobs that set CODSPEED_EXPERIMENTAL_CYCLE_ESTIMATION=false are worse: the env var is ignored and the new default enables cycle estimation anyway.

Knowledge Base Used: CLI Commands

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

Comment:
**Old Cycle Flag Breaks**

Existing jobs that still pass `--experimental-cycle-estimation` now fail argument parsing because the old field was removed without an alias or hidden compatibility option. Existing jobs that set `CODSPEED_EXPERIMENTAL_CYCLE_ESTIMATION=false` are worse: the env var is ignored and the new default enables cycle estimation anyway.

**Knowledge Base Used:** [CLI Commands](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/cli-commands.md)

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex


/// Exclude memory allocation time from simulation results.
#[arg(
long,
env = "CODSPEED_EXCLUDE_ALLOCATIONS",
default_value_t = true,

@greptile-apps greptile-apps Bot Jul 20, 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.

P1 Upload Hash Changes Globally

Defaulting exclude_allocations to true makes Runner.excludeAllocations serialize for every run, so the upload metadata hash changes for the same benchmark invocation even when users did not opt in. The upload metadata contract treats this field as an opt-in serialized signal, so this needs a metadata-version migration or another compatibility path to avoid mixing old and new hash semantics.

Knowledge Base Used: Upload flow

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

Comment:
**Upload Hash Changes Globally**

Defaulting `exclude_allocations` to `true` makes `Runner.excludeAllocations` serialize for every run, so the upload metadata hash changes for the same benchmark invocation even when users did not opt in. The upload metadata contract treats this field as an opt-in serialized signal, so this needs a metadata-version migration or another compatibility path to avoid mixing old and new hash semantics.

**Knowledge Base Used:** [Upload flow](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/upload.md)

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

default_missing_value = "true",
num_args = 0..=1,
require_equals = true,
action = clap::ArgAction::Set
)]
pub exclude_allocations: bool,
Comment on lines +129 to +138

@greptile-apps greptile-apps Bot Jul 20, 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.

P1 Old Allocation Override Ignored

Existing jobs that still pass --experimental-exclude-allocations now fail argument parsing because the old field was removed without an alias or hidden compatibility option. Existing jobs that set CODSPEED_EXPERIMENTAL_EXCLUDE_ALLOCATIONS=false are silently changed to the new default, so uploads send excludeAllocations: true even though the job asked to keep allocation time included.

Knowledge Base Used:

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

Comment:
**Old Allocation Override Ignored**

Existing jobs that still pass `--experimental-exclude-allocations` now fail argument parsing because the old field was removed without an alias or hidden compatibility option. Existing jobs that set `CODSPEED_EXPERIMENTAL_EXCLUDE_ALLOCATIONS=false` are silently changed to the new default, so uploads send `excludeAllocations: true` even though the job asked to keep allocation time included.

**Knowledge Base Used:**
- [CLI Commands](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/cli-commands.md)
- [Upload flow](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/upload.md)

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex


#[command(flatten)]
pub profiler_run_args: ProfilerRunArgs,

Expand Down
4 changes: 2 additions & 2 deletions src/executor/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ impl OrchestratorConfig {
poll_results_options: PollResultsOptions::new(false, None),
extra_env: HashMap::new(),
fair_sched: false,
cycle_estimation: false,
exclude_allocations: false,
cycle_estimation: true,
exclude_allocations: true,
}
}
}
Expand Down