Re: [PATCH] profiler updates

On 07/05/2012 12:37 AM, Amos Jeffries wrote:
> This patch implements the changes we have discussed so far, apart from
> choosing the <atomic> std library atomics over the GNU ones.

Sigh.

> The behaviour and profiling coverage changes remains as described
> earlier. Just implementation details different to the last patch.
>
> If I missed anything or am still getting it wrong, please inform.

Well, making sure the code compiles would have been nice!

> +void
> +XProfilerNode::Initialize()

This method is not declared in the XProfilerNode class. The lower-case
version is. This should probably be a static method so fix the
declaration instead (capitalize and make static).

> + if (!xprof_inited.load()) {
> xprof_InitLib();

xprof_InitLib() has been removed by this patch.

> + // init may only be done once. potential callers need
> + static std::atomic_flag xprof_GlobalLock = ATOMIC_FLAG_INIT;
> + while(std::atomic_flag_test_and_set_explicit(&xprof_globalLock, std::memory_order_acquire))
> + ; // spin until the global lock is acquired

Ideally, this loop should be removed to avoid artificial synchronization
at the beginning of profiling (and future profiling code may start in
the middle of a Squid run). If the lock is needed and you cannot get it
at once, disable profiling for that node (i.e., do not start it). Here
is a sketch:

bool XProfilerNode::Initialize() {

    // common case first
    if (xprof_inited.load())
        return true;

    if (xprof_init_count++) <-- atomic (use std equivalent)
        return false; // somebody else got here first

    ... initialize ...
    xprof_inited.store(true);

    return true;
}

XProfilerNode::start()
{
    if (!xprof_inited.load() && !Initialize())
        return; // cannot start

    ...
}

> - /* Get here? We're at the top level; unaccounted */
> - xp_UNACCOUNTED->start = tt;
> +
> + hrtime_t stopped_ = get_tick();
> +
> + // calculate how long this timer was running.
> + hrtime_t delta = stopped_ - started_;

Please declare the stopped_ and delta variables as const if possible.

Please rename stopped_ to remove the underscore which implies that it is
a private class member. You can use "finished" if "stopped" is taken.

> +XProfilerNode::stop()
> {
> + if (started_ < 0)
> return;

You also need checks to prevent manually stopped nodes from stopping
twice (the second time during destruction). AFAICT setting started_ to a
negative value would suffice.

> +xprof_Init(void)
> {
> + xprof_delta = xprof_verystart = xprof_start_t = get_tick();
> Mgr::RegisterAction("cpu_profile", "CPU Profiling Stats", xprof_summary, 0, 1);
> }

I recommend calling xprof_Init() directly from main.cc instead of
calling xprof_event() which checks whether it needs to call
xprof_Init(). xprof_InitEvents() is also much easier to call from
main.cc -- it does not require knowing the sampling frequency and such.

Or you could use the new Registered Runners API to initialize this.

> void
> xprof_event(void *data)
> {
> - now = get_tick();
> - xprof_Init();
> + static bool inited = false;
> + if (inited) {
> + xprof_Init();
> + inited = true;
> + }

The logic here should be reversed, of course. This bug would disappear
if you call xprof_Init() directly from main.cc instead.

HTH,

Alex.
Received on Thu Jul 05 2012 - 17:13:20 MDT