On Sat, Apr 14, 2018 at 12:54 AM, Nick Kew <n...@apache.org> wrote:
>
>> On 13 Apr 2018, at 21:46, Yann Ylavic <ylavic....@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.
Index: include/apr_reslist.h
===================================================================
--- include/apr_reslist.h (revision 1829106)
+++ include/apr_reslist.h (working copy)
@@ -119,6 +119,27 @@ APR_DECLARE(apr_status_t) apr_reslist_acquire(apr_
void **resource);
/**
+ * Retrieve the newest resource from the list, creating a new one if necessary.
+ * If we have met our maximum number of resources, we will block
+ * until one becomes available.
+ * @param reslist The resource list.
+ * @param resource An address where the pointer to the resource
+ * will be stored.
+ */
+#define apr_reslist_acquire_lifo apr_reslist_acquire
+
+/**
+ * Retrieve the oldest resource from the list, creating a new one if necessary.
+ * If we have met our maximum number of resources, we will block
+ * until one becomes available.
+ * @param reslist The resource list.
+ * @param resource An address where the pointer to the resource
+ * will be stored.
+ */
+APR_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
+ void **resource);
+
+/**
* Return a resource back to the list of available resources.
* @param reslist The resource list.
* @param resource The resource to return to the list.
Index: util-misc/apr_reslist.c
===================================================================
--- util-misc/apr_reslist.c (revision 1829106)
+++ util-misc/apr_reslist.c (working copy)
@@ -60,22 +60,50 @@ struct apr_reslist_t {
#endif
};
+/*
+ * The list of available resources is maintained such that the head is the
+ * newest one, and the tail the oldest. Thus the LIFO (default) and FIFO
+ * accesses grab at the corresponding end of the list.
+ */
+
/**
- * Grab a resource from the front of the resource list.
+ * Peek a resource from the list of available resources.
* Assumes: that the reslist is locked.
*/
-static apr_res_t *pop_resource(apr_reslist_t *reslist)
+static APR_INLINE apr_res_t *peek_resource(apr_reslist_t *reslist, int fifo)
{
- apr_res_t *res;
- res = APR_RING_FIRST(&reslist->avail_list);
+ if (fifo) {
+ return APR_RING_LAST(&reslist->avail_list);
+ }
+ else {
+ return APR_RING_FIRST(&reslist->avail_list);
+ }
+}
+
+/**
+ * Take the resource off the list of available resources.
+ * Assumes: that the reslist is locked.
+ */
+static APR_INLINE void take_resource(apr_reslist_t *reslist, apr_res_t *res)
+{
APR_RING_REMOVE(res, link);
reslist->nidle--;
+}
+
+/**
+ * Pop a resource from the list of available resources.
+ * Assumes: that the reslist is locked.
+ */
+static apr_res_t *pop_resource(apr_reslist_t *reslist, int fifo)
+{
+ apr_res_t *res = peek_resource(reslist, fifo);
+ take_resource(reslist, res);
return res;
}
/**
- * Add a resource to the beginning of the list, set the time at which
- * it was added to the list.
+ * Push a resource to the head of the list of available resources,
+ * setting the time at which it was added (i.e. now).
* Assumes: that the reslist is locked.
*/
static void push_resource(apr_reslist_t *reslist, apr_res_t *resource)
@@ -148,7 +176,7 @@ static apr_status_t reslist_cleanup(void *data_)
while (rl->nidle > 0) {
apr_status_t rv1;
- res = pop_resource(rl);
+ res = pop_resource(rl, 0);
rl->ntotal--;
rv1 = destroy_resource(rl, res);
if (rv1 != APR_SUCCESS) {
@@ -223,7 +251,7 @@ APR_DECLARE(apr_status_t) apr_reslist_maintain(apr
/* 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 */
+ /* Peak at the oldest resource in the list */
res = APR_RING_LAST(&reslist->avail_list);
/* See if the oldest entry should be expired */
if (now - res->freed < reslist->ttl) {
@@ -325,8 +353,8 @@ APR_DECLARE(apr_status_t) apr_reslist_destroy(apr_
return apr_pool_cleanup_run(reslist->pool, reslist, reslist_cleanup);
}
-APR_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
- void **resource)
+static APR_INLINE apr_status_t reslist_acquire(apr_reslist_t *reslist,
+ void **resource, int fifo)
{
apr_status_t rv;
apr_res_t *res;
@@ -340,11 +368,17 @@ APR_DECLARE(apr_status_t) apr_reslist_destroy(apr_
* them right away. */
now = apr_time_now();
while (reslist->nidle > 0) {
- /* Pop off the first resource */
- res = pop_resource(reslist);
+#if 0
+ /* Peek off the oldest resource, more likely to expire and
+ * thus to be recycled here when needed. Otherwise, with a
+ * busy than idle list, the oldest resources could starve.
+ */
+#endif
+ res = peek_resource(reslist, fifo);
if (reslist->ttl && (now - res->freed >= reslist->ttl)) {
/* this res is expired - kill it */
reslist->ntotal--;
+ take_resource(reslist, res);
rv = destroy_resource(reslist, res);
free_container(reslist, res);
if (rv != APR_SUCCESS) {
@@ -355,6 +389,17 @@ APR_DECLARE(apr_status_t) apr_reslist_destroy(apr_
}
continue;
}
+#if 0
+ /* If there is more than one resource available and the user wants
+ * the other end (the newest), restore this one, point to the head
+ * and fall through.
+ */
+ if (!fifo && reslist->nidle > 1) {
+ APR_RING_INSERT_TAIL(&reslist->avail_list, res, apr_res_t, link);
+ res = APR_RING_FIRST(&reslist->avail_list);
+ }
+#endif
+ take_resource(reslist, res);
*resource = res->opaque;
free_container(reslist, res);
#if APR_HAS_THREADS
@@ -383,7 +428,7 @@ APR_DECLARE(apr_status_t) apr_reslist_destroy(apr_
/* If we popped out of the loop, first try to see if there
* are new resources available for immediate use. */
if (reslist->nidle > 0) {
- res = pop_resource(reslist);
+ res = pop_resource(reslist, fifo);
*resource = res->opaque;
free_container(reslist, res);
#if APR_HAS_THREADS
@@ -408,6 +453,18 @@ APR_DECLARE(apr_status_t) apr_reslist_destroy(apr_
}
}
+APR_DECLARE(apr_status_t) apr_reslist_acquire(apr_reslist_t *reslist,
+ void **resource)
+{
+ return reslist_acquire(reslist, resource, 0);
+}
+
+APR_DECLARE(apr_status_t) apr_reslist_acquire_fifo(apr_reslist_t *reslist,
+ void **resource)
+{
+ return reslist_acquire(reslist, resource, 1);
+}
+
APR_DECLARE(apr_status_t) apr_reslist_release(apr_reslist_t *reslist,
void *resource)
{
