> From: Brian Pane [mailto:[EMAIL PROTECTED]
> 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
