Support building without statistical analysis #19
Conversation
runBenchmark now accepts the threshold of sample duration and how many samples we must take which are above the threshold. This will allow us to use runBenchmark in more flexible ways.
In this mode: 1) Speed is roughly 10x faster e.g. 5 secs vs 50 secs. No more waiting for results. 2) There is no statistical analysis of samples done 3) We take 2 samples above 30ms duration and print the stats for the last one. We do not take avg, min or max. We print just one sample because all the stats can be consistently interpreted with respect to each other. That is not possible when we take averages. 4) In default mode only the time is printed, in verbose mode all stats are printed. In future we can have a command line selection filter to select which stats are to be printed. 5) Results are quite stable, there is a little more variance compared to statistically analysed results but it is not significant. The bang for the buck is quite high.
At the sample cature time itself we now capture only those samples that are above the threshold so that we do not have to remember the threshold to filter them out later during analysis. This removes a dependency between capture and analysis code.
benchmark/benchmarkWith use quick analysis - belong to Internal.hs benchmark'/benchmarkWith' use statistical analysis - belong to Analysis.hs This fixes the dependencies and now Analysis.hs is completely independent. Only Main.hs and Gauge.hs depend on it at the top level.
Because of rounding errors mkCL 0.95 was not matching with cl95. We now use cl95 when the user does not specify a confidence level.
Otherwise we may have inconsistency across different stats. Anyway if a stat is optional it must either be present in all samples or absent in all samples.
Gauge/Internal.hs
Outdated
| @@ -71,8 +71,8 @@ runOne desc bm file = do | |||
| Just prog -> gaugeIO $ runBenchmarkWith prog desc timeLimit quickMode | |||
| Nothing -> gaugeIO $ | |||
| if quickMode | |||
| then runBenchmark bm threshold 2 0 | |||
| else runBenchmark bm threshold 10 timeLimit | |||
| then runBenchmark bm 30 2 0 | |||
There was a problem hiding this comment.
I don't think it's a good idea to remove the self-explanatory binding threshold in favor of hardcoding the value in 3 different places with no explanation. it would have been easier to move to internal as a constant relative to measurement instead of analysis.
There was a problem hiding this comment.
I thought quite a bit when writing this and then finally discarded the binding. The reasoning was if I am using a binding for threshold then I should use a binding for number of samples as well (the other hardcoded argument). When I used that as well I thought I am adding too much for too little gain because the function definition explains the arguments and if we type them better later then it will become obvious.
Also, this is the only place where we would be using it. The other place i.e. getOverhead is an ugliness that should go away very soon.
|
I like that we can now build without any stats stuff and have a quick mode. definitely something I wanted to have. The PR was a bit hard to follow though |
|
Thanks for spending time on it and merging it quickly. Cheers to building a kickass perf tool! You have already done quite a bit of work by simplifying the code and making it amenable to changes and that's how I could make more changes quickly. But, there is a lot more we need to do. I have a couple of big rocks on mind, we'll discuss. |
This PR is based on the previous, yet unmerged PR. It modularises the analysis and measurement code. I added a build flag too to compile without statistical analysis. When compiled without statistical analysis it defaults to quick mode.
The commits are better viewed individually rather than as a whole, they are pretty incremental in nature, each one is small (when logical changes are made) or simple (e.g. when code movement is done).