Re: Profiler class, suggestions and comments welcome
On 20 Ago, 10:49, James Kanze <james.ka...@gmail.com> wrote:
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.
Yes, you're right. With "particular case" I meant "creating a
profiler" not the specific case of the Profile ctor. I suppose that I
could profile nested functions, hence I suppose it would be good to
save as many CPU cycles as possible. Sure, profiling nested functions
will lead to "inflated" results on the outermost functions, but those
results could have a use anyway - easy on my "suppositions", they're
sincere suppositions, not sarcasm
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.)
Just like for the pointer arithmetic issue, I surely will take on
these good habits. Please point out as many of these as you see
fitting.
[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.
Fine, you spotted another misconception of mine. I thought I could use
the initializer list only when defining the function inside of the
class itself.
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.)
As above, I meant to refer to this very case of making a profiler.
But for the general rules I definitely agree with you.
[...]
-------
// 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.
I don't know, maybe I'll try to profile it to check whether it makes
any difference.
~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.
Very good point. I'll add that comment.
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.
Good point, I will.
// 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).
I never considered this, thanks a lot for pointing it out. Another
good habit to take on.
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.
Good points, taken all of them after Victor's and your advices.
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 ;
Very good point, I'll take on the scaling.
(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'.)
Sorry but I don't understand this, what do you mean exactly?
Do you mean typo-free (about the escaping) or something else related
to streams and flushing them with endl?
}
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() ;
}
Good features to consider, I'll take them on.
Thanks a lot for your in-depth advices, once more.
Francesco