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
