----- Original Message ----- From: "Gennadiy Rozental" <gennadiy.rozental@thomson.com> To: <boost@lists.boost.org> Sent: Monday, February 07, 2005 1:57 PM Subject: [boost] Re: Re: Boost Profiler Proposal [snip]
Well, I've read you docs and code. Here is what I think:
Once I started looking through proposed design it immideatly stroke me that you are making most common mistake in policy based designs (PBD): putting everything into one single policy. IMO PBD is not about providing complete implementation as template parameter (at least is not only about it). But mostly about finding small reusable and orthogonal! policies that in combination deliver flexibility and power. This is the reason why your policy are templates and why you need to implement counting, reporting and counting_and_reporting policies. These are orthogonal parts of profiler functionality. No need to combine them into single policy.
Good point, I agree that the timer_type should be extracted. However both reporting and collecting policies model the same concept, they react to when the profiler object is destroyed. Separating them does not seem neccessary to me. I think that particular point may be more of an issue of personal style, because overall it does not change the behaviour of the class.
Also I believe from my expirience that profiler model you chose is way too simple for real-life needs. Here is specific profiling modes I saw:
1. Take piece of code and wrap into timer. You cover this case 2. Take piece of code and time an average execution time during multiple code invokation. You model couldn't do that
Actually it can. The reporting_policy stores elasped_total and entry_count which can be used to construct averages. I have improved the collecting_policy to generate detailed reports: struct counted_sum : pair<int, double> { counted_sum() : pair<int, double>(0, 0) { } counted_sum(int x, double y) : pair<int, double>(x, y) { } void operator+=(double x) { first++; second += x; } }; typedef std::map<std::string, counted_sum> stats_map; struct collecting_policy { static stats_map stats; static void on_profiled(const char* name, double dSec, double dMin, double dMax) { stats[name] += dSec <= dMin ? 0.0 : dSec; } static void generate_report() { cout << "profile name" << '\t' << "total elapsed" << '\t' << "entry count" << '\t' << "average" << endl; for (stats_map::iterator i=0; i != stats.end(); i++) { string sName = i->first; int nCount = i->second.first; double dTotal = i->second.second; double dAvg = dTotal / nCount; cout << sName << '\t' << dTotal << '\t' << nCount << '\t' << dAvg << endl; } } }; stats_map collecting_policy::stats;
3. Frequently code you want to profile is "interrupted" with islands of code that you are not interested in. You need an ability to do accumulation
I would prefer to leave this to the user when they generate their report. I see no real obstacle for them to do the subtraction of stats.
4. Hierarchical checking points. Frequently you want to be able to see whe whole picture and easily add check points somewhere inside. In a result you want to see time from point A to point B then from point B to point C, compined time from A to C and so on.
See above.
Here is how I think policies could be designed:
1. TimerPolicy Responcible for "timing implementation", including timer, it's resolution e.t.c. Possible incarnations: BoostTimerPolicy, HighResolutionTimerPolicy, WallTimerPolicy. 2. CollectionPolicy Responcible for "what and how to collect" (elapsed time in one run, average time in multiple runs, profiler name, location, relations). Possible incarnations: SingleRunCollector. MultirunCollector, GlobalCollector e.t.c. This policy may need to be separated into two. 3. LoggingPolicy Responcible for "how to report". Here where Boost.Test integration kicks in. Possible incarnations: OstreamLogger, StdoutLogger, BoostTestLogger e.t.c. 4. ReportingPolicy Responcible for "when to report". Possible incarnations: ScopedReporter, ExplicitReporter, ReportNone (to be used with GlobalCollector that perform reporting itself)
and profiler definition would look like this:
template<typename TimerPolicy,typename CollectionPolicy,typename LoggingPolicy,typename ReportingPolicy> class profiler : public TimerPolicy, public CollectionPolicy, public LoggingPolicy, public ReportingPolicy { .... };
I have posted a new version of the code at http://www.cdiggins.com/profiler/profile2.hpp which separates the timer policy, and allows the generation of reports after execution. How do you feel about the new design, does it go far enough in your mind, or do you still want significantly more functionality? I feel relatively strongly that the library should simply provide the data to the user, and provide a minimal default functionality in terms of generating reports, and leave the rest up to the user. Thanks for your suggestions and feedback, Christopher Diggins