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

[Fabien Tassin]

According to 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.

AFAIR, it is HIS_SYNC called while renumbering the active file in ICDsetup().
With my implementation, it should not be a problem to start the timer
at the beginning but it is bad to call a timer before the initialization.

> > - 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,

in which way ? 

>  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

I don't think we have to care about begin thread-safe in the current INN.
It will only add complexity and it will remain only partial as everything
else is nor threaded nor threadable. I think thread should be kept for
INN 3.* which I hope will be based on INNT.

> (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.

If you read the patch, you'll see that once each timer has been called
once, it is only a matter of moving a pointer to its direct neighbour
or at worse, a simple one way linear search.

>  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.

a simple array lookup given which index ? I fear I do not follow you
here. How big will it be for a 5 levels nested timer taken in a list of 10
declared timers ?? 10^5 * sizeof(struct timer) ?

> 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?

Looking only at your timer struct, I can't see what you'll gain compared
to my implementation. If you want to substitute my lists by arrays, you'll
spend your time in (re)init code.
Or did I miss something ?

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

If you do that, I wish to add a level to each timer and an argument to
"ctlinnd time" with its pending option in inn.conf. If a given timer is
called with a level greater than the current limit, it is ignored.
It will allows us to use nested timers spread at all critical points
with no cost for those that are not interested by them.

-- 
Fabien Tassin -+- fta at sofaraway.org