[INN-COMMITTERS] inn/lib (timer.c Makefile)

[Russ Allbery]

Fabien Tassin <fta at sofaraway.org> writes:

> Here is the patch.. 3 files are impacted.

> - 2 to move TMRinit() *before* the first call to TMRstart()

What's calling TMRstart before CHANreadloop?  Whatever that is should
probably be fixed.  The timer statistics are going to be sort of
meaningless before the main loop starts, since innd will still be doing
all sorts of setup stuff that won't be representative of its normal
operation.

> - 1 for */timer.c. If the recent changes are that general, this patch
> should not be a problem.

Yup, I can merge this in, no problem.

Looking this over, though, I think I can simplify this, and I'm thinking
that since we're all in the middle of this timer code anyway, maybe we
should go ahead and bite the bullet and change the API so that it's
thread-safe (by returning a context from TMRinit -- or timer_init, since
I'd rename all the functions if we change the API to make sure that I can
catch all of the calls -- and requiring it as the first argument to all
the other calls).  We'll have to do this sooner or later anyway if we're
going to use this code with multiple threads.

I think we can also make this somewhat more efficient, not that it
probably matters all that much but linked lists are always annoying to
traverse.  How's this sound, for solving both of those problems....

struct timings {
    unsigned int max;
    unsigned int current;

    struct timer *timers;
}

struct timer {
    unsigned long start;
    unsigned long cumulative;
    unsigned long count;

    struct timer *parent;
    struct timer *first_child;
    struct timer *last_child;
    struct timer *next;
}

timer_init would return a struct timings * and everything else would take
a pointer to that struct.  It contains an array of timers, so starting and
stopping timers will still be a simple array lookup rather than having to
chase pointers to find one.  If a timer is already running, set parent of
the new timer to point to the running timer and update last_child of the
parent and the next pointer of the old last_child (provided that parent is
NULL; if parent isn't NULL and doesn't match current, that's an error and
we can warn and reject the timer start.

Then, the same summary routine that your code uses can be used to produce
the right output in the right order.

I think that may be easier to follow (I hate reading code that constructs
linked lists), but that could be my prejudices showing.  What do you
think?

If the above sounds good, I can do the work and change all the current
timer calls.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>