Maybe a reliable way to remove events would be to use the new
functions from r1671957 (something like the example added to
testskiplist.c in this commit)?
This can't be backported to APR-1.5.x though, so if that's suitable we
would have to copy the skiplist's code (back) to httpd (as discussed
elsewhere)...
On Mon, Mar 23, 2015 at 11:09 PM, Yann Ylavic wrote:
> On Sat, Mar 14, 2015 at 1:09 AM, wrote:
>> Author: ylavic
>> Date: Sat Mar 14 00:09:32 2015
>> New Revision: 119
>>
>> URL: http://svn.apache.org/r119
>> Log:
>> mpm_motorz: follow up to r1666482.
>> We only need one compare function for add semantic with apr_skiplist_insert()
>> and unique timers (pointers). It also works with apr_skiplist_remove() and
>> apr_skiplist_find().
>>
>> Modified:
>> httpd/httpd/trunk/server/mpm/motorz/motorz.c
>>
>> Modified: httpd/httpd/trunk/server/mpm/motorz/motorz.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/motorz/motorz.c?rev=119&r1=118&r2=119&view=diff
>> ==
>> --- httpd/httpd/trunk/server/mpm/motorz/motorz.c (original)
>> +++ httpd/httpd/trunk/server/mpm/motorz/motorz.c Sat Mar 14 00:09:32 2015
>> @@ -64,21 +64,18 @@ static motorz_core_t *motorz_core_get()
>> return g_motorz_core;
>> }
>>
>> -static int indexing_comp(void *a, void *b)
>> +static int timer_comp(void *a, void *b)
>> {
>> -apr_time_t t1 = (apr_time_t) (((motorz_timer_t *) a)->expires);
>> -apr_time_t t2 = (apr_time_t) (((motorz_timer_t *) b)->expires);
>> -AP_DEBUG_ASSERT(t1);
>> -AP_DEBUG_ASSERT(t2);
>> -return ((t1 < t2) ? -1 : 1);
>> -}
>> -
>> -static int indexing_compk(void *ac, void *b)
>> -{
>> -apr_time_t *t1 = (apr_time_t *) ac;
>> -apr_time_t t2 = (apr_time_t) (((motorz_timer_t *) b)->expires);
>> -AP_DEBUG_ASSERT(t2);
>> -return ((*t1 < t2) ? -1 : 1);
>> +if (a != b) {
>> +apr_time_t t1 = (apr_time_t) (((motorz_timer_t *) a)->expires);
>> +apr_time_t t2 = (apr_time_t) (((motorz_timer_t *) b)->expires);
>> +AP_DEBUG_ASSERT(t1);
>> +AP_DEBUG_ASSERT(t2);
>> +return ((t1 < t2) ? -1 : 1);
>> +}
>> +else {
>> +return 0;
>> +}
>> }
>
> I expected this compare function to work with apr_skiplist_remove()
> but actually it does not.
> The goal was to match the given pointer only (and not any other
> duplicate!), but by returning +1 when a != b && t1 == t2, we may skip
> timers inserted first should any duplicate be inserted later with a
> higher eight.
>
> For example, the below is the gdb output (dump_dipklist) after 10
> duplicates have been inserted, where each 2-tuple is the address of
> the node (0x...,) containing the address of the element (,00...):
>
> (gdb) dump_skiplist skiplist
> skiplist@0x69aba0: size=10: height=3
> (0x69b008,) (0x6ac3c0,) (0x6ac628,)
> (0x69b048,0069b000)
> (0x6ac338,0069b088)
> (0x6ac380,006ac378) (0x6ac400,006ac378)
> (0x6ac448,006ac440)
> (0x6ac490,006ac488) (0x6ac4d0,006ac488)
> (0x6ac518,006ac510)
> (0x6ac560,006ac558)
> (0x6ac5a8,006ac5a0) (0x6ac5e8,006ac5a0) (0x6ac668,006ac5a0)
> (0x6ac6b0,006ac6a8) (0x6ac6f0,006ac6a8)
> (0x6ac738,006ac730) (0x6ac778,006ac730)
>
> If e.g. we now try to remove the element (0x6ac448,006ac440),
> starting at the top (0x6ac628,), we will jump directly
> next to (0x6ac668,006ac5a0), then down (no next) to
> (0x6ac5e8,006ac5a0), then next to (0x6ac6f0,006ac6a8), then
> next to (0x6ac778,006ac730), then down (no next) to
> (0x6ac738,006ac730), and finally (no next, no down) to NULL.
> All the elements (all duplicates) between the top and
> (0x6ac668,006ac5a0) have been skipped, hence we had no chance to
> find (0x6ac448,006ac440), because at least one element was
> inserted later (exactly 4 here) with a higher eight.
>
> This is not an issue about insert vs add semantic, with both functions
> we still need a "reliable" compare function to remove the exact
> (requested) timer, and I see no way to build one...
>
> Thoughts?
