httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: Event(opt) skiplist's indexing_compk function (and unused skiplist index)
Date Mon, 13 Oct 2014 12:04:34 GMT
The skiplist impl was via a donation of the code from Theo
who developed it as part of mod_spread (iirc) et.al. so
it likely includes functionality that we don't use *yet*.
But having functionality in it allows for future use.

On Oct 13, 2014, at 7:25 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> Hi,
> 
> I think there is some confusion in the comparek() function set for the
> timers' skiplist in mpm_event(opt) (or am I the one confused?).
> 
> In the APR skiplist implementation, the comparek() function is used by
> apr_skiplist_find[_compare]() and apr_skiplist_remove[_compare]() to
> find the given entry (data), calling skiplist->comparek(data,
> node->data) until 0 is returned for a skiplist node.
> 
> These functions are not used by event(opt), so there is no harm...
> But should they, with comparek defined as :
> 
> static int indexing_compk(void *ac, void *b)
> {
>    apr_time_t *t1 = (apr_time_t *) ac;
>    apr_time_t t2 = (apr_time_t) (((timer_event_t *) b)->when);
>    AP_DEBUG_ASSERT(t2);
>    return ((*t1 < t2) ? -1 : ((*t1 > t2) ? 1 : 0));
> }
> 
> we would compare *(apr_time_t *)data to ((timer_event_t
> *)node->data)->when, whereas the given data is really (also) a
> timer_event_t* (unfortunately "when" is not the first field of the
> timer_event_t struct either).
> 
> So probably there should be no need for a special indexing_compk() in
> event(opt), and we should use indexing_comp() (without the k) as both
> compare() and comparek() functions for apr_skiplist_set_compare().
> 
> Do you agree I should commit this change?
> 
> Moreover, since we don't use apr_skiplist_find*() or
> apr_skiplist_remove*(), we probably don't need indexing at all for our
> skiplists, and could avoid the overhead of inserting into the
> index(es) for each apr_skiplist_insert(). That's an APR patch though
> (moving the index creation from apr_skiplist_init() to
> apr_skiplist_add_index(), ie. the first time it is used), I'll propose
> it on apr@.
> 
> Regards,
> Yann.
> 


Mime
View raw message