fix: disable allocator exclusion by default - #489
Conversation
Greptile SummaryThis PR disables allocator exclusion by default.
Confidence Score: 4/5The PR appears safe to merge, but its deprecated-option warning should be updated to describe the new allocator-exclusion default accurately. The changed default flows consistently through run and exec, with the only accepted issue being stale user-facing guidance that still claims allocator exclusion defaults to true. Files Needing Attention: src/cli/shared.rs and src/cli/experimental.rs
|
| Filename | Overview |
|---|---|
| src/cli/shared.rs | Changes the allocator-exclusion default for both run and exec; the related deprecated-option warning still documents the previous default. |
Prompt To Fix All With AI
### Issue 1
src/cli/shared.rs:129
**Stale allocator default guidance**
The deprecated `--experimental-exclude-allocations` warning still says allocator exclusion defaults to true, while this change makes the effective default false, giving users incorrect guidance when configuring benchmark measurements.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: disable allocator exclusion by defa..." | Re-trigger Greptile
Performance comparison unavailableThe base and head of this comparison were measured with different runner settings, so their benchmark values are not directly comparable. What changed between base and head:
Re-run the base with the same settings to get a valid performance comparison. Comparing |
Allocation exclusion has defaulted to false since #489, but the deprecation warning still told users it defaults to true, and the serde-skip rationale still called --exclude-allocations=false the "opt-out path".
…9994) ## Summary Mimalloc's deferred page collection and arena purging can land inside a single-shot benchmark and get reported as a Vortex regression. In [this CI comparison](https://app.codspeed.io/vortex-data/vortex/runs/compare/6ab29cf078100e55aedd4885..6ab2a14f78100e55aedd4921), `_mi_theap_collect_retired` and its callees account for an additional 22.30 µs in the dictionary case and 38.64 µs in the FSL case, closely matching their reported gaps. ## Changes Enable `exclude-allocations` for the array dictionary and selection simulation shards, which contain `dict_compress`, `take_fsl`, and `take_chunked`. This keeps mimalloc in use while excluding allocator time from these scores. Allocation-focused benchmarks in the core shard continue to measure allocator cost. <details> <summary>Previous attempts</summary> - #8742 and #8861 addressed earlier two-level CodSpeed results by adding mimalloc. #8807 tracked the cases that remained flaky. - #9951 resized noisy cases, and #9953 extended mimalloc to the remaining benchmark binaries. Deferred cleanup still varies within mimalloc. - #9174 included a commit titled “Try and exclude codspeed allocations,” but its diff only upgraded the action. Its report used runner 5.0.1, where [allocation exclusion was already disabled by default](CodSpeedHQ/codspeed#489). No explicit opt-in was added. This PR sets it for the affected shards. </details> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
No description provided.