You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(valgrind): add --experimental-rep-bulk-cost flag - #620
Adds an opt-in --experimental-rep-bulk-cost flag (env CODSPEED_EXPERIMENTAL_REP_BULK_COST). When set together with --cycle-estimation, the runner passes --rep-bulk-cost=yes to Callgrind, which prices amd64 rep movs/rep stos from a calibrated bulk cost curve instead of the per-element model.
The option is off by default in valgrind-codspeed, so default results are unchanged. Once we are happy with the results, the runner can turn it on by default.
Depends on CodSpeedHQ/valgrind-codspeed#60. Requires a valgrind-codspeed release containing it and a bump of VALGRIND_CODSPEED_ITERATION before merging: older valgrind builds reject the unknown option.
System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.
### Issue 1
src/executor/valgrind/measure.rs:50-52
The new `get_valgrind_args` branch has no tests; the existing tests only cover the shell wrapper. Add checks that `--rep-bulk-cost=yes` appears only for Callgrind when both settings are true, and stays absent by default, with cycle estimation disabled, and with Tracegrind. This would catch future changes that silently alter benchmark costs.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
Adds opt-in --experimental-rep-bulk-cost and CODSPEED_EXPERIMENTAL_REP_BULK_COST support.
Benchmark runs can opt into Callgrind's calibrated REP-copy cost model.
Known prerequisite acknowledged by not-matthias: publish a compatible Valgrind release and bump VALGRIND_CODSPEED_ITERATION before merging, because older builds reject the option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an opt-in
--experimental-rep-bulk-costflag (envCODSPEED_EXPERIMENTAL_REP_BULK_COST). When set together with--cycle-estimation, the runner passes--rep-bulk-cost=yesto Callgrind, which prices amd64rep movs/rep stosfrom a calibrated bulk cost curve instead of the per-element model.The option is off by default in valgrind-codspeed, so default results are unchanged. Once we are happy with the results, the runner can turn it on by default.
Depends on CodSpeedHQ/valgrind-codspeed#60. Requires a valgrind-codspeed release containing it and a bump of
VALGRIND_CODSPEED_ITERATIONbefore merging: older valgrind builds reject the unknown option.Closes COD-4199