Return-Path: X-Original-To: apmail-httpd-dev-archive@www.apache.org Delivered-To: apmail-httpd-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 8BB7B17BEE for ; Fri, 13 Mar 2015 09:26:06 +0000 (UTC) Received: (qmail 88648 invoked by uid 500); 13 Mar 2015 09:26:06 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 88581 invoked by uid 500); 13 Mar 2015 09:26:06 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 88571 invoked by uid 99); 13 Mar 2015 09:26:06 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 13 Mar 2015 09:26:06 +0000 X-ASF-Spam-Status: No, hits=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of ylavic.dev@gmail.com designates 209.85.223.176 as permitted sender) Received: from [209.85.223.176] (HELO mail-ie0-f176.google.com) (209.85.223.176) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 13 Mar 2015 09:26:02 +0000 Received: by iegc3 with SMTP id c3so96381641ieg.3 for ; Fri, 13 Mar 2015 02:24:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:content-transfer-encoding; bh=hai8x+udjEhWMfgLkVLoQDvtEQFVriXfsXRma9/6/qE=; b=LBjCgBmLKb0wbqEW44WEl5nUaCZkSzhE/YIStnQZEPeIJVXukxabHdE9BNald+ODot SVgV6ZIewcttTs/YSrHRqXGEd2+ITPeSxDMnKpXJQ6MBgOB6b+WanUsfyM+FmkVUKR/0 4I66CNxyOqsf7VV2XPQEd9meCaGbcdRBkpDWbd8g+1ut8lv29oMZwIZHe2DgY5pguV73 qVwMznBMam3ADgJk+76j5I4It96x7TnRHxojQMn3xyuTzI3EAC1QBhZN4mWSKrTnFkXi +yzlgZ9RMPetEhwB+vSJE973r4jjgnPoEM5nuyjGGuQdrkV4KwlFmF87I2NqeRfhQHtA oavQ== MIME-Version: 1.0 X-Received: by 10.50.82.68 with SMTP id g4mr107877583igy.26.1426238696661; Fri, 13 Mar 2015 02:24:56 -0700 (PDT) Received: by 10.42.255.7 with HTTP; Fri, 13 Mar 2015 02:24:56 -0700 (PDT) In-Reply-To: References: Date: Fri, 13 Mar 2015 10:24:56 +0100 Message-ID: Subject: Re: apr_skiplist dependency From: Yann Ylavic To: httpd Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Virus-Checked: Checked by ClamAV on apache.org That is: Index: server/mpm/event/event.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D (apr_time_t) (((timer_event_t *) a)->when); + apr_time_t t2 =3D (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 =3D 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D (apr_time_t) (((motorz_timer_t *) a)->expires); + apr_time_t t2 =3D (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 =3D (motorz_conn_t *)baton; @@ -305,7 +314,7 @@ static void motorz_register_timer(motorz_conn_t *s elem->mz =3D 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 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 =3D (apr_time_t) (((timer_event_t *) a)->when); > apr_time_t t2 =3D (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 adde= d 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 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 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 wrote: >>>> I'm +1 on that. >>>> >>>> >>>> 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. >>>> >>>> >>>> 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 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 ti= me 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 gr= eatest. >>>>> >>>>> Looking forward, I see the same thing; apr_skiplist needs design chan= ges to supply what httpd needs, and doing so means extra work in APR to pre= serve compatibility, as well as dependence of Event on a new APR release st= ream. 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 fo= r apr_skiplist itself: for the codebase to evolve naturally to support this= important (primary, only???) consumer, without the constraints of APR vers= ioning 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 inter= face 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 ht= tpd 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 f= or httpd. The APR stable releases can pick up compatible code fixes, but t= hat 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/ >>>>> >>>> >>