Skip to content

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

Merged
not-matthias merged 1 commit into
mainfrom
cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for
Jul 27, 2026
Merged

feat: enable cycle estimation and allocation exclusion by default#461
not-matthias merged 1 commit into
mainfrom
cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for

Conversation

@not-matthias

Copy link
Copy Markdown
Member

Summary

Graduate --cycle-estimation and --exclude-allocations out of the experimental flag set so both simulation behaviors are enabled for everyone by default.

  • Both are now normal (non-experimental) options with default_value_t = true.
  • They remain toggleable escape hatches: --cycle-estimation=false / --exclude-allocations=false (or env CODSPEED_CYCLE_ESTIMATION, CODSPEED_EXCLUDE_ALLOCATIONS) disable them.
  • Dropped from the Experimental help heading and no longer trigger the experimental-flags warning.
  • --experimental-fair-sched is intentionally left untouched (still experimental, default off).

Changes

File Change
src/cli/shared.rs Add cycle_estimation / exclude_allocations toggleable bools (default true, require_equals) to ExecAndRunSharedArgs
src/cli/experimental.rs Remove both from ExperimentalArgs; only experimental_fair_sched remains
src/cli/run/mod.rs, src/cli/exec/mod.rs Read the new shared fields
src/executor/config.rs OrchestratorConfig::test() defaults both true

Behavior note

Runner.exclude_allocations in upload metadata now serializes true on every simulation run (previously omitted via skip_serializing_if). This is the intended signal, just always present now.

Verification

  • cargo build, cargo fmt --check, cargo clippy --all-targets clean.
  • Verified via the built binary: --cycle-estimation, --cycle-estimation=false, and --exclude-allocations=false all parse; help lists both as normal options.
  • Pre-existing Unsupported system valgrind-executor test failures are environment-gated (unsupported valgrind host) and unrelated to this change.

@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes cycle estimation and allocation exclusion the normal default behavior. The main changes are:

  • Adds default-on shared CLI options for run and exec.
  • Keeps the old experimental options hidden as deprecated no-ops.
  • Wires the new shared options into orchestrator config.
  • Updates test defaults and upload metadata comments for the new default.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
src/cli/shared.rs Adds default-on shared flags and environment variables for cycle estimation and allocation exclusion.
src/cli/experimental.rs Keeps the old graduated experimental fields hidden and warns when active deprecated flags are used.
src/cli/run/mod.rs Uses the new shared fields when building run orchestrator config and updates test defaults.
src/cli/exec/mod.rs Uses the new shared fields when building exec orchestrator config.
src/executor/config.rs Updates test orchestrator defaults to match the new default-on behavior.
src/upload/interfaces.rs Updates the exclude_allocations serialization comment for the new opt-out behavior.

Reviews (6): Last reviewed commit: "feat: enable cycle estimation and alloca..." | Re-trigger Greptile

Comment thread src/cli/shared.rs
Comment thread src/cli/shared.rs
Comment thread src/cli/shared.rs
@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Performance comparison unavailable

A heads-up: this makes performance comparison unavailable when comparing benchmarks before and after this change:

  • Cycle estimation changes how benchmark performance is calculated. It was enabled on only one side of the comparison (base off → head on), so the values are not directly comparable. View runner releases
  • Allocator-time exclusion changes benchmark values by removing allocator time from the measurement. It was enabled on only one side of the comparison (base off → head on), so the values are not directly comparable. View runner releases

Comparing cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for (aebdbbc) with main (266f658)

Open in CodSpeed

@not-matthias
not-matthias marked this pull request as ready for review July 21, 2026 18:27

@GuillaumeLagrange GuillaumeLagrange left a comment

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.

olgtm, make sure to not merge before we minor release

Comment thread src/cli/experimental.rs
Comment thread src/cli/shared.rs
@not-matthias
not-matthias force-pushed the cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for branch from 92f25c7 to 0f82e27 Compare July 27, 2026 09:26
Comment thread src/cli/experimental.rs
@not-matthias
not-matthias force-pushed the cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for branch from 0f82e27 to 13499ef Compare July 27, 2026 09:32
Comment thread src/cli/experimental.rs
@not-matthias
not-matthias force-pushed the cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for branch 2 times, most recently from a50fe11 to d4b65cd Compare July 27, 2026 10:02
Comment thread src/cli/shared.rs
Graduate --cycle-estimation and --exclude-allocations out of the
experimental flag set. They are now normal, default-on options that
can be disabled with --cycle-estimation=false / --exclude-allocations=false
(env: CODSPEED_CYCLE_ESTIMATION, CODSPEED_EXCLUDE_ALLOCATIONS).

--experimental-fair-sched remains experimental.
@not-matthias
not-matthias force-pushed the cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for branch from d4b65cd to aebdbbc Compare July 27, 2026 17:21
@not-matthias

Copy link
Copy Markdown
Member Author

@greptileai review

@not-matthias
not-matthias merged commit aebdbbc into main Jul 27, 2026
23 checks passed
@not-matthias
not-matthias deleted the cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for branch July 27, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants