feat: add experimental --exclude-allocations setting#425
Conversation
Merging this PR will not alter performance
|
Greptile SummaryThis PR adds an experimental setting to exclude allocation time from measurement results. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "feat: add experimental --exclude-allocat..." | Re-trigger Greptile |
16220f0 to
9f62198
Compare
Add an experimental `--exclude-allocations` flag (env `CODSPEED_EXCLUDE_ALLOCATIONS`) that signals the backend to exclude memory allocation time from simulation results. The value is surfaced as `runner.excludeAllocations` in the upload metadata so the backend can gate comparability and history on it: runs with different values are not comparable. The field is additive, so the metadata version stays at 10 and older payloads remain compatible. COD-3016
9f62198 to
ca7c0f6
Compare
| /// Exclude memory allocation time from simulation results. | ||
| /// | ||
| /// Signals the backend to parse and subtract allocator time when interpreting | ||
| /// the run. Runs with different values are not comparable, so the value is part | ||
| /// of the run's measurement configuration. | ||
| #[arg( | ||
| long = "unstable-exclude-allocations", | ||
| default_value_t = false, | ||
| help_heading = "Experimental", | ||
| env = "CODSPEED_UNSTABLE_EXCLUDE_ALLOCATIONS" | ||
| )] | ||
| pub exclude_allocations: bool, |
There was a problem hiding this comment.
We have three different schemes for the three exeprimental args
one encodes experimental in the field name
one encodes unstable as part of the clap proc macro
one does not do any of it
can we align these? I suggest keeping experimental because it's the oldest and the most likely to be used by extermal users, but I'm fine with unstable as well
| /// Signals the backend to parse and subtract allocator time when interpreting | ||
| /// the run. Runs with different values are not comparable, so the value is part | ||
| /// of the run's measurement configuration. |
There was a problem hiding this comment.
Too much information IMO here
No description provided.