apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sander Striker" <stri...@apache.org>
Subject RE: Pools rewrite [2]
Date Wed, 05 Dec 2001 11:35:00 GMT
> From: Justin Erenkrantz [mailto:jerenkrantz@ebuilt.com]
> Sent: 05 December 2001 03:56
> 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.

Damn, I really need to find a way for me not to hit that
tab key.  Sorry about that, force of habbit that is a)
hard to change (working on it though) b) doesn't easily
show on (my) screen.

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

It already does per Brians suggestion after my first post.

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

I'm using LookOut :).  I'm going to inline and attach (as an experiment).
There is an option in outlook that lets it wrap lines at col 132 instead
of the default 76 or 78.  For the interested: 
Tools -> Options -> Mail Format -> Settings (Message Format).

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

Not much anyway AFAIK, it's been a while :)

> 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

Ack.

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

Right, ok.  The 8192 was a number used in the original pools code,
I just ripped it :).  The BOUNDARY_SIZE is set to be 4096, which
is the size of a page on most systems.
 
> /*
>  * 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>

Ok, will change that.

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

When out of mem, this will segfault at the point where the
apr_pcalloc macro is used.

Secondly, the size is aligned to the next multiple of 8 bytes within
apr_pcalloc, this to the advantage of memset.  I wonder if taking
the memset out of the function will improve performance.  I personally
doubt it.  Dean, care to enlighten me?

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

My point was if it was correct to return APR_SUCCESS.  People are
initializing twice if they reach that line of code.

Isn't apr_thread_once restricted to platforms where we actually have
threads?  Hmmm, I now start to wonder about my apr_thread_mutex_xxx
calls.  Aaron?
     
>     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>

APR_DECLARE(void) apr_pool_alloc_term(apr_pool_t *pool)
{
    /*
     * XXX: What happens when global_pool != pool?
     * IMHO apr_pool_alloc_term should take void, not an
     * apr_pool_t *.
     */

This one bothered me personally.  The current pools implementation
assumes that pool == permanent_pool IIRC.  Could apr_pool_alloc_term()
be changed to just take void?  [lousy wording, hope you know what I
mean].

Sander



Mime
View raw message