You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <je...@ebuilt.com> on 2001/08/25 08:04:38 UTC
Review of possible replacement for pools
Enclosed is my review of your posted patch. Actually, I probably would
like to see Brian's patch again to compare it as I think they achieve
similar goals.
The important aspect of this patch is that it makes the pool code
easier to understand. I'll say that this would need comments.
I'll volunteer to add comments that if this patch is accepted.
Here goes - my comments are prefaced with a > at the beginning of
the line (the reverse of normal as I'm too lazy to insert > at the
beginning of the source)...
-------------
#define MIN_ALLOC 8192
#define MAX_INDEX 32
#define BOUNDARY_INDEX 12
#define BOUNDARY_SIZE (1 << BOUNDARY_INDEX)
> Okay so that people have a clue what is going on here, Sander has
> a freelist that is arranged by size. It is based on the power of
> 2 that it is closest to, IIRC. The 0 element is reserved for items
> bigger than 2^32. Sander can correct me if I'm misremembering this.
#define ALIGN_DEFAULT(size) ALIGN(size, 8)
> Why 8 as the default?
static APR_INLINE node_t *node_malloc(allocator_t *allocator, apr_size_t size)
{
> ...blah, blah, blah...search for it on the indexed freelist and
> return it...if none available, call malloc...at this point, we are
> setting up the newly allocated node.
node->next = NULL;
node->index = index;
node->first_avail = (char *)node + SIZEOF_NODE_T;
node->endp = (char *)node + size;
> Problem A. You are adding the SIZEOF_NODE_T here that is implicitly
> included in the size parameter (most calls to node_malloc has size +
> SIZEOF_NODE_T). Either move this portion out (not sure how you
> would do this), or have node_malloc add the space required for the
> node_t and don't call node_malloc with + SIZEOF_NODE_T. However, that
> strategy does present a problem in apr_pool_create_ex where you call
> node_malloc with MIN_ALLOC as the size rather than
> MIN_ALLOC+SIZEOF_NODE_T. So, in order to truly get MIN_ALLOC, you'd
> have to ask for MIN_ALLOC-SIZEOF_NODE_T. Hmm. But, I don't like
> this strategy where you *must* pass in an extra SIZEOF_NODE_T bytes.
> (Yes, this function isn't user-callable, but still...)
static void node_free(allocator_t *allocator, node_t *node)
{
node_t *next;
apr_uint32_t index, max_index;
if (allocator->lock)
apr_lock_acquire(allocator->lock);
> This reminds me that we should have something like
> apr_lock_acquire_opt(foo) which will not do anything if the passed in
> parameter is NULL. This would seriously cut down on the number of
> places where this if (lock) lock_acquire syntax occurrs. Someone
> brought this up in a commit log recently. It's a good point. Hell,
> even a #defined macro works well for this.
APR_DECLARE(void *) apr_palloc(apr_pool_t *pool, apr_size_t size)
{
node_t *active, *node;
void *mem;
char *endp;
size = ALIGN_DEFAULT(size);
active = pool->mactive;
endp = active->first_avail + size;
if (endp < active->endp) {
mem = active->first_avail;
active->first_avail = endp;
return mem;
}
> I think this is a race condition. This is also present in the current
> pool code, so this isn't something that you introduced. Consider the
> following scenario:
> Thread A has access to pool Z.
> Thread B has access to pool Z as well.
> Thread A calls apr_palloc, gets active, sees that there is enough
> space, sets mem.
> *CONTEXT SWITCH*
> Thread B now calls apr_palloc, gets active, sess that there is enough
> space, sets mem, updates first_avail, returns.
> *CONTEXT SWITCH*
> Thread A updates first_avail. Oops. =-) However, I'd suggest an
> option in apr_create_pool_ex that says that this pool is NOT shared
> and doesn't need locking. Otherwise, we must lock this case (not
> an allocator lock, but a pool lock). This seems to be the only case
> where this could happen. Seems a waste, but it definitely could happen.
> This race doesn't matter in the apr_pool_clear or apr_pool_destroy
> since two children don't try to clear the pool at the same time.
>
> I know the code doesn't crash under high load, so I know it has
> a reasonable degree of quality and it is definitely easier to
> understand. Assuming the matters brought up above are addressed
> to my satisfaction, I'll give this a +1. -- justin
>
> P.S. Oh, man. This week's fun never ends. My mail server that
> I receive email on had a full /var partition. What is going on
> this week? Well, I'm slowly getting queued-up emails now...I
> thought the list was awfully quiet.
Re: Review of possible replacement for pools
Posted by Brian Pane <bp...@pacbell.net>.
Bill Stoddard wrote:
>Why are we spending time trying to optimize pools when we haven't eliminated the
>malloc/frees in the bucket brigade calls? The miniscule performance improvements
>you -might- get optimizing pools will be completely obscured by the overhead of the
>malloc/frees.
>
I'm skeptical of this conclusion. Ian's and Justin's experimental results
show measurable improvements in CPU usage and throughput from modified pool
code. Fixing the malloc/free overhead in brigades will be a big win on
systems with a slow malloc, but the benchmark numbers so far indicate that
it need not be in the critical path for fixing the pool scalability.
IMHO, fixing the mutex contention in pools sooner, rather than later, is
also
a good idea in general because it removes an architectural bottleneck
that can
hide the effects of performance tuning in other areas.
--Brian
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> > > Why are we spending time trying to optimize pools when we haven't
> > > eliminated the
> > > malloc/frees in the bucket brigade calls? The miniscule
> > > performance improvements
> > > you -might- get optimizing pools will be completely obscured by
> > > the overhead of the
> > > malloc/frees.
> > >
> > > Bill
> >
> > Because it also eliminates the locking in the threaded mpm.
> > And, I wanted to see if I could squeeze some extra performance
> > out of it and make it a bit more readable. I don't know if
> > I succeeded in the latter. Or the performance for that matter :)
> >
> > Btw, why couldn't we use pools in brigades again?
> >
>
> Brigades need to be maintained across multiple requests to handle
> http pipelined
> responses, so allocating them out of a request pool clearly will
> not work. You could
> allocate the brigades out of a connection pool, but connections
> can remain active for
> 100's of requests so you could leak a lot of storage for the
> duration of the connection.
Wait, couldn't you create a new pool for each brigade, allocate
the brigade out of that pool, associate that pool with the brigade?
Then, when the brigade needs to go, just destroy that pool.
What am I missing?
> Bill
Re: Review of possible replacement for pools
Posted by Bill Stoddard <bi...@wstoddard.com>.
> > Why are we spending time trying to optimize pools when we haven't
> > eliminated the
> > malloc/frees in the bucket brigade calls? The miniscule
> > performance improvements
> > you -might- get optimizing pools will be completely obscured by
> > the overhead of the
> > malloc/frees.
> >
> > Bill
>
> Because it also eliminates the locking in the threaded mpm.
> And, I wanted to see if I could squeeze some extra performance
> out of it and make it a bit more readable. I don't know if
> I succeeded in the latter. Or the performance for that matter :)
>
> Btw, why couldn't we use pools in brigades again?
>
Brigades need to be maintained across multiple requests to handle http pipelined
responses, so allocating them out of a request pool clearly will not work. You could
allocate the brigades out of a connection pool, but connections can remain active for
100's of requests so you could leak a lot of storage for the duration of the connection.
Bill
Re: pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Bill Stoddard" <bi...@wstoddard.com>
Sent: Saturday, August 25, 2001 1:40 PM
> > I have a problem with the original question though. I don't understand why
> > anybody is trying to state that if people want to improve the pools code, they
> > shouldn't, because we use malloc in one place in the code. People are
> > free to work on whatever they want.
>
> I am not being philosophical. Sure, people can spend their time doing whatever they want.
> I assume open source developers like to see others benefit from their labor. If the goal
> is to improve the performance of the http server, (remember this was posted to dev@httpd
> and that is why I reference the http server), they are wasting their time trying to
> improve the pool code. There are clearly more serious memory allocation problems to solve
> first.
Then scratch that itch, Bill, instead of moaning about it.
> Gotta run. Party on...
My point exactly ;)
Re: pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> I have a problem with the original question though. I don't understand why
> anybody is trying to state that if people want to improve the pools code, they
> shouldn't, because we use malloc in one place in the code. People are
> free to work on whatever they want.
I am not being philosophical. Sure, people can spend their time doing whatever they want.
I assume open source developers like to see others benefit from their labor. If the goal
is to improve the performance of the http server, (remember this was posted to dev@httpd
and that is why I reference the http server), they are wasting their time trying to
improve the pool code. There are clearly more serious memory allocation problems to solve
first.
Gotta run. Party on...
Bill
Re: pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 25 August 2001 11:38, Sander Striker wrote:
> > > Btw, why couldn't we use pools in brigades again?
> >
> > Because of lifetimes. If you allocate buckets and brigades out of pools,
> > the lifetimes can't change, but bucket lifetimes aren't known at creation
> > time. And, you can't use the longest possible lifetime, because that
> > would be a leak.
>
> Ah, I suppose associating a pool with a brigade wouldn't work, would it?
> The buckets can move from brigade to brigade?
There is alreadya pool associated with a brigade. That doesn't get you
anything though. A bucket needs to be able to move from one brigade
to another.
I have a problem with the original question though. I don't understand why
anybody is trying to state that if people want to improve the pools code, they
shouldn't, because we use malloc in one place in the code. People are
free to work on whatever they want. If somebody wants to remove the malloc
calls from the buckets and bucket brigades, then they can, but that doesn't
mean that everybody has to focus on that.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 25 August 2001 11:38, Sander Striker wrote:
> > > Btw, why couldn't we use pools in brigades again?
> >
> > Because of lifetimes. If you allocate buckets and brigades out of pools,
> > the lifetimes can't change, but bucket lifetimes aren't known at creation
> > time. And, you can't use the longest possible lifetime, because that
> > would be a leak.
>
> Ah, I suppose associating a pool with a brigade wouldn't work, would it?
> The buckets can move from brigade to brigade?
There is alreadya pool associated with a brigade. That doesn't get you
anything though. A bucket needs to be able to move from one brigade
to another.
I have a problem with the original question though. I don't understand why
anybody is trying to state that if people want to improve the pools code, they
shouldn't, because we use malloc in one place in the code. People are
free to work on whatever they want. If somebody wants to remove the malloc
calls from the buckets and bucket brigades, then they can, but that doesn't
mean that everybody has to focus on that.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> > Btw, why couldn't we use pools in brigades again?
>
> Because of lifetimes. If you allocate buckets and brigades out of pools,
> the lifetimes can't change, but bucket lifetimes aren't known at creation
> time. And, you can't use the longest possible lifetime, because that
> would be a leak.
Ah, I suppose associating a pool with a brigade wouldn't work, would it?
The buckets can move from brigade to brigade?
> Ryan
Sander
pools and buckets/brigades, WAS: RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> > Btw, why couldn't we use pools in brigades again?
>
> Because of lifetimes. If you allocate buckets and brigades out of pools,
> the lifetimes can't change, but bucket lifetimes aren't known at creation
> time. And, you can't use the longest possible lifetime, because that
> would be a leak.
Ah, I suppose associating a pool with a brigade wouldn't work, would it?
The buckets can move from brigade to brigade?
> Ryan
Sander
Re: Review of possible replacement for pools
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 25 August 2001 11:26, Sander Striker wrote:
> > Why are we spending time trying to optimize pools when we haven't
> > eliminated the
> > malloc/frees in the bucket brigade calls? The miniscule
> > performance improvements
> > you -might- get optimizing pools will be completely obscured by
> > the overhead of the
> > malloc/frees.
> >
> > Bill
>
> Because it also eliminates the locking in the threaded mpm.
> And, I wanted to see if I could squeeze some extra performance
> out of it and make it a bit more readable. I don't know if
> I succeeded in the latter. Or the performance for that matter :)
>
> Btw, why couldn't we use pools in brigades again?
Because of lifetimes. If you allocate buckets and brigades out of pools,
the lifetimes can't change, but bucket lifetimes aren't known at creation
time. And, you can't use the longest possible lifetime, because that
would be a leak.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: Review of possible replacement for pools
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 25 August 2001 11:26, Sander Striker wrote:
> > Why are we spending time trying to optimize pools when we haven't
> > eliminated the
> > malloc/frees in the bucket brigade calls? The miniscule
> > performance improvements
> > you -might- get optimizing pools will be completely obscured by
> > the overhead of the
> > malloc/frees.
> >
> > Bill
>
> Because it also eliminates the locking in the threaded mpm.
> And, I wanted to see if I could squeeze some extra performance
> out of it and make it a bit more readable. I don't know if
> I succeeded in the latter. Or the performance for that matter :)
>
> Btw, why couldn't we use pools in brigades again?
Because of lifetimes. If you allocate buckets and brigades out of pools,
the lifetimes can't change, but bucket lifetimes aren't known at creation
time. And, you can't use the longest possible lifetime, because that
would be a leak.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> Why are we spending time trying to optimize pools when we haven't
> eliminated the
> malloc/frees in the bucket brigade calls? The miniscule
> performance improvements
> you -might- get optimizing pools will be completely obscured by
> the overhead of the
> malloc/frees.
>
> Bill
Because it also eliminates the locking in the threaded mpm.
And, I wanted to see if I could squeeze some extra performance
out of it and make it a bit more readable. I don't know if
I succeeded in the latter. Or the performance for that matter :)
Btw, why couldn't we use pools in brigades again?
Sander
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> Why are we spending time trying to optimize pools when we haven't
> eliminated the
> malloc/frees in the bucket brigade calls? The miniscule
> performance improvements
> you -might- get optimizing pools will be completely obscured by
> the overhead of the
> malloc/frees.
>
> Bill
Because it also eliminates the locking in the threaded mpm.
And, I wanted to see if I could squeeze some extra performance
out of it and make it a bit more readable. I don't know if
I succeeded in the latter. Or the performance for that matter :)
Btw, why couldn't we use pools in brigades again?
Sander
Re: Review of possible replacement for pools
Posted by Bill Stoddard <bi...@wstoddard.com>.
Why are we spending time trying to optimize pools when we haven't eliminated the
malloc/frees in the bucket brigade calls? The miniscule performance improvements
you -might- get optimizing pools will be completely obscured by the overhead of the
malloc/frees.
Bill
> Justin Erenkrantz wrote:
>
> >On Sat, Aug 25, 2001 at 08:05:03PM +0200, Sander Striker wrote:
> >
> >>Adding an option to apr_pool_create_ex to indicate that the pool is
> >>going to be shared across threads would be an option I can certainly
> >>live with. OTOH, this will introduce an extra if(lock) pair in the
> >>apr_palloc and apr_pcalloc calls. I'll send in a patch later on.
> >>
> >
> >If you aren't shared, you have an extra NULL-check. I think that is
> >acceptable. If you are shared, we make sure we can't get trounced.
> >
> I agree, as long as the NULL-check doesn't measurably impact
> performance. One of the key success factors for the original pool
> code is the very short code path in the common case where it doesn't
> need to allocate new blocks. Adding another branch to this code
> path might or might not slow it down measurably--but we can find
> out through benchmarking.
>
> --Brian
>
>
>
Re: Review of possible replacement for pools
Posted by Brian Pane <bp...@pacbell.net>.
Justin Erenkrantz wrote:
>On Sat, Aug 25, 2001 at 08:05:03PM +0200, Sander Striker wrote:
>
>>Adding an option to apr_pool_create_ex to indicate that the pool is
>>going to be shared across threads would be an option I can certainly
>>live with. OTOH, this will introduce an extra if(lock) pair in the
>>apr_palloc and apr_pcalloc calls. I'll send in a patch later on.
>>
>
>If you aren't shared, you have an extra NULL-check. I think that is
>acceptable. If you are shared, we make sure we can't get trounced.
>
I agree, as long as the NULL-check doesn't measurably impact
performance. One of the key success factors for the original pool
code is the very short code path in the common case where it doesn't
need to allocate new blocks. Adding another branch to this code
path might or might not slow it down measurably--but we can find
out through benchmarking.
--Brian
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> Attached is a patch for the suggested additional flag to
> be able to share and use a pool across threads.
Grmpf. I sent to quickly. The lock should be used in
the cleanup functions aswell.
> Sander
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
Attached is a patch for the suggested additional flag to
be able to share and use a pool across threads.
Sander
> -----Original Message-----
> From: Justin Erenkrantz [mailto:jerenkrantz@ebuilt.com]
> Sent: 25 August 2001 19:57
> To: Sander Striker
> Cc: dev@apr.apache.org
> Subject: Re: Review of possible replacement for pools
>
>
> On Sat, Aug 25, 2001 at 08:05:03PM +0200, Sander Striker wrote:
> > Adding an option to apr_pool_create_ex to indicate that the pool is
> > going to be shared across threads would be an option I can certainly
> > live with. OTOH, this will introduce an extra if(lock) pair in the
> > apr_palloc and apr_pcalloc calls. I'll send in a patch later on.
>
> If you aren't shared, you have an extra NULL-check. I think that is
> acceptable. If you are shared, we make sure we can't get trounced.
>
> > I didn't implement apr_prealloc yet, but would like to know if people
> > would find it usefull. If so, I will post a patch.
>
> Probably - I've come across the need for such a function before. But,
> it does get to be tricky to implement. -- justin
>
>
>
>
Re: Review of possible replacement for pools
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sat, Aug 25, 2001 at 08:05:03PM +0200, Sander Striker wrote:
> Adding an option to apr_pool_create_ex to indicate that the pool is
> going to be shared across threads would be an option I can certainly
> live with. OTOH, this will introduce an extra if(lock) pair in the
> apr_palloc and apr_pcalloc calls. I'll send in a patch later on.
If you aren't shared, you have an extra NULL-check. I think that is
acceptable. If you are shared, we make sure we can't get trounced.
> I didn't implement apr_prealloc yet, but would like to know if people
> would find it usefull. If so, I will post a patch.
Probably - I've come across the need for such a function before. But,
it does get to be tricky to implement. -- justin
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
> On Sat, Aug 25, 2001 at 01:45:21PM +0200, Sander Striker wrote:
> > This _is_ a race condition. I reported this at the end of june on list.
> > Subject: Possible race condition in pools WAS: RE: Thoughts on locks.
> > At the end of the thread the conclusion was to fix it, most likely
> > through documentation.
> >
> > Personally I think that locking on each allocation will kill
> performance.
> > Just never use a pool in two threads.
>
> Yeah, I vaguely remember that thread.
>
> You can certainly use a pool in two threads. If we add an option when
> creating a pool to indicate that it isn't shared, the performance
> isn't a factor. But, allowing this race condition is going to trounce
> somebody at sometime. The likelihood of having the context switch
> happening here is low, but it is possible.
Agreed.
> I'm not exactly sure how to fix it in the current pool code without
> killing everyone. With your version, you can do it via an option to
> apr_pool_create_ex. So, I don't think we'd end up losing that much
> performance if we use the option in the right places. -- justin
Adding an option to apr_pool_create_ex to indicate that the pool is
going to be shared across threads would be an option I can certainly
live with. OTOH, this will introduce an extra if(lock) pair in the
apr_palloc and apr_pcalloc calls. I'll send in a patch later on.
I didn't implement apr_prealloc yet, but would like to know if people
would find it usefull. If so, I will post a patch.
Sander
Re: Review of possible replacement for pools
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sat, Aug 25, 2001 at 01:45:21PM +0200, Sander Striker wrote:
> This _is_ a race condition. I reported this at the end of june on list.
> Subject: Possible race condition in pools WAS: RE: Thoughts on locks.
> At the end of the thread the conclusion was to fix it, most likely
> through documentation.
>
> Personally I think that locking on each allocation will kill performance.
> Just never use a pool in two threads.
Yeah, I vaguely remember that thread.
You can certainly use a pool in two threads. If we add an option when
creating a pool to indicate that it isn't shared, the performance
isn't a factor. But, allowing this race condition is going to trounce
somebody at sometime. The likelihood of having the context switch
happening here is low, but it is possible.
I'm not exactly sure how to fix it in the current pool code without
killing everyone. With your version, you can do it via an option to
apr_pool_create_ex. So, I don't think we'd end up losing that much
performance if we use the option in the right places. -- justin
RE: Memory management requirements buckets/brigades, new pools
Posted by Ian Holsman <Ia...@cnet.com>.
On Sun, 2001-08-26 at 09:02, Sander Striker wrote:
> > Hi Sander.
> > is it possible to post patches from the most recent CVS version.
> > that would make it much easier for people to apply them
>
> Ok, I agree with you on applyability, but for readability that
> is quite different. Want me to post some patches against recent
> CVS?
[removed httpd]
if you could.
that would make it easier for me to test tomorrow.
>
> > Thanks
> > ...Ian
>
> Sander
--
Ian Holsman
Performance Measurement & Analysis
CNET Networks - 415 364-8608
RE: Memory management requirements buckets/brigades, new pools
Posted by Sander Striker <st...@apache.org>.
> Hi Sander.
> is it possible to post patches from the most recent CVS version.
> that would make it much easier for people to apply them
Ok, I agree with you on applyability, but for readability that
is quite different. Want me to post some patches against recent
CVS?
> Thanks
> ...Ian
Sander
RE: Memory management requirements buckets/brigades, new pools
Posted by Sander Striker <st...@apache.org>.
> Hi Sander.
> is it possible to post patches from the most recent CVS version.
> that would make it much easier for people to apply them
Ok, I agree with you on applyability, but for readability that
is quite different. Want me to post some patches against recent
CVS?
> Thanks
> ...Ian
Sander
RE: Memory management requirements buckets/brigades, new pools
Posted by Ian Holsman <Ia...@cnet.com>.
Hi Sander.
is it possible to post patches from the most recent CVS version.
that would make it much easier for people to apply them
Thanks
..Ian
On Sun, 2001-08-26 at 08:29, Sander Striker wrote:
> [this time including the attachment...]
>
> > Hi,
> >
> > It seems that the memory management requirements for
> > buckets is that they have to be able to control their
> > own lifetime. In other words, they need to be allocated
> > and freed on an individual basis.
> > It seems that their lifetime is bound by the lifetime
> > of the connection.
> >
> > The above let me believe that buckets need a free
> > function to complement apr_palloc. Hence the
> > attached patch that introduces apr_pfree *). I know
> > this patch introduces some extra overhead, although
> > not much, which could be unacceptable. OTOH would
> > this make it possible to use one memory management
> > scheme throughout apache...
> >
> > Maybe something to consider, maybe not. I don't
> > even know if these are the criteria or not ;)
> >
> >
> > Sander
> >
> > *) patch is against the recently posted possible
> > replacement code for pools.
>
--
Ian Holsman
Performance Measurement & Analysis
CNET Networks - 415 364-8608
RE: Memory management requirements buckets/brigades, new pools
Posted by Ian Holsman <Ia...@cnet.com>.
Hi Sander.
is it possible to post patches from the most recent CVS version.
that would make it much easier for people to apply them
Thanks
..Ian
On Sun, 2001-08-26 at 08:29, Sander Striker wrote:
> [this time including the attachment...]
>
> > Hi,
> >
> > It seems that the memory management requirements for
> > buckets is that they have to be able to control their
> > own lifetime. In other words, they need to be allocated
> > and freed on an individual basis.
> > It seems that their lifetime is bound by the lifetime
> > of the connection.
> >
> > The above let me believe that buckets need a free
> > function to complement apr_palloc. Hence the
> > attached patch that introduces apr_pfree *). I know
> > this patch introduces some extra overhead, although
> > not much, which could be unacceptable. OTOH would
> > this make it possible to use one memory management
> > scheme throughout apache...
> >
> > Maybe something to consider, maybe not. I don't
> > even know if these are the criteria or not ;)
> >
> >
> > Sander
> >
> > *) patch is against the recently posted possible
> > replacement code for pools.
>
--
Ian Holsman
Performance Measurement & Analysis
CNET Networks - 415 364-8608
RE: Memory management requirements buckets/brigades, new pools
Posted by Sander Striker <st...@apache.org>.
[this time including the attachment...]
> Hi,
>
> It seems that the memory management requirements for
> buckets is that they have to be able to control their
> own lifetime. In other words, they need to be allocated
> and freed on an individual basis.
> It seems that their lifetime is bound by the lifetime
> of the connection.
>
> The above let me believe that buckets need a free
> function to complement apr_palloc. Hence the
> attached patch that introduces apr_pfree *). I know
> this patch introduces some extra overhead, although
> not much, which could be unacceptable. OTOH would
> this make it possible to use one memory management
> scheme throughout apache...
>
> Maybe something to consider, maybe not. I don't
> even know if these are the criteria or not ;)
>
>
> Sander
>
> *) patch is against the recently posted possible
> replacement code for pools.
Memory management requirements buckets/brigades, new pools
Posted by Sander Striker <st...@apache.org>.
Hi,
It seems that the memory management requirements for
buckets is that they have to be able to control their
own lifetime. In other words, they need to be allocated
and freed on an individual basis.
It seems that their lifetime is bound by the lifetime
of the connection.
The above let me believe that buckets need a free
function to complement apr_palloc. Hence the
attached patch that introduces apr_pfree *). I know
this patch introduces some extra overhead, although
not much, which could be unacceptable. OTOH would
this make it possible to use one memory management
scheme throughout apache...
Maybe something to consider, maybe not. I don't
even know if these are the criteria or not ;)
Sander
*) patch is against the recently posted possible
replacement code for pools.
RE: Review of possible replacement for pools
Posted by Dirk-Willem van Gulik <di...@covalent.net>.
> This _is_ a race condition. I reported this at the end of june on list.
> Subject: Possible race condition in pools WAS: RE: Thoughts on locks.
> At the end of the thread the conclusion was to fix it, most likely
> through documentation.
>
> Personally I think that locking on each allocation will kill performance.
> Just never use a pool in two threads.
Whilst debugging some of my own code I found it usefull to add to each
pool a record of the thread id (which is very cheap to retrive) and do a
comparision each time you try to use it when a DEBUG flag is on. I found
that to be a practical compromize to spot the issue.
Dw
RE: Review of possible replacement for pools
Posted by Sander Striker <st...@apache.org>.
Hi,
> Enclosed is my review of your posted patch. Actually, I probably would
> like to see Brian's patch again to compare it as I think they achieve
> similar goals.
>
> The important aspect of this patch is that it makes the pool code
> easier to understand. I'll say that this would need comments.
> I'll volunteer to add comments that if this patch is accepted.
Yes. Just for clarity: I started with an empty file and copied source
or concept over from the original pools code. Then I added some of
my personal sause and this is the result. So, similarity with the
original pools code is no coincedence.
> Here goes - my comments are prefaced with a > at the beginning of
> the line (the reverse of normal as I'm too lazy to insert > at the
> beginning of the source)...
>
> -------------
> #define MIN_ALLOC 8192
> #define MAX_INDEX 32
>
> #define BOUNDARY_INDEX 12
> #define BOUNDARY_SIZE (1 << BOUNDARY_INDEX)
>
>> Okay so that people have a clue what is going on here, Sander has
>> a freelist that is arranged by size. It is based on the power of
>> 2 that it is closest to, IIRC. The 0 element is reserved for items
>> bigger than 2^32. Sander can correct me if I'm misremembering this.
Every node is a multiple of BOUNDARY_SIZE, which is in this case 4096.
So, when requesting 9034 bytes, we alloc a node of 12288 bytes and
serve it from that. The remainder is saved for the next allocation,
just like the original pools code. The minimal size of a node is
MIN_ALLOC, which is set to 8192.
On the free list, the nodes are indexed. Or, nodes up to a certain
size are indexed, I should say, the rest is dumped in the first slot.
The indexing takes place to up to BOUNDARY_SIZE * MAX_INDEX node
sizes. To see how it works, take a close look at node_malloc() and
node_free().
> #define ALIGN_DEFAULT(size) ALIGN(size, 8)
>
>> Why 8 as the default?
A guessed fixed number. No science involved. If someone has a better
way of doing this _without_ introduction of '/' and '*' I'll be happy.
If 8 is bogus, please suggest a better number.
> static APR_INLINE node_t *node_malloc(allocator_t *allocator,
> apr_size_t size)
> {
> > ...blah, blah, blah...search for it on the indexed freelist and
> > return it...if none available, call malloc...at this point, we are
> > setting up the newly allocated node.
> node->next = NULL;
> node->index = index;
> node->first_avail = (char *)node + SIZEOF_NODE_T;
> node->endp = (char *)node + size;
>
>> Problem A. You are adding the SIZEOF_NODE_T here that is implicitly
>> included in the size parameter (most calls to node_malloc has size +
>> SIZEOF_NODE_T). Either move this portion out (not sure how you
>> would do this), or have node_malloc add the space required for the
>> node_t and don't call node_malloc with + SIZEOF_NODE_T. However, that
>> strategy does present a problem in apr_pool_create_ex where you call
>> node_malloc with MIN_ALLOC as the size rather than
>> MIN_ALLOC+SIZEOF_NODE_T. So, in order to truly get MIN_ALLOC, you'd
>> have to ask for MIN_ALLOC-SIZEOF_NODE_T. Hmm. But, I don't like
>> this strategy where you *must* pass in an extra SIZEOF_NODE_T bytes.
>> (Yes, this function isn't user-callable, but still...)
I was facing the same, this is what I chose to use.
Other solutions welcomed. I somewhat like the MIN_ALLOC - SIZEOF_NODE_T
better in apr_pool_create_ex. I'll put this in a seperate patch, for
people to choose (or someone can come up with something better :).
Keep in mind that this is an internal function and a little line at the
top of the function explaining what you need to pass in would be
satisfactory
too IMHO.
> static void node_free(allocator_t *allocator, node_t *node)
> {
> node_t *next;
> apr_uint32_t index, max_index;
>
> if (allocator->lock)
> apr_lock_acquire(allocator->lock);
>
>> This reminds me that we should have something like
>> apr_lock_acquire_opt(foo) which will not do anything if the passed in
>> parameter is NULL. This would seriously cut down on the number of
>> places where this if (lock) lock_acquire syntax occurrs. Someone
>> brought this up in a commit log recently. It's a good point. Hell,
>> even a #defined macro works well for this.
I'll put a simple macro solution in this code as a seperate patch.
We might consider exporting such a macro in apr_lock.h.
> APR_DECLARE(void *) apr_palloc(apr_pool_t *pool, apr_size_t size)
> {
> node_t *active, *node;
> void *mem;
> char *endp;
>
> size = ALIGN_DEFAULT(size);
> active = pool->mactive;
>
> endp = active->first_avail + size;
> if (endp < active->endp) {
> mem = active->first_avail;
> active->first_avail = endp;
> return mem;
> }
>
>> I think this is a race condition. This is also present in the current
>> pool code, so this isn't something that you introduced. Consider the
>> following scenario:
>> Thread A has access to pool Z.
>> Thread B has access to pool Z as well.
>> Thread A calls apr_palloc, gets active, sees that there is enough
>> space, sets mem.
>> *CONTEXT SWITCH*
>> Thread B now calls apr_palloc, gets active, sess that there is enough
>> space, sets mem, updates first_avail, returns.
>> *CONTEXT SWITCH*
>> Thread A updates first_avail. Oops. =-) However, I'd suggest an
>> option in apr_create_pool_ex that says that this pool is NOT shared
>> and doesn't need locking. Otherwise, we must lock this case (not
>> an allocator lock, but a pool lock). This seems to be the only case
>> where this could happen. Seems a waste, but it definitely could happen.
>> This race doesn't matter in the apr_pool_clear or apr_pool_destroy
>> since two children don't try to clear the pool at the same time.
This _is_ a race condition. I reported this at the end of june on list.
Subject: Possible race condition in pools WAS: RE: Thoughts on locks.
At the end of the thread the conclusion was to fix it, most likely
through documentation.
Personally I think that locking on each allocation will kill performance.
Just never use a pool in two threads.
Btw, similar race conditions are present in the cleanup code in pools.
I haven't touched those. Take a look and conclude that no 2 threads
should ever register a cleanup at the same time. Or I must be missing
something...
If there are going to be two locks (I doubt it, but it might happen),
the pool lock should be used when adding/removing child pools to a
pool. At the moment I am using the allocator lock for that.
>> I know the code doesn't crash under high load, so I know it has
>> a reasonable degree of quality and it is definitely easier to
>> understand. Assuming the matters brought up above are addressed
>> to my satisfaction, I'll give this a +1. -- justin
Thanks for the review, patches to last version and new version (which
is the old version, with patches applied), attached.
Sander