[WIP] Adding a througput chart to the benchmark summary. #315
Conversation
|
It seems I got confused by the two structs report::BenchmarkId which has throughput and BenchmarkId which doesn't store throughput in benchmark_group.rs which I also mistakenly assumed I was passing around. |
|
Hey, sorry I haven't had a chance to respond to this yet. Yeah, |
There was a problem hiding this comment.
As you noticed, you can get the throughput out of the benchmark ID so all of the changes made to pass the throughput along are unnecessary. There's also some leftover debug output.
Otherwise this looks good. It won't make it into 0.3.0 (which I plan to release today) but it won't require a breaking change so we can add it to 0.3.1.
|
Thanks, I updated this pull request, let's see if it passes CI. |
|
Finally got around to addressing the reviews. The build currently fails due to unrelated issues,should be fixed with #336 |
|
I changed the plotting for throughput slightly to take into account parameterized benchmarks. |
Otherwise the summary plot may end up with different unit display as the #elements can change for different benchmark.
Otherwise this leads to problem in graphs where we can have different values in Throughput::Elements, or Throughput::Bytes that lead to putting values in the same plot with different units. However, after changing this I had to adjust the format_throughput function because it would just use the raw time as a typical value before which could lead to odd unit choices (like 2_000_000 Kelem/s). With these changes, now the format_throughput also takes a typical value that can be estimated in advance with Throughput::per_second().
Also finds the maximum tput for the plot properly by looking at tput values and not just mean.
|
Hey! Just wanted to ask if there's any news on this PR, as it'd be very nice :) |
|
Hey, the branch/PR works/worked for me -- feel free to give it a try ... Unfortunately, I couldn't quite fit my requirements for a multi-core benchmark harness into criterion (even with |
|
It would be really nice to have this in. Although it fails to generate a plot for logarithmic scale (min yrange can't be zero). That should can be solved by removing |
|
Hey folks, thanks for your patience. Yeah, if someone else wants to take over and finish this off, that'd be great! |
I'm ready to review if you take over this PR. |
lemmih
changed the title
|
Closing due to inactivity. |
Fixes #149. Fix issues from review in bheisler#315. Fix throughput check and make plot a scatter instead of line plot. Include ParameterizedBenchmark config string in plot label. Plot label takes function parameterization into account. Put deny warnings back. Make unit decision only based on typical value. Otherwise the summary plot may end up with different unit display as the #elements can change for different benchmark. Undeny warnings to avoid OsRng error. Make throughput unit decision only based on typical_value. Otherwise this leads to problem in graphs where we can have different values in Throughput::Elements, or Throughput::Bytes that lead to putting values in the same plot with different units. However, after changing this I had to adjust the format_throughput function because it would just use the raw time as a typical value before which could lead to odd unit choices (like 2_000_000 Kelem/s). With these changes, now the format_throughput also takes a typical value that can be estimated in advance with Throughput::per_second(). Fix plot to have a line similar to line chart. Also finds the maximum tput for the plot properly by looking at tput values and not just mean.
A first version to address #149. This adds a throughput chart to the summary page.
Some more thoughts:
It was a bit of a nuisance to get the Throughput struct down to the reporting and plotting, maybe there is an easier, better way that I don't know?
Also, plot::line_comparison_throughput is doing unwrap on Option so the caller should check that every BenchmarkId has a throughput (and that its of the same type probably). I can add that if the general direction of this pull request is deemed fine.
Maybe, instead of a line-plot a barchart will work better here?
To test I use: