Re: Profiler class, suggestions and comments welcome
On Aug 19, 8:21 pm, Francesco <entul...@gmail.com> wrote:
On 19 Ago, 18:02, Victor Bazarov <v.Abaza...@comAcast.net> wrote:
Francesco wrote:
namespace profiler {
class Profile {
public:
Profile(std::string = "");
For the sake of performance (and with a profiler it's kind
of important, wouldn't you say?) you should *never* pass
objects of class type by value. *Always* prefer passing by
a const ref, unless there is a strong reason not to.
You're right, of course performance is important, even more in
this particular case
Yes and no. I suspect that the difference in performance here
would be negligible in most applications; performance typically
isn't important, at least at this level. (Not using O(n^2)
algorithms when you know you have a lot of data is important,
for example.) And even here, the copy doesn't occur in the part
of the code being measured.
Still, conventions are conventions, and the usual convension is
to pass class types by const reference, so I'd do it. (Even in
cases where it might result in a slight loss of performance,
e.g. for the copy assignment operator.)
[snip]
profiler::Profile::Profile(std::string s) {
id = s;
*Always* prefer initialisation (using the initialiser list)
over assignment. There is a FAQ about that. Have you
actually read the FAQ?
Yes, I read it. Actually, my intention here was to hide the
fact of using "clock()" and "clock_t" by not including <ctime>
into the profiler's header.
But the constructor isn't in the profiler's header.
Now I realize that hiding that implementation detail is way
less important than writing the constructor in the most
efficient way.
No. Hiding the implementation details is far more important
than performance (until actual measurements show otherwise). On
the other hand, the rules of C++ don't allow perfect hiding, and
<time.h> isn't exactly a header that's constantly changing, so
using clock_t in the class wouldn't bother me. (Using some
complicated user class might, depending on the context, and for
all but the simplest classes, I tend to adopt the compilation
firewall idiom automatically. But I don't avoid dependencies on
standard headers.)
[...]
-------
// header (double-inclusion check snipped out)
#include <ctime>
#include <string>
namespace profiler {
class Profile {
public:
Profile(const std::string& sid)
: id(sid), start_time(clock()) {}
Does this really have to be inline.
~Profile();
private:
std::string id;
clock_t start_time;
And the order of the declarations here is extremely important!
You got it right, but since it's so exceptional for the order to
be important, I'd add a comment stating why, so that a
maintenance programmer doesn't accidentally change it.
};
std::string report();
}
What happens if you copy this class? Do you want to allow that?
(IMHO, it doesn't make sense.) If not, you need to inhibit it.
// implementation
#include <map>
#include <sstream>
// profiler header inclusion snipped
The comment takes as much place as the inclusion:-).
Also, it's a good idea to include the header for the class
you're implementing first, in order to ensure that it doesn't
accidentally contain other dependencies (e.g. that it only
compiles if the user has included <map> first).
namespace {
struct timing_record {
clock_t used_time;
int hits;
timing_record() : used_time(0), hits(0) {}};
typedef std::map<std::string, timing_record> timing_map_type;
timing_map_type t_map;
}
profiler::Profile::~Profile() {
clock_t ct = clock() - start_time;
timing_record& tr = t_map[id];
tr.used_time += ct;
++tr.hits;
}
std::string profiler::report() {
timing_map_type::const_iterator iter;
Never define a variable until you're ready to initialize it.
std::stringstream res;
Unless you can't avoid it:-).
for(iter = t_map.begin(); iter != t_map.end(); ++iter) {
I'd have defined the iterator in the for.
res << iter->first << "\n";
const timing_record& tr = iter->second;
res << "Calls: " << tr.hits << ", ";
res << "avg time: " << tr.used_time / tr.hits << "\n";
There are two things wrong with this statement. First, you
don't scale the time by CLOCKS_PER_SEC, so you're outputting
in some implementation defined unit. (Posix requires that
CLOCKS_PER_SEC be 1000000, but I've seen other values.
Including 60 on some older systems.) The second is that you
probably want the results in a floating point type; for a very
short function with a lot of hits, the used_time might actually
be less than hits, and even in less extreme cases, the results
may not be very accurate. Try something like:
res << "avg time (ms): "
<< 1000.0 * tr.used_time / CLOCKS_PER_SEC / tr.hits
<< std::endl ;
(In general, until you really, really know what you're doing,
and can write practically error free code from the start, you
should prefer std::endl to '\n'.)
}
return res.str();}
More generally, I'd have written this function:
void Profiler::report(
std::ostream& dest )
{
// ...
}
It seems likely to me that most of the time, it will be called
for direct output, so you might as well format directly into the
target stream. This also gives the user the possibility of
setting various format flags (probably not very useful here, but
you never know---if you adopt floating point, precision could be
relevant). If you really do want to offer the possibility to
get a string, you can always overload with:
std::string Profiler::report()
{
std::ostringstream result ;
report( result ) ;
return result.str() ;
}
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34