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

[Russ Allbery]

Fabien Tassin <fta at sofaraway.org> writes:

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

Okay, I'll take a look.

>> Looking this over, though, I think I can simplify this,

> in which way ? 

Hm.  Simplify may be the wrong word.  I like the model where turning on or
off a timer is just a matter of a simple array lookup, though.  One of the
nice things about that is that none of the tree manipulation comes into
play unless there's actually a nested timer.

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

Yeah, but I'd like to use the same infrastructure for INNT as much as
possible rather than reinventing it (which basically means salvaging the
infrastructure work in libinn and using the new history API).  Could you
use that same timer implementation for INNT?  Could you use one that used
a context parameter?  If you couldn't use the former and could use the
latter, I think it's worthwhile to make the timer library potentially
threadsafe now so that you can use the same library as the existing
architecture.

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

Right, I saw.  But the linear search is the common case with no nested
timers.  It's not a big deal; for innd, it's a matter of a linear search
through an approximately 15-20 item linked list rather than an array
lookup.  But I think I can get the array lookup back without any negative
effects and while still generating the same tree structure that your
implementation was generating.

>>  How's this sound, for solving both of those problems....

>> struct timings {
>>     unsigned int max;
>>     unsigned int current;
>> 
>>     struct timer *timers;
>> }

The above being the context (and we could add more elements to it as
needed).

>> 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;
>> }

The idea, which I didn't explain all that well, is to allocate all the
timers (whether they're nested under another timer or not) in a flat array
of timers indexed by their id.  (Hm, that struct does need unsigned int id
in it as well; forgot that.)

When a timer is started, we find it in the array with a simple array
lookup.  We then check to see if we currently have a timer running (via
current).  If we do, we make sure that the timer we're starting already
has the running timer set as its parent.  If it doesn't, we grab a pointer
to the running timer via an array lookup, set parent of the new timer to
the running timer, set parent->last_child->next to the new timer, and then
set parent->last_child to the new timer (and set parent->first_child to
the new timer if it's NULL).

When we stop a timer, we follow its parent pointer and set current to the
id of that node (if parent isn't NULL).  Otherwise, we set current to the
"no timers are running" value.

So there's only one array of timers, and the tree structure is constructed
dynamically as the timers are started and stopped.  To summarize, we'd
then walk the array, skipping any timers that have a non-NULL parent, and
for any timer that has a non-NULL first_child we'd decend into the same
tree-walking algorithm that your implementation was using until we reached
the bottom of the tree, and then resume the linear scan of the array.

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

Should just be 10 * sizeof(struct timer).  The above should explain better
what I'm talking about.

It's not that big of a deal; if you don't think it would be worth it, it
probably isn't.  (It also wouldn't be more than an hour or two for me to
implement on top of your existing code, though.)

Oh, wait... I missed something in reading your code the first time.  That
implementation actually allows the same timer to be called at different
levels; there can be a "history sync" at the top level and a separate
"history sync" that's a sub-timer of "history write".  Was that
intentional and one of the features you wanted?  If so, then my idea won't
work, since it requires that all timers live at one and only one place in
the tree hierarchy.

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

With the levels still being dynamically determined based on what order the
timers are called in?  And what would the ctlinnd time option do?  I don't
quite follow.

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