httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Event(opt) skiplist's indexing_compk function (and unused skiplist index)
Date Mon, 13 Oct 2014 11:25:39 GMT
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