Re:: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[William A Rowe Jr]

On Mon, Apr 16, 2018 at 6:22 AM, Ruediger Pluem  wrote:
>
> On 04/16/2018 12:04 PM, Yann Ylavic wrote:
>> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem 
wrote:
>>>
>>> On 04/14/2018 02:32 AM, Yann Ylavic 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.
>
>> 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?

Given that code behavior itself is documentation, and this is hardly what
we can call an 'edge case' (it's the actually intended functionality)...

I'd suggest we can't modify this unilaterally until 2.0.

If there is a desire to change this for library consumers prior to 2.0,
perhaps a build-time toggle? Provided there is no interest in offering
two behaviors simultaneously.

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Jim Jagielski]

> On Apr 16, 2018, at 7:22 AM, Ruediger Pluem  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

[Ruediger Pluem]

On 04/16/2018 12:04 PM, Yann Ylavic wrote:
> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem  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(>avail_list);
>>> +res = APR_RING_LAST(>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

[Eric Covener]

On Mon, Apr 16, 2018 at 6:04 AM, Yann Ylavic  wrote:
> On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem  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(>avail_list);
>>> +res = APR_RING_LAST(>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
cove...@gmail.com

Re: svn commit: r1828369 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Yann Ylavic]

On Mon, Apr 16, 2018 at 8:45 AM, Ruediger Pluem  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(>avail_list);
>> +res = APR_RING_LAST(>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

[Ruediger Pluem]

On 04/14/2018 02:32 AM, Yann Ylavic wrote:
> On Sat, Apr 14, 2018 at 2:18 AM, Yann Ylavic  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(>avail_list);
> +res = APR_RING_LAST(>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