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 <[email protected]> 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. >
