Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Date: Wed, 05 Dec 2001 22:51:43 -0800
From: Brian Pane <bpane@pacbell.net>
Subject: Re: Pools rewrite [3]
To: Sander Striker <striker@apache.org>
Cc: dev@apr.apache.org
Message-id: <3C0F157F.3020701@pacbell.net>
MIME-version: 1.0
Content-type: text/plain; charset=us-ascii; format=flowed
Content-transfer-encoding: 7BIT
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.6) Gecko/20011120
References: <JLEGKKNELMHCJPNMOKHOOEHIEPAA.striker@apache.org>

Sander Striker wrote:

> This is the 3rd round of the pools rewrite.  This time I have gotten
> rid of all tabs (grmpf) and followed some suggestions by Justin.
> 
> I'm reposting the whole file instead of a diff, since this is
> the first time it is posted inline.  Hopefully the interested lot
> out there have an easier time looking at it now.


Thanks, here are my comments on the code:
...

>static APR_INLINE void node_free(allocator_t *allocator, node_t *node)
>{
>    node_t *next;
>    apr_uint32_t index, max_index;
>
>    LOCK(allocator->mutex);
>
>    max_index = allocator->max_index;
>
>    /* Walk the list of submitted nodes and free them one by one,
>     * shoving them in the right 'size' buckets as we go.
>     */
>    do {
>        next = node->next;
>        index = node->index;
>
>        if (index < MAX_INDEX) {
>            /* Add the node to the appropiate 'size' bucket.  Adjust
>             * the max_index when appropiate.
>             */
>            if ((node->next = allocator->free[index]) != NULL && index > max_index) {
>                 max_index = index;
>            }
>            allocator->free[index] = node;
>        }
>

I don't understand that "(node->next = allocator->free[index]) != NULL"
clause in the if-statement.  If allocator->free[index] was NULL before
the new node was linked into that bucket, shouldn't max_index still be
updated to be >= index?

...

>APR_DECLARE(void *) apr_palloc(apr_pool_t *pool, apr_size_t size)
>{
>
...

>    /* Reset the active node, get ourselves a new one and activate it. */
>    active->first_avail = (char *)active + SIZEOF_NODE_T;
>

I think it might make debugging easier if we waited until the
active node was freed (at pool destruction) before resetting
its first_avail pointer.  Just in case anybody ever ends up
looking through the contents of the node list in gdb, it would
be less confusing if the first_avail pointers of the previously
active nodes still had meaningful values.  (Maybe it would be
cleanest to just set the first_avail pointer on a node in
node_malloc, right before handing the recycled block to the
caller?)

...

>APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool, 
>                                             apr_pool_t *parent,
>                                             apr_abortfunc_t abort_fn,
>                                             apr_uint32_t flags)
>{
>
...

>        memset(new_allocator, 0, SIZEOF_ALLOCATOR_T);
>        new_allocator->max_index = 0;
>        new_allocator->mutex = NULL;
>        new_allocator->owner = pool;
>        new_allocator->free[0] = NULL;
>

Isn't it redundant to set all these fields to 0 and NULL right
after the memset?

...

>    if ((flags & POOL_FLOCK) == POOL_FLOCK) {
>            if ((rv = apr_thread_mutex_create(&allocator->mutex, 
>

I think all the lock code needs to be wrapped in "#if APR_HAS_THREADS "

--Brian