You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2018/04/04 17:43:46 UTC
svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Author: ylavic
Date: Wed Apr 4 17:43:46 2018
New Revision: 1828369
URL: http://svn.apache.org/viewvc?rev=1828369&view=rev
Log:
reslist: follow up to r1828289: adjust maintenance top too.
Also, clarify in doxygen when apr_reslist_fifo_set() should be called.
Modified:
apr/apr/trunk/include/apr_reslist.h
apr/apr/trunk/util-misc/apr_reslist.c
Modified: apr/apr/trunk/include/apr_reslist.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_reslist.h?rev=1828369&r1=1828368&r2=1828369&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_reslist.h (original)
+++ apr/apr/trunk/include/apr_reslist.h Wed Apr 4 17:43:46 2018
@@ -134,11 +134,17 @@ APR_DECLARE(apr_status_t) apr_reslist_re
*/
APR_DECLARE(void) apr_reslist_timeout_set(apr_reslist_t *reslist,
apr_interval_time_t timeout);
+
/**
* Set whether the reslist reuses resources as FIFO (First In First Out) or
* LIFO (Last In First Out).
* @param reslist The resource list.
* @param fifo Set as FIFO (non zero) or LIFO (zero).
+ * @remark This function should be called before any resource is in the
+ * the reslist, otherwise maintenance optimizations based on the expiration
+ * time relative to the order of insertion (i.e. position in the list) won't
+ * work as expected.
+ *
*/
APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo);
Modified: apr/apr/trunk/util-misc/apr_reslist.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_reslist.c?rev=1828369&r1=1828368&r2=1828369&view=diff
==============================================================================
--- apr/apr/trunk/util-misc/apr_reslist.c (original)
+++ apr/apr/trunk/util-misc/apr_reslist.c Wed Apr 4 17:43:46 2018
@@ -229,8 +229,13 @@ APR_DECLARE(apr_status_t) apr_reslist_ma
/* Check if we need to expire old resources */
now = apr_time_now();
while (reslist->nidle > reslist->smax && reslist->nidle > 0) {
- /* Peak at the last resource in the list */
- res = APR_RING_LAST(&reslist->avail_list);
+ /* Peek at the next expiring resource in the list */
+ if (reslist->fifo) {
+ res = APR_RING_FIRST(&reslist->avail_list);
+ }
+ else {
+ res = APR_RING_LAST(&reslist->avail_list);
+ }
/* See if the oldest entry should be expired */
if (now - res->freed < reslist->ttl) {
/* If this entry is too young, none of the others
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 6, 2018 at 10:18 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Apr 6, 2018 at 8:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> I don't think that a remark is strong enough here. I would like to
>> see this parameter either added to a new apr_reslist_create_ex or if
>> we want to stick with apr_reslist_fifo_set it should return an error
>> if elements are already in the reslist.
>
> r1828492, I kept it as a helper rather than _create_ex() because:
> - it's already how settings seem to be added there (timeout, ...)
> - _create() has a substantial number of args already :/
> - _create_ex() are definitive extensions or may end up in endless
> discussions for the next function name when/should an _ex_ex() is/be
> needed :)
Also, the lock is not acquired so the empty test is not that accurate
when multi-threaded.
I'd prefer to keep it light since we document the expected usage in
the first place, and I wouldn't want to penalize respectful users...
>
> Thanks for the review!
>
> Regards,
> Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Eric Covener <co...@gmail.com>.
On Mon, Apr 16, 2018 at 6:04 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 04/14/2018 02:32 AM, Yann Ylavic wrote:
>>>
>>> IOW, this simple patch would work equally for me (and could go in any version):
>>>
>>> Index: util-misc/apr_reslist.c
>>> ===================================================================
>>> --- util-misc/apr_reslist.c (revision 1829106)
>>> +++ util-misc/apr_reslist.c (working copy)
>>> @@ -61,13 +61,13 @@ struct apr_reslist_t {
>>> };
>>>
>>> /**
>>> - * Grab a resource from the front of the resource list.
>>> + * Grab a resource from the back of the resource list.
>>> * Assumes: that the reslist is locked.
>>> */
>>> static apr_res_t *pop_resource(apr_reslist_t *reslist)
>>> {
>>> apr_res_t *res;
>>> - res = APR_RING_FIRST(&reslist->avail_list);
>>> + res = APR_RING_LAST(&reslist->avail_list);
>>> APR_RING_REMOVE(res, link);
>>> reslist->nidle--;
>>> return res;
>>> --
>>
>> Hm, but this would change this to become a fifo list instead of the current lifo list or do I miss something?
>
> Yes clearly, I suggested that reslists be changed to FIFO
> unconditionnaly, because I find that LIFO and expiry don't mix well
> w.r.t. starvation..
>
> That would be the simpler patch too (not that
> apr_reslist_acquire_fifo() is hard to implement, but I wonder who
> needs LIFO in the firtst place with such a structure...).
>
Looks like it could cause some subtle/sneaky system behavior changes.
IIUC consider mod_dbd which may even have functional issues.
How many times should a caller call ap_dbd_open()? 1, 3, or until it
hopefully stops returning NULL?
OTOH I do agree that FIFO looks a lot more sensible as a default, but
I think that ship has sailed.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Apr 16, 2018, at 7:22 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> I think there are scenarios where LIFO is useful, but the question is if these are frequent enough to warrant LIFO /
> FIFO as an option.
>
As an option, yes. But not, in my opinion, a behind-the-curtains change
which will likely affect users in several not-known ways.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/16/2018 12:04 PM, Yann Ylavic wrote:
> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 04/14/2018 02:32 AM, Yann Ylavic wrote:
>>>
>>> IOW, this simple patch would work equally for me (and could go in any version):
>>>
>>> Index: util-misc/apr_reslist.c
>>> ===================================================================
>>> --- util-misc/apr_reslist.c (revision 1829106)
>>> +++ util-misc/apr_reslist.c (working copy)
>>> @@ -61,13 +61,13 @@ struct apr_reslist_t {
>>> };
>>>
>>> /**
>>> - * Grab a resource from the front of the resource list.
>>> + * Grab a resource from the back of the resource list.
>>> * Assumes: that the reslist is locked.
>>> */
>>> static apr_res_t *pop_resource(apr_reslist_t *reslist)
>>> {
>>> apr_res_t *res;
>>> - res = APR_RING_FIRST(&reslist->avail_list);
>>> + res = APR_RING_LAST(&reslist->avail_list);
>>> APR_RING_REMOVE(res, link);
>>> reslist->nidle--;
>>> return res;
>>> --
>>
>> Hm, but this would change this to become a fifo list instead of the current lifo list or do I miss something?
>
> Yes clearly, I suggested that reslists be changed to FIFO
> unconditionnaly, because I find that LIFO and expiry don't mix well
> w.r.t. starvation..
>
> That would be the simpler patch too (not that
> apr_reslist_acquire_fifo() is hard to implement, but I wonder who
I think there are scenarios where LIFO is useful, but the question is if these are frequent enough to warrant LIFO /
FIFO as an option.
> needs LIFO in the firtst place with such a structure...).
There seems to be no promise in the API documentation that this is LIFO, but of course switching to FIFO changes the
behavior of the structure for the better (as you assume) or for the worse (for people who build on the current
implementation detail that it is LIFO).
So the question is: Can we do that in 1.6.x or even in 1.7.x from our guarantees we give point of view?
Regards
Rüdiger
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 04/14/2018 02:32 AM, Yann Ylavic wrote:
>>
>> IOW, this simple patch would work equally for me (and could go in any version):
>>
>> Index: util-misc/apr_reslist.c
>> ===================================================================
>> --- util-misc/apr_reslist.c (revision 1829106)
>> +++ util-misc/apr_reslist.c (working copy)
>> @@ -61,13 +61,13 @@ struct apr_reslist_t {
>> };
>>
>> /**
>> - * Grab a resource from the front of the resource list.
>> + * Grab a resource from the back of the resource list.
>> * Assumes: that the reslist is locked.
>> */
>> static apr_res_t *pop_resource(apr_reslist_t *reslist)
>> {
>> apr_res_t *res;
>> - res = APR_RING_FIRST(&reslist->avail_list);
>> + res = APR_RING_LAST(&reslist->avail_list);
>> APR_RING_REMOVE(res, link);
>> reslist->nidle--;
>> return res;
>> --
>
> Hm, but this would change this to become a fifo list instead of the current lifo list or do I miss something?
Yes clearly, I suggested that reslists be changed to FIFO
unconditionnaly, because I find that LIFO and expiry don't mix well
w.r.t. starvation..
That would be the simpler patch too (not that
apr_reslist_acquire_fifo() is hard to implement, but I wonder who
needs LIFO in the firtst place with such a structure...).
Regards,
Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/14/2018 02:32 AM, Yann Ylavic wrote:
> On Sat, Apr 14, 2018 at 2:18 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> when the ttl is to be
>> checked against the resource we should always peek it as LIFO (i.e.
>> s/fifo/1/ in the first peek_resource() of reslist_acquire() in my
>> patch).
>> This would prevent starvation, and we should possibly do that in 1.6.x
>> too (where apr_reslist_acquire_fifo() can't land).
>> If we do that unconditionnaly, this patch is moot. After all,
>> apr_reslist_maintain() and/hence apr_reslist_release() are already
>> LIFO for recycling resources.
>
> IOW, this simple patch would work equally for me (and could go in any version):
>
> Index: util-misc/apr_reslist.c
> ===================================================================
> --- util-misc/apr_reslist.c (revision 1829106)
> +++ util-misc/apr_reslist.c (working copy)
> @@ -61,13 +61,13 @@ struct apr_reslist_t {
> };
>
> /**
> - * Grab a resource from the front of the resource list.
> + * Grab a resource from the back of the resource list.
> * Assumes: that the reslist is locked.
> */
> static apr_res_t *pop_resource(apr_reslist_t *reslist)
> {
> apr_res_t *res;
> - res = APR_RING_FIRST(&reslist->avail_list);
> + res = APR_RING_LAST(&reslist->avail_list);
> APR_RING_REMOVE(res, link);
> reslist->nidle--;
> return res;
> --
Hm, but this would change this to become a fifo list instead of the current lifo list or do I miss something?
Regards
Rüdiger
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Apr 14, 2018 at 2:18 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> when the ttl is to be
> checked against the resource we should always peek it as LIFO (i.e.
> s/fifo/1/ in the first peek_resource() of reslist_acquire() in my
> patch).
> This would prevent starvation, and we should possibly do that in 1.6.x
> too (where apr_reslist_acquire_fifo() can't land).
> If we do that unconditionnaly, this patch is moot. After all,
> apr_reslist_maintain() and/hence apr_reslist_release() are already
> LIFO for recycling resources.
IOW, this simple patch would work equally for me (and could go in any version):
Index: util-misc/apr_reslist.c
===================================================================
--- util-misc/apr_reslist.c (revision 1829106)
+++ util-misc/apr_reslist.c (working copy)
@@ -61,13 +61,13 @@ struct apr_reslist_t {
};
/**
- * Grab a resource from the front of the resource list.
+ * Grab a resource from the back of the resource list.
* Assumes: that the reslist is locked.
*/
static apr_res_t *pop_resource(apr_reslist_t *reslist)
{
apr_res_t *res;
- res = APR_RING_FIRST(&reslist->avail_list);
+ res = APR_RING_LAST(&reslist->avail_list);
APR_RING_REMOVE(res, link);
reslist->nidle--;
return res;
--
Except that for some reason it doesn't pass test_shrinking() in the
tests suite (neither is my patch).
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
To expand a bit...
On Sat, Apr 14, 2018 at 1:46 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Apr 14, 2018 at 12:54 AM, Nick Kew <ni...@apache.org> wrote:
>>
>>> On 13 Apr 2018, at 21:46, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> On Fri, Apr 13, 2018 at 7:30 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>>> I'm still unclear why you believe we need APR to purge the elts when the
>>>> user chooses to flip the fifo/lifo switch. That seems very wrong to me, the
>>>> consumer can do so if that is what they desire.
>>>
>>> To be clear (with myself):
>>>
>>> /* What we have */
>>> APU_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
>>> void **resource);
>>> /* What we want */
>>> #define apr_reslist_acquire_lifo apr_reslist_acquire
>>> APU_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
>>> void **resource);
>>
>> You have a plan to implement that with the existing structs?
>
> I'm almost there yes, see attached patch (no tested yet..).
The #if 0s there should always be 1s IMHO, and when the ttl is to be
checked against the resource we should always peek it as LIFO (i.e.
s/fifo/1/ in the first peek_resource() of reslist_acquire() in my
patch).
This would prevent starvation, and we should possibly do that in 1.6.x
too (where apr_reslist_acquire_fifo() can't land).
If we do that unconditionnaly, this patch is moot. After all,
apr_reslist_maintain() and/hence apr_reslist_release() are already
LIFO for recycling resources.
(Note that there is a bug in apr_reslist_maintain() which doesn't
check if ttl != 0 before expiring resources, hence kills everything
between smax and hmax. I have a unit test and plan to commit a fix
later).
>
>> How does it work if an application mixes the two forms of
>> acquire randomly, as seems likely if (for example) a reslist is
>> shared between applications/components of differing provenances?
If finally we need this patch and apr_reslist_acquire_fifo() gets used
arbitrarily/concurrently with the non-fifo version, we'd still recycle
resources appropriately in order, so I see no issue. Anything besides
the performance concern (which isn't one I think)?
Regards,
Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Apr 14, 2018 at 12:54 AM, Nick Kew <ni...@apache.org> wrote:
>
>> On 13 Apr 2018, at 21:46, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Fri, Apr 13, 2018 at 7:30 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>> I'm still unclear why you believe we need APR to purge the elts when the
>>> user chooses to flip the fifo/lifo switch. That seems very wrong to me, the
>>> consumer can do so if that is what they desire.
>>
>> To be clear (with myself):
>>
>> /* What we have */
>> APU_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
>> void **resource);
>> /* What we want */
>> #define apr_reslist_acquire_lifo apr_reslist_acquire
>> APU_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
>> void **resource);
>
> You have a plan to implement that with the existing structs?
I'm almost there yes, see attached patch (no tested yet..).
> How does it work if an application mixes the two forms of
> acquire randomly, as seems likely if (for example) a reslist is
> shared between applications/components of differing provenances?
> I’d be reluctant to see a significant performance hit.
Not performance hit expected, whether acquire return the first or last
entry in the should be costless.
>
> I’m also thinking, with fifo the reslist might tend to grow bigger,
> as more elements are re-used well within the timeout and thus
> never be deleted to reduce the size. Isn’t that an argument for
> making reslist an either/or thing with policy determined once
> and for all when it’s created?
Actually this is what happens with current LIFO, most recent resources
are mainly reused and oldest ones can even starve when busyness
decreases. With FIFO, release() will always check ttl against oldest
entries first so it's more likely to expire them.
Regards,
Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Nick Kew <ni...@apache.org>.
> On 13 Apr 2018, at 21:46, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Apr 13, 2018 at 7:30 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>> I'm still unclear why you believe we need APR to purge the elts when the
>> user chooses to flip the fifo/lifo switch. That seems very wrong to me, the
>> consumer can do so if that is what they desire.
>
> To be clear (with myself):
>
> /* What we have */
> APU_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
> void **resource);
> /* What we want */
> #define apr_reslist_acquire_lifo apr_reslist_acquire
> APU_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
> void **resource);
You have a plan to implement that with the existing structs?
How does it work if an application mixes the two forms of
acquire randomly, as seems likely if (for example) a reslist is
shared between applications/components of differing provenances?
I’d be reluctant to see a significant performance hit.
I’m also thinking, with fifo the reslist might tend to grow bigger,
as more elements are re-used well within the timeout and thus
never be deleted to reduce the size. Isn’t that an argument for
making reslist an either/or thing with policy determined once
and for all when it’s created?
—
Nick Kew
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 13, 2018 at 7:30 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> I'm still unclear why you believe we need APR to purge the elts when the
> user chooses to flip the fifo/lifo switch. That seems very wrong to me, the
> consumer can do so if that is what they desire.
To be clear (with myself):
/* What we have */
APU_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
void **resource);
/* What we want */
#define apr_reslist_acquire_lifo apr_reslist_acquire
APU_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
void **resource);
And all we have to do is implement the latter.
Of course you are right, how much simpler! No fifo setter (or
_create_ex) for the whole reslist lifetime, no headache on what to do
when it shouldn't be called, no nothing, and much more user friendly.
When the user wants the head of the list, we return it, likewise for
the tail, with the exact same expiry checks on the oldest resources
(we know where they are internally) or min ones to create. This is
just about which end we finally return...
I've been ridiculously long to get your point, but I do now I think, right?
Thanks Bill, will revert and restart from there!
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 12, 2018 at 11:13 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Thu, Apr 12, 2018 at 12:56 PM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>> Other options:
>>
>> 2. Reverse the order in the ring when apr_reslist_fifo_set is called.
>>
>> Of course both options require a lock.
>
> Here's an odd idea. What if the *traversal* were reversed based on the lifo bit,
> and the structure itself remained static?
>
> 3. Interpret the fifo/lifo flag as an insertion mechanic, so previous fifo
> items are returned in original sequence, once all later lifo items are
> retrieved.
Oh, I missed your message before replying to Rüdiger.
I'm afraid it'd be a bit over-engineered for a setting unlikely to
change in the lifetime of the reslist, or do you foresee a real need
for this?
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, Apr 12, 2018 at 12:56 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> Other options:
>
> 2. Reverse the order in the ring when apr_reslist_fifo_set is called.
>
> Of course both options require a lock.
Here's an odd idea. What if the *traversal* were reversed based on the lifo bit,
and the structure itself remained static?
3. Interpret the fifo/lifo flag as an insertion mechanic, so previous fifo
items are returned in original sequence, once all later lifo items are
retrieved.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Apr 12, 2018 at 7:56 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> Other options:
>
> 1. Destroy all reslist elements when apr_reslist_fifo_set is called.
> 2. Reverse the order in the ring when apr_reslist_fifo_set is called.
Good idea, I'll go for 1. unless someone beats me at it with 2. :)
>
> Of course both options require a lock.
OK, you convinced me ;)
Thanks!
Regards,
Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/11/2018 01:26 PM, Yann Ylavic wrote:
> On Fri, Apr 6, 2018 at 10:18 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Fri, Apr 6, 2018 at 8:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>> I don't think that a remark is strong enough here. I would like to
>>> see this parameter either added to a new apr_reslist_create_ex or if
>>> we want to stick with apr_reslist_fifo_set it should return an error
>>> if elements are already in the reslist.
>>
>> r1828492, I kept it as a helper rather than _create_ex() because:
>> - it's already how settings seem to be added there (timeout, ...)
>> - _create() has a substantial number of args already :/
>> - _create_ex() are definitive extensions or may end up in endless
>> discussions for the next function name when/should an _ex_ex() is/be
>> needed :)
>
> Actually it's not that easy.
> While writing the unit test for apr_reslist_fifo_set(), I realized
> that if min > 0 is configured the reslist is immediately filled in
> upon creation, so there is no way to call apr_reslist_fifo_set()
> afterward...
>
> This brings us to either apr_reslist_create_ex() as you suggested, or
> disable the non-empty check in apr_reslist_fifo_set() as the original
> commit (and rely on the user to read docs...).
Other options:
1. Destroy all reslist elements when apr_reslist_fifo_set is called.
2. Reverse the order in the ring when apr_reslist_fifo_set is called.
Of course both options require a lock.
> Setting fifo just after create() with min > 0 is not an issue per se,
> it'll really matter at acquire time anyway...
I guess this is true.
> So I tend to prefer the documentation only over the _ex() (potential
> _ex++() later...), but I could live the the latter too :)
>
> Thoughts?
>
Regards
Rüdiger
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 6, 2018 at 10:18 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Apr 6, 2018 at 8:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> I don't think that a remark is strong enough here. I would like to
>> see this parameter either added to a new apr_reslist_create_ex or if
>> we want to stick with apr_reslist_fifo_set it should return an error
>> if elements are already in the reslist.
>
> r1828492, I kept it as a helper rather than _create_ex() because:
> - it's already how settings seem to be added there (timeout, ...)
> - _create() has a substantial number of args already :/
> - _create_ex() are definitive extensions or may end up in endless
> discussions for the next function name when/should an _ex_ex() is/be
> needed :)
Actually it's not that easy.
While writing the unit test for apr_reslist_fifo_set(), I realized
that if min > 0 is configured the reslist is immediately filled in
upon creation, so there is no way to call apr_reslist_fifo_set()
afterward...
This brings us to either apr_reslist_create_ex() as you suggested, or
disable the non-empty check in apr_reslist_fifo_set() as the original
commit (and rely on the user to read docs...).
Setting fifo just after create() with min > 0 is not an issue per se,
it'll really matter at acquire time anyway...
So I tend to prefer the documentation only over the _ex() (potential
_ex++() later...), but I could live the the latter too :)
Thoughts?
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 6, 2018 at 8:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> I don't think that a remark is strong enough here. I would like to
> see this parameter either added to a new apr_reslist_create_ex or if
> we want to stick with apr_reslist_fifo_set it should return an error
> if elements are already in the reslist.
r1828492, I kept it as a helper rather than _create_ex() because:
- it's already how settings seem to be added there (timeout, ...)
- _create() has a substantial number of args already :/
- _create_ex() are definitive extensions or may end up in endless
discussions for the next function name when/should an _ex_ex() is/be
needed :)
Thanks for the review!
Regards,
Yann.
Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/04/2018 07:43 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Wed Apr 4 17:43:46 2018
> New Revision: 1828369
>
> URL: http://svn.apache.org/viewvc?rev=1828369&view=rev
> Log:
> reslist: follow up to r1828289: adjust maintenance top too.
>
> Also, clarify in doxygen when apr_reslist_fifo_set() should be called.
>
>
> Modified:
> apr/apr/trunk/include/apr_reslist.h
> apr/apr/trunk/util-misc/apr_reslist.c
>
> Modified: apr/apr/trunk/include/apr_reslist.h
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_reslist.h?rev=1828369&r1=1828368&r2=1828369&view=diff
> ==============================================================================
> --- apr/apr/trunk/include/apr_reslist.h (original)
> +++ apr/apr/trunk/include/apr_reslist.h Wed Apr 4 17:43:46 2018
> @@ -134,11 +134,17 @@ APR_DECLARE(apr_status_t) apr_reslist_re
> */
> APR_DECLARE(void) apr_reslist_timeout_set(apr_reslist_t *reslist,
> apr_interval_time_t timeout);
> +
> /**
> * Set whether the reslist reuses resources as FIFO (First In First Out) or
> * LIFO (Last In First Out).
> * @param reslist The resource list.
> * @param fifo Set as FIFO (non zero) or LIFO (zero).
> + * @remark This function should be called before any resource is in the
> + * the reslist, otherwise maintenance optimizations based on the expiration
> + * time relative to the order of insertion (i.e. position in the list) won't
> + * work as expected.
> + *
I don't think that a remark is strong enough here. I would like to see this parameter either added to a new
apr_reslist_create_ex or if we want to stick with apr_reslist_fifo_set it should return an error if elements
are already in the reslist.
Regards
Rüdiger