apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@ebuilt.com>
Subject Re: Pools rewrite [2]
Date Wed, 05 Dec 2001 02:56:11 GMT
On Fri, Nov 30, 2001 at 06:27:14PM +0100, Sander Striker wrote:
> Hi,
> 
> This is my second go at the pools code.
> I incorporated some suggestions made by Brian Pane.
> 
> Please review,

General comments:
1) Lose the tabs.
2) It probably needs to be modified to use the new locking API.
   I don't see this as a showstopper, but I think it's something
   that should happen and may help with the locking performance.
   Aaron can speak more about this...

Rest of my comments are inline (denoted by <comment>).  (Are you 
using Outlook?  If not, please inline your patches...)

My comments aren't really about the core logic though.  I know I 
reviewed the overall logic and found it correct before.  I'm going 
to assume that the core memory logic hasn't changed.  And, Brian 
and Ian's testing prove that it works in practice, so I'll look at 
the logic again when I have more time.  -- justin

------
<snip, snip>
/*
 * Magic numbers
 */

#define MIN_ALLOC 8192
#define MAX_INDEX   16

#define BOUNDARY_INDEX 12
#define BOUNDARY_SIZE (1 << BOUNDARY_INDEX)

<comment>
I think I mentioned it before, but it'd probably be good to 
indicate rationale why we think these values are good.  I'm
not disagreeing with them, but I think it might be helpful to
explain how they fit in the overall scheme of things.

And, if this goes in, I'll probably do the same thing I did
with SMS and make a pass that adds source code comments.  That
won't happen until next week at the earliest.
</comment>

/*
 * Macros and defines
 */

/* ALIGN() is only to be used to align on a power of 2 boundary */
#define ALIGN(size, boundary) \
    (((size) + ((boundary) - 1)) & ~((boundary) - 1))

#define ALIGN_DEFAULT(size) ALIGN(size, 8)

#define LOCK(mutex) \
    if (mutex) \
        apr_thread_mutex_lock(mutex);

#define UNLOCK(mutex) \
    if (mutex) \
        apr_thread_mutex_unlock(mutex);

<comment>
Ack.  No.  Declare a new scope like so:
#define UNLOCK(mutex) \
  do { \
    if (mutex) \
        apr_thread_mutex_unlock(mutex); \
  } while(0)

Calling it like UNLOCK(mutex) (i.e. no semicolon) is just badness
and will shoot us down the road later.
</comment>

<snip, snip>
/* apr_pcalloc is almost exactly the same as apr_palloc, except for
 * a few memset()s.  This saves an extra function call though, which
 * is enough justification for this code duplication IMO.
 */

<comment>
On a thought from Dean Gaudet, how would performance be helped if
we #define'd apr_pcalloc to be:
#define apr_pcalloc(pool, size) memset(apr_palloc(pool, size), '\0', size);

The key idea is that Dean pointed out that the compiler can do a
*much* better job of optimizing if we don't stick the memset deep
down in a function somewhere.  I'd be curious to see how this
optimization would help.  And, I think code duplication is evil
anyway.  This has the potential to be faster than apr_pcalloc and 
use the same code.
</comment>

APR_DECLARE(void *) apr_pcalloc(apr_pool_t *pool, apr_size_t size)
{

<snip, snip>

/*
 * Initialization
 */

APR_DECLARE(apr_status_t) apr_pool_alloc_init(apr_pool_t *global)
{
    apr_status_t rv;

    if (global_allocator_initialized)
        return APR_SUCCESS; /* Is this correct? */

<comment>
Sure.  apr_thread_once is probably a much better way to handle this
though I think.
</comment>
    
    memset(&global_allocator, 0, SIZEOF_ALLOCATOR_T);
    
    if ((rv = apr_thread_mutex_create(&global_allocator.mutex, 
                  APR_THREAD_MUTEX_DEFAULT, global)) != APR_SUCCESS) {
        return rv;
    }

    global_allocator.owner = global;
    global_pool = global;
    global_allocator_initialized = 1;

    return APR_SUCCESS;
}

<snip, snip>


Mime
View raw message