feat: enable cycle estimation and allocation exclusion by default#461
feat: enable cycle estimation and allocation exclusion by default#461not-matthias wants to merge 1 commit into
Conversation
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.
Greptile SummaryThis PR makes cycle estimation and allocation exclusion default-on runner options. The main changes are:
Confidence Score: 4/5The changed CLI and upload metadata paths need fixes before merging.
src/cli/shared.rs, src/cli/experimental.rs, src/upload/interfaces.rs Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/cli/shared.rs:117-126
**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.
### Issue 2 of 3
src/cli/shared.rs:129-138
**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.
### Issue 3 of 3
src/cli/shared.rs:132
**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.
Reviews (1): Last reviewed commit: "feat: enable cycle estimation and alloca..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[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, |
There was a problem hiding this comment.
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.
| #[arg( | ||
| long, | ||
| env = "CODSPEED_EXCLUDE_ALLOCATIONS", | ||
| default_value_t = true, | ||
| default_missing_value = "true", | ||
| num_args = 0..=1, | ||
| require_equals = true, | ||
| action = clap::ArgAction::Set | ||
| )] | ||
| pub exclude_allocations: bool, |
There was a problem hiding this comment.
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.| #[arg( | ||
| long, | ||
| env = "CODSPEED_EXCLUDE_ALLOCATIONS", | ||
| default_value_t = true, |
There was a problem hiding this comment.
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.
Merging this PR will degrade performance by 43.06%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | sleep 1 |
126.3 µs | 221.8 µs | -43.06% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing cod-2950-enable-cycle-estimationexclude-allocators-in-runner-for (92f25c7) with main (6794d28)
1 participant
Summary
Graduate
--cycle-estimationand--exclude-allocationsout of the experimental flag set so both simulation behaviors are enabled for everyone by default.default_value_t = true.--cycle-estimation=false/--exclude-allocations=false(or envCODSPEED_CYCLE_ESTIMATION,CODSPEED_EXCLUDE_ALLOCATIONS) disable them.Experimentalhelp heading and no longer trigger the experimental-flags warning.--experimental-fair-schedis intentionally left untouched (still experimental, default off).Changes
src/cli/shared.rscycle_estimation/exclude_allocationstoggleable bools (default true,require_equals) toExecAndRunSharedArgssrc/cli/experimental.rsExperimentalArgs; onlyexperimental_fair_schedremainssrc/cli/run/mod.rs,src/cli/exec/mod.rssrc/executor/config.rsOrchestratorConfig::test()defaults both trueBehavior note
Runner.exclude_allocationsin upload metadata now serializestrueon every simulation run (previously omitted viaskip_serializing_if). This is the intended signal, just always present now.Verification
cargo build,cargo fmt --check,cargo clippy --all-targetsclean.--cycle-estimation,--cycle-estimation=false, and--exclude-allocations=falseall parse; help lists both as normal options.Unsupported systemvalgrind-executor test failures are environment-gated (unsupported valgrind host) and unrelated to this change.