httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: apr_skiplist dependency
Date Fri, 13 Mar 2015 09:24:56 GMT
That is:

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c    (revision 1666294)
+++ server/mpm/event/event.c    (working copy)
@@ -1471,6 +1471,15 @@ static int indexing_compk(void *ac, void *b)
     return ((*t1 < t2) ? -1 : ((*t1 > t2) ? 1 : 0));
 }

+static int indexing_add_comp(void *a, void *b)
+{
+    apr_time_t t1 = (apr_time_t) (((timer_event_t *) a)->when);
+    apr_time_t t2 = (apr_time_t) (((timer_event_t *) b)->when);
+    AP_DEBUG_ASSERT(t1);
+    AP_DEBUG_ASSERT(t2);
+    return ((t1 < t2) ? -1 : 1);
+}
+
 static apr_thread_mutex_t *g_timer_skiplist_mtx;

 static timer_event_t * event_get_timer_event(apr_time_t t,
@@ -1500,8 +1509,8 @@ static timer_event_t * event_get_timer_event(apr_t
     te->remove = remove;

     if (insert) {
-        /* Okay, insert sorted by when.. */
-        apr_skiplist_insert(timer_skiplist, (void *)te);
+        /* Okay, add sorted by when.. */
+        apr_skiplist_insert_compare(timer_skiplist, te, indexing_add_comp);
     }
     apr_thread_mutex_unlock(g_timer_skiplist_mtx);

Index: server/mpm/motorz/motorz.c
===================================================================
--- server/mpm/motorz/motorz.c    (revision 1666294)
+++ server/mpm/motorz/motorz.c    (working copy)
@@ -81,6 +81,15 @@ static int indexing_compk(void *ac, void *b)
     return ((*t1 < t2) ? -1 : ((*t1 > t2) ? 1 : 0));
 }

+static int indexing_add_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 apr_status_t motorz_conn_pool_cleanup(void *baton)
 {
     motorz_conn_t *scon = (motorz_conn_t *)baton;
@@ -305,7 +314,7 @@ static void motorz_register_timer(motorz_conn_t *s
     elem->mz = mz;

     apr_thread_mutex_lock(mz->mtx);
-    apr_skiplist_insert(mz->timer_ring, (void *)elem);
+    apr_skiplist_insert_compare(mz->timer_ring, elem, indexing_add_comp);
     apr_thread_mutex_unlock(mz->mtx);
 }
--


On Fri, Mar 13, 2015 at 10:05 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
> I think I have an acceptable solution for still APR-1.5's skiplists
> (having insert() with addne semantic) in httpd.
>
> For add semantic, simply use the following compare function and
> insert_compare():
> static int indexing_add_comp(void *a, void *b)
> {
>     apr_time_t t1 = (apr_time_t) (((timer_event_t *) a)->when);
>     apr_time_t t2 = (apr_time_t) (((timer_event_t *) b)->when);
>     return (t1 < t2) ? -1 : +1;
> }
> It never returns 0, hence the entry never exists, and duplicates are added next!
>
> That's all is needed for mpm_event, which only uses skiplist_insert()
> and skiplist_pop(), so it could even become the default compare
> function (ie. set with apr_skiplist_set_compare()).
>
> For mpm_motorz, we also need skiplist_remove(), so we can keep the
> existing indexing_comp() function as the default (we still need the
> exact match to find or remove an entry :), while the new
> indexing_add_comp() would only be used for insertions (with
> insert_compare()).
>
> Any issue with this?
>
> On Sat, Mar 7, 2015 at 8:02 PM, Jim Jagielski <jim@jagunet.com> wrote:
>> My 2c is that I don't like the idea that "add" and "insert"
>> means 2 different things with 2 different behaviors. I
>> know we were kinda forced into that due to me, mistakenly,
>> not realizing that dups were the *compliant* impl.
>>
>> The real rub is what do we call "place into skiplist
>> unless it would overwrite a previously placed element".
>> My choice of "addne" was shorthand for "ADD if element
>> does Not Exist".
>>
>>> On Mar 7, 2015, at 11:38 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>>>
>>> +1
>>>
>>> If/before we do this, maybe we can at least agree on
>>> skiplist_insert/add() vs skiplist_insert/addne().
>>> I known it depends on whether or not names will finally be rollbacked
>>> in APR, but we'd better not copy skiplist's code from there before
>>> this is decided.
>>> Just to avoid having different semantics in APR and httpd, and ease
>>> keeping the code in sync when necessary...
>>>
>>>
>>> On Sat, Mar 7, 2015 at 5:10 PM, Jim Jagielski <jim@jagunet.com> wrote:
>>>> I'm +1 on that.
>>>>
>>>> <rant>
>>>> I'll be honest, I think that in several ways the "lag"
>>>> between httpd and APR is putting httpd at risk. Every
>>>> possible change to APR is being held-up by, imo, irrational
>>>> concepts of what "breaks" the API. Now this wouldn't be
>>>> so bad if we saw releases of APR more often than every
>>>> year or so. As it is, we end up with things that should
>>>> be in APR, but aren't, because we want to be able to keep
>>>> development and release of httpd at a rationale level,
>>>> OR things get "pushed" into APR quickly because, well,
>>>> with such infrequent releases, people (myself very included)
>>>> rush stuff in because we know it'll be months (several!)
>>>> before the idea of an APR release is even envisioned. ("Hurry
>>>> up and commit! God knows when the next release will be!!")
>>>>
>>>> Personally, I think any "new" features that httpd requires
>>>> than would historically go into APR, STAY in httpd. Call
>>>> it aprx_ or whatever. APR can decide to pick those up when/if
>>>> it chooses. It can even live in ./srclib.
>>>> </rant>
>>>>
>>>> As far as what to do w/ skiplist itself: httpd (event and motorz
>>>> and likely others, when its available) requires a *compliant*
>>>> skiplist implementation. If APR can't provide it, then we
>>>> provide our own.
>>>>
>>>>> On Mar 7, 2015, at 7:40 AM, Jeff Trawick <trawick@gmail.com> wrote:
>>>>>
>>>>> Looking back, I think that apr_skiplist wasn't ready for general use
(both doc and code) when it was put in APR and released, and at the same time it was unfortunate
that it placed a prereq on a new APR release in order to use Event, introducing another speedbump
to using httpd's latest and greatest.
>>>>>
>>>>> Looking forward, I see the same thing; apr_skiplist needs design changes
to supply what httpd needs, and doing so means extra work in APR to preserve compatibility,
as well as dependence of Event on a new APR release stream.  That's another speedbump that
httpd 2.4 users don't need.
>>>>>
>>>>> The best thing for httpd (Event) w.r.t. skiplist is the best thing for
apr_skiplist itself: for the codebase to evolve naturally to support this important (primary,
only???) consumer, without the constraints of APR versioning rules and APR release cycles
(and perhaps without the constraints of any versioning rules).
>>>>>
>>>>> Pull this small amount of code into httpd, perhaps as a private interface
for 1-2 modules that need it.  Let it improve with fewer constraints.  Future APR 2.0 will
improve from the relatively unfettered changes, and httpd 2.4 users won't have speedbumps
introduced by dependence on an evolving skiplist implementation.
>>>>>
>>>>> (Keep the skiplist code in APR trunk up to date with changes needed for
httpd.  The APR stable releases can pick up compatible code fixes, but that probably won't
be a priority for anyone but non-httpd consumers, and I'm not aware of any such people working
on skiplist thus far.)
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> --
>>>>> Born in Roswell... married an alien...
>>>>> http://emptyhammock.com/
>>>>>
>>>>
>>

Mime
View raw message