Repository navigation
Replace "Sum Total" synthetic row with "Geomean" - #341
Conversation
Otherwise, the heavy benchmarks dominate.
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Comparing based on geomean sounds great (it's the best way to compare speedups/rates IMHO) but I'm not sure I understand something below -- clarification would be great:
| /// engine's own noise and does not cancel between two engines: a merely noisier | ||
| /// engine would be reported as faster. Rescaling is a constant factor per group, | ||
| /// so it moves the mean without touching the relative spread the confidence | ||
| /// interval is built from. |
There was a problem hiding this comment.
I'm not sure I understand this paragraph: can you explain a bit more what is actually going on? Why are we trying to make arithmetic means and geometric means match?
There was a problem hiding this comment.
The way we implemented sum total rows was to inject synthetic rows in the benchmark data for each iteration. The "benchmark" was "Sum Total" instead of an actual .wasm path and the count was the sum of all the benchmarks' counts for this iteration. From here on out, these rows were treated the same as any other, and this is how we got confidence intervals and significance tests for "Sum Total" rows For Free(tm).
We do the same thing for "Geomean" now. However, taking the average of averages introduces skew, IIUC, and this skew is related to how noisy the control/variable engine is. The rescaling is an attempt to bound this skew and keep it to a level that doesn't matter for our purposes, and we have tests enforcing this.
I don't love it, and I'm not married to it. The alternative would be to stop injecting these synthetic rows and getting the confidence intervals, significance tests, and all that For Free(tm) and instead have special-cased code to compute it at a higher level in the benchmark data analysis. I don't think we could reuse the existing crates/APIs we use for significance testing today if we did this because IIUC we would be working in log space, but I'm not 100% sure, I'd need to dig into it more.
For now, I've dialed back the detail on this comment, boiling it down to its essence. Let me know if you think we should continue with this PR's approach, or if I should start investigating the alternative.
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
OK, that seems reasonable -- the way this fits into the existing pipeline is fine, I just didn't understand it / couldn't parse that paragraph. Happy to see this ship and we can always refine/improve later. Thanks!
2 participants
Otherwise, the heavy benchmarks dominate. This is especially bad for compile time, where one benchmark can be up to 40% of the total benchmarking budget.
r? @cfallin