Re: Profiler class, suggestions and comments welcome
On 19 Ago, 18:02, Victor Bazarov <v.Abaza...@comAcast.net> wrote:
Francesco wrote:
Hi everybody,
with this very post I'm starting my first thread into this NG.
I know that my behavior so far has been [..]
If you didn't write this, nobody would notice. Everybody here couldn't
care less about your behavior unless it's against the spirit of the
forum or the rules of netiquette.
Uh... well, OK, I'll stop worrying about such things. Now I was about
to explain why I did it, but it's pretty obvious and nobody would
care, again. Sort of a good thing to know, I must remark ;-)
[snip]
Here is my profiler class. A couple of questions right after it:
-------
// combining header and implementation for easy copy&paste
// header (double-inclusion check snipped out)
#include <string>
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 - self-slap on the forehead :-/
[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. Now I realize that hiding that implementation
detail is way less important than writing the constructor in the most
efficient way.
clock_t& rct = time_map[id].start;
rct = clock();
}
So, your function here could be written as
profiler::Profile::Profile(std::string const & s)
: id(s)
{
time_map[id].start = clock();
}
I've followed the suggestion and extended it a bit, see the new code
below.
[snip]
- Why must I set "~Profile()" as public?
Because a local object 'p' is created *AND* destroyed from a scope that
has no special access to your 'Profile' class.
Yes, I've realized it just after posting the "how to ensure dynamic
allocation" question elsewhere and after reading Alf Steinbach's
response. But your details helped me refining further this new thing
I'm learning.
[snip]
- Notice how I used an additional variable and a reference to avoid
including the map lookup time into the logged time, in the ctor and
the dtor. Is this a smart move or is it just paranoia?
Pat yourself on the back. After you have stood in the corner for not
passing arguments by const ref.
Eheheheh, thank you very much for your "slaps" - luckily, this wasn't
a real-life code review ;-)
[snip]
Also, consider that the results of profiling of any particular function
are overridden by a subsequent calls, so you might consider improving
your system to (a) count the number of calls to that particular function
and (b) accumulate the overall time spent in it (instead of recording
start-stop ticks):
struct timing_result {
int hit_count;
clock_t total_time_spent;
};
This is really an important thing I missed, thank you very much for
pointing it out.
Some time later I'll ask for your advice about something more
interesting (more interesting to me: a simulation of ants harvesting
food around their nest). I'm not so sure, though. It will involve
posting the SDL interface code, asking questions about generic OOP and
about ants' behavior, hence I'm not sure this will be the right place
to post it - assuming I'll be ever brave enough to show my
"abomination" to the world ;-)
Remember that we often do not have time to delve into the intricacies of
your model domain. If you have a language problem or a question, we're
happy to help. Modelling and OOA/OOD are discussed in comp.object.
I guessed right, thanks for the pointers and finally thank you very
much for your quick and constructive reply, Victor.
Here is the improved version, applying all of your fixes and
suggestions:
-------
// 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()) {}
~Profile();
private:
std::string id;
clock_t start_time;
};
std::string report();
}
// implementation
#include <map>
#include <sstream>
// profiler header inclusion snipped
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;
std::stringstream res;
for(iter = t_map.begin(); iter != t_map.end(); ++iter) {
res << iter->first << "\n";
const timing_record& tr = iter->second;
res << "Calls: " << tr.hits << ", ";
res << "avg time: " << tr.used_time / tr.hits << "\n";
}
return res.str();
}
-------
Any further comments and fixes - and slaps ;-) - welcome.
Thank you for your attention,
Francesco