Add support for other metrics/units#18
Conversation
|
Hi @matheusd, Thanks for Initiative. LGTM. You could move on. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| benchStat = shared.Stat{ | ||
| Type: "Throughput", | ||
| Value: float64(value.Value) / 1e6, | ||
| Unit: "MB/s", | ||
| NotPerOp: true, |
There was a problem hiding this comment.
Adding the throughput branch here means only benchmarks that emit B/s (i.e., those calling SetBytes) get an extra Stat, while others in the same run still have the original length. GenerateHTMLCharts iterates stat indices from the first result across all results and blindly indexes r.Stats[statIndex]; if the first benchmark includes throughput but a later one does not, chart creation will panic or shift subsequent metrics under the wrong title. This shows up on a mixed benchmark file where the first test sets bytes per op and the next one doesn’t. Please normalize the stat slices before charting (e.g., pad missing entries or align by stat type) so mixed workloads don’t crash.
Useful? React with 👍 / 👎.
fahimfaisaal
force-pushed
the
main
branch
2 times, most recently
from
159daef to
7629bfa
Compare
2 participants
Initial work to support metrics other than the common ones. In particular, this adds support for the standard throughput metrics (MB/s) added by the
SetBytes()call.This is only a basic implementation, I can add the missing stuff later (tests, parametrization) if you think this is in the right direction and something desirable.