Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
From: "Sander Striker" <striker@apache.org>
To: "Brian Pane" <bpane@pacbell.net>
Cc: <dev@apr.apache.org>
Subject: RE: Pools rewrite [3]
Date: Thu, 6 Dec 2001 09:50:25 +0100
Message-ID: <JLEGKKNELMHCJPNMOKHOMEIMEPAA.striker@apache.org>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit
In-Reply-To: <3C0F157F.3020701@pacbell.net>
Importance: Normal

> From: Brian Pane [mailto:bpane@pacbell.net]
> Sent: 06 December 2001 07:52

> 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:
> ....

Thx for the review.
 
> >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?

Good catch!  It should read "(node->next = allocator->free[index]) == NULL".

> ....
> 
> >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?)

Ah, no.  I tried to handle as much as possible of the cleanup in apr_palloc.
This way, apr_pool_clear can be a lot faster than what we have in
the current pools code.  The only thing that needs to be done is reset
the active node instead of resetting all the nodes.

> ....
> 
> >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?

It is.  The free[] array is NULL'd aswell.  What is excessive is
that I am zeroing the max_index, mutex and free[0] fields by hand
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 "

Acked.

> --Brian

Sander