brigade cleanup (was: Re: Finally found the problems. :-()

[Greg Stein <gs...@lyra.org>]

Ah. My mistake... I thought brigade structures were malloc'd. If they're
from a pool, then we have problems (see below). Also, presuming that we make
them mallocs, then we need to be proactive (see below).

On Sun, Dec 31, 2000 at 03:21:43PM -0800, rbb@covalent.net wrote:
>...
> We should not be trying to outsmart our allocation system.  Brigades are
> incredibly small, specifically so that they can be allocated and just left
> for the pools to deal with.  We are literally taking about a total of
> three pointers.

Three pointers multiplied by the content-size of the response is too big.
Heck, one byte in proportion to the content-size is too big.

Like I said: if a brigade is created in proportion to the content-size, then
it must be destroyed.

If the brigade is created as a fixed count per response, then yes: leave it
for the pools.

But that fixed vs proportional is important... proportional a Bad Thing(tm).

> We just don't need the added function calls associated
> with destroying a bucket_brigade.  This requires that we kill a cleanup
> that isn't going to do anything anyway.  The memory doesn't actually get
> freed when we destory the brigade, and if the brigade has just be
> concat'ed, then it is going to be empty.
> 
> Basically, what you are asking for, is to do always kill a cleanup that is
> basically a no-op whenever we destroy a brigade.  There is no reason to do
> this.  We don't free any memory, because the memory is allocated out of a
> pool, and we never free memory from pools.  If we call destroy_brigade
> whenever we concat, all we are doing is searching a linked list and
> calling a function that is essentially a no-op because the brigade is
> empty.

Yes, the brigade will be empty, so no worries in terms of buckets.

I think the problem is that the brigade itself is allocated from a pool. As
you say: that means we can't toss it after a concat. It would seem that we
should reconsider allocating brigades from pools.

[ specifically: consider that ap_r* creates a new brigade each time they are
  called. this means the number of brigades is proportional to the content
  size. we must be proactive in clearing them out ]

In summary:

1) don't use pool allocations for brigade structures. Use malloc and a
   tricky cleanup (we can't use a dumb cleanup because then we're doing a
   pool-alloc (for the cleanup info) for every brigade creation again).

2) call ap_brigade_destroy() when we're done with a brigade

[ we don't need to be as careful with destroying brigades that are fixed in
  count. the pool cleanup will toss those. ]

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Re: brigade cleanup (was: Re: Finally found the problems. :-()

[rb...@covalent.net]

> Ah. My mistake... I thought brigade structures were malloc'd. If they're
> from a pool, then we have problems (see below). Also, presuming that we make
> them mallocs, then we need to be proactive (see below).

We cannot malloc() these.  There is too big a chance that we will miss
cleaning them up.  These really have to be palloc's.  As far as how many
we allocate, this is currently a problem, but not an unsolvable one.  I
knopw that Tony and others (I think Victor from IBM or Bill) have been
working on making ap_r* functions buffer their data, so that we don't use
as many brigades.  We could also modify some of the code slightly, so that
we used one brigade per filter + 1 (the handler).  This is all possible
within our current framework, without upsetting things too much.

> On Sun, Dec 31, 2000 at 03:21:43PM -0800, rbb@covalent.net wrote:
> >...
> > We should not be trying to outsmart our allocation system.  Brigades are
> > incredibly small, specifically so that they can be allocated and just left
> > for the pools to deal with.  We are literally taking about a total of
> > three pointers.
> 
> Three pointers multiplied by the content-size of the response is too big.
> Heck, one byte in proportion to the content-size is too big.

If we are using a brigade per byte, then we have big problems.  This is
one of the reasons that I dislike the ap_r* functions.  Basically, any
handler that uses those functions will be very sub-optimal.  We can work
on improving that, but we shouldn't try to optimize for those
handlers.  We should optimize for handlers that create their own
brigades, which is what we currently do.

> In summary:
> 
> 1) don't use pool allocations for brigade structures. Use malloc and a
>    tricky cleanup (we can't use a dumb cleanup because then we're doing a
>    pool-alloc (for the cleanup info) for every brigade creation again).

I disagree, and this was discussed at the filter meeting, and everybody
agreed that using pools for the brigade was the correct thing to do.

> 2) call ap_brigade_destroy() when we're done with a brigade

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------