Code review comment for lp:~prafulla-t/drizzle/drz-fast-timer-and-time-profile-refactoring

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/17/2010 08:18 AM, Prafulla Tekawade wrote:
> Prafulla Tekawade has proposed merging lp:~prafulla-tekawade/drizzle/drz-fast-timer-and-time-profile-refactoring into lp:drizzle.
>
> Requested reviews:
> Drizzle Merge Team (drizzle-merge)
>
>
> Hi
> This is first attempt to have timer classes and easy macros
> for doing time-related profiling of drizzle code.

This looks very potentially useful.

> We have three choice for timer classes.
> rtdtc based timer
> clock_gettime based timer
> gettimeofday based timer.

Could you also investigate Boost::DateTime and see if it's a useful
choice for timer class implementation? I'm looking at using it for some
other things as well and I'd love it if we could have one set of
time-related code.

However, if it's not suitable, that's fine.

> As an example of how macros can be used, I have added few timer(uSec precision) profile related macros around filesort code.
>
> Here what is dumped in error stream once query/command execution is done.
>
> -------------Profile Output----------
> query
> {
> mysql_parse, acc= 366639 microsec, occ=1
> }
>
> filesort
> {
> estimateRowsUpperBound, acc= 5 microsec, occ=1
> filesort, acc= 262365 microsec, occ=1
> find_all_keys, acc= 227612 microsec, occ=1
> innodb::releaselatches, acc= 3 microsec, occ=1
> make_char_array, acc= 18 microsec, occ=1
> optimizer::SqlSelect::skip_record, acc= 18647 microsec, occ=12010
> raw_read, acc= 68733 microsec, occ=12011
> write_keys, acc= 46767 microsec, occ=12010
> }
>
> acc = accumulated time
> occ = number of timers timer code was invoked.
>
> In order to enable time profiling, we need to turn on TIME_PROFILE
> macro in time_profile.h
>
> I hope this would be helpful in profiling various part of drizzle code.
> Could you please go over the code and suggest improvement if any ?
>

A few quick notes...

Please do not include config.h in drizzled/time_profile.h ... it will be
included by any .cc files that use it.

For the output - would you see if it's possible to hook in to the error
message infrastructure? (errmsg_printf and friends) - that way it's
conceivable that someone could use profiling in an infrastructure where
server messages are reported to a centralized server. Like
boost::datetime, it's possible this won't work well yet - I've been
meaning to add a stream-like interface to that system and just haven't
gotten there yet.

Other than that it looks reasonable (I haven't walked through the actual
timer impl code fully yet.)

Monty
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkxqq2AACgkQ2Jv7/VK1RgEAngCguk02ETTC9xj7ruBbalvLr/B7
n7IAoO7Yr/Py6y3bu6P8RFzQNfzQ1nY0
=UPvq
-----END PGP SIGNATURE-----