Code review comment for lp:~glcompbench-dev/glcompbench/manmower-options-3

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Thanks for the explanation, it makes much more sense now. You are right that for this use case using separate Profiler instances would be messy.

Based on your patch I have implemented what is essentially a more generic version of your proposal:

https://code.launchpad.net/~linaro-graphics-wg/glcompbench/profiler-stat-collection

The current code in the branch is not complete yet. It only tracks per-iteration statistics, but adding per-test and total statistics is easy. The code is also missing the 'squares' stats datum, but this is trivial to add, too.

In your proposal the total stats are displayed after each test, but they are not really per-test stats, which I find somewhat confusing. I think it would be better to show per-test stats after each test is completed and total stats only once at the end.

Also, I would rather keep the accumulation logic outside of the profiler class. It's easy to calculate accumulated data outside the profiler, given the statistical data in the 'total' collection (calculating std. dev. needs a bit more care).

What do you think?

Meanwhile I will merge into trunk some of the other changes that don't depend on the substats infrastructure.

« Back to merge proposal