You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Cliff Woolley <jw...@virginia.edu> on 2002/03/01 07:12:04 UTC
bucket api patch
Well, it's not finished yet, but it's something you can look at if you
like. http://icarus.apache.org/~jwoolley/ .
Things missing:
(a) calls to apr_bucket_alloc_create() and _destroy() at strategic
locations within the MPMs. I'm hoping that's something one of you guys
can help me with, since I haven't exactly spent a lot of time in the MPMs
lately.
(b) the actually allocator itself -- right now, it's a wrapper around
malloc() and free() (which is why we can get away without (a) at the
moment) -- this part will fall easily into place as soon as Sander's pool
API change is done. He and I talked about that today and it seems a
perfect fit for the job.
I've tested the patch as it is right now to compile and run and pass
httpd-test, but more eyes looking for stupid mistakes in largely
uninteresting but widespread changes would be much appreciated.
Thanks,
Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: bucket api patch
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Mar 02, 2002 at 10:47:07PM -0800, Brian Pane wrote:
> Brian Pane wrote:
> > Cliff Woolley wrote:
> ...
>
> >> apr_bucket_free() knows nothing about buckets, so ignore the fact that a
Maybe call it apr_freelist_* then? If the systems are orthogonal, then
reflect that in the name.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
>On Sat, 2 Mar 2002, Brian Pane wrote:
>
>>The 8KB allocations are from apr_brigade_write().
>>
>
>Not all of them should be. If they are, I forgot some changes. All of
>the APR_BUCKET_BUFF_SIZE malloc()'s should have been changed to
>apr_bucket_alloc() calls, and those happen in foo_read() and
>apr_bucket_heap_make() and a handful of other places in addition to
>apr_brigade_write(). Though we might not be hitting many of those other
>spots since we have sendfile and mmap, etc.
>
You're right. I just did some gdb'ing and found that the 8KB
allocs in my test case (a simple mod_include scenario) were
actually from the socket_read() function in apr_buckets_socket.c.
This test case didn't actually hit the apr_brigade_write()
8KB alloc.
>>These 8KB blocks imply a design constraint for
>>the bucket allocator: it can't quite be a simple
>>power-of-two allocator.
>>
>
>I'd thought about that. We are of course free to pick whatever block
>sizes we want.... if we want a block size that's just a bit bigger than
>8KB, I don't see a problem with that. Or we could just decrease
>APR_BUCKET_BUFF_SIZE a bit so that we stay inside our two-page (on most
>systems) 8KB boundary. Either way's fine with me.
>
+1 on decreasing APR_BUCKET_BUFF_SIZE to fit the whole thing
within 8KB.
--Brian
Re: bucket api patch
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 2 Mar 2002, Brian Pane wrote:
> 8 10%
> 16 15%
> 32 73%
> 8192 2%
Cool, that looks exactly like what I'd expected it to look like. Thanks
for doing that.
> The 8KB allocations are from apr_brigade_write().
Not all of them should be. If they are, I forgot some changes. All of
the APR_BUCKET_BUFF_SIZE malloc()'s should have been changed to
apr_bucket_alloc() calls, and those happen in foo_read() and
apr_bucket_heap_make() and a handful of other places in addition to
apr_brigade_write(). Though we might not be hitting many of those other
spots since we have sendfile and mmap, etc.
> These 8KB blocks imply a design constraint for
> the bucket allocator: it can't quite be a simple
> power-of-two allocator.
I'd thought about that. We are of course free to pick whatever block
sizes we want.... if we want a block size that's just a bit bigger than
8KB, I don't see a problem with that. Or we could just decrease
APR_BUCKET_BUFF_SIZE a bit so that we stay inside our two-page (on most
systems) 8KB boundary. Either way's fine with me.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
Brian Pane wrote:
> Cliff Woolley wrote:
...
>> apr_bucket_free() knows nothing about buckets, so ignore the fact that a
>> bucket contains an apr_bucket_alloc_t* (see below). The block allocated
>> by apr_bucket_alloc() will have internal information stored immediately
>> prior to (or following?) the location returned from
>> apr_bucket_alloc. In
>> other words, apr_bucket_alloc will over-allocate so that it has a few
>> bytes for its own purposes. You and I had talked about that a while
>> back
>> and agreed that that seemed a reasonable way to handle the problem.
>>
>
> Cool, I'd forgotten about storing the metadata stored right in
> front of the block returned by apr_bucket_alloc(); that will solve
> the problem very cleanly.
I just ran an httpd with an instrumented version of
apr_bucket_alloc to collect data about the sizes of
the buckets being allocated. The results look like
this:
alloc size percent
(rounded up of total
to next power bucket
of two) allocs
8 10%
16 15%
32 73%
8192 2%
The 8KB allocations are from apr_brigade_write().
These 8KB blocks imply a design constraint for
the bucket allocator: it can't quite be a simple
power-of-two allocator. Each allocation of an 8KB
block will really need 8KB+sizeof(void*) bytes, in
order to hold the apr_bucket_alloc_t* in front of
the block. So if the underlying allocator is strictly
applying a power-of-two rule, it will create 16KB
blocks for these. (I think we'll run into this
problem if the apr_brigade_alloc() function uses
the same node_malloc() function that's currently
used in apr_pool_t, for example.)
--Brian
Re: bucket api patch
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 2 Mar 2002, Brian Pane wrote:
> apr_bucket_heap and ap_bucket_error, I think
Hmmm.. I'll go check it out. Thanks for the review. :)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
>On Sat, 2 Mar 2002, Brian Pane wrote:
>
>>I'm starting to look through the code in detail now, and one
>>aspect of the API bothers me:
>>
>>How is apr_bucket_free() going to figure out which apr_bucket_alloc_t
>>to return the block to? All it has as input is a pointer to a
>>bucket, in void* form. Some of the bucket types contain an
>>apr_bucket_alloc_t*, but not all of them.
>>
>
>apr_bucket_free() knows nothing about buckets, so ignore the fact that a
>bucket contains an apr_bucket_alloc_t* (see below). The block allocated
>by apr_bucket_alloc() will have internal information stored immediately
>prior to (or following?) the location returned from apr_bucket_alloc. In
>other words, apr_bucket_alloc will over-allocate so that it has a few
>bytes for its own purposes. You and I had talked about that a while back
>and agreed that that seemed a reasonable way to handle the problem.
>
Cool, I'd forgotten about storing the metadata stored right in
front of the block returned by apr_bucket_alloc(); that will solve
the problem very cleanly.
>The reason for the apr_bucket_alloc_t* in some bucket types is so that
>apr_bucket_foo_make() can know which allocator to use when the new bucket
>type needs to allocate storage for private bucket-type-specific structures
>(eg struct apr_bucket_file). But *all* bucket types should have this
>pointer... did I miss one?
>
apr_bucket_heap and ap_bucket_error, I think
--Brian
Re: bucket api patch
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 2 Mar 2002, Brian Pane wrote:
> I'm starting to look through the code in detail now, and one
> aspect of the API bothers me:
>
> How is apr_bucket_free() going to figure out which apr_bucket_alloc_t
> to return the block to? All it has as input is a pointer to a
> bucket, in void* form. Some of the bucket types contain an
> apr_bucket_alloc_t*, but not all of them.
apr_bucket_free() knows nothing about buckets, so ignore the fact that a
bucket contains an apr_bucket_alloc_t* (see below). The block allocated
by apr_bucket_alloc() will have internal information stored immediately
prior to (or following?) the location returned from apr_bucket_alloc. In
other words, apr_bucket_alloc will over-allocate so that it has a few
bytes for its own purposes. You and I had talked about that a while back
and agreed that that seemed a reasonable way to handle the problem.
The reason for the apr_bucket_alloc_t* in some bucket types is so that
apr_bucket_foo_make() can know which allocator to use when the new bucket
type needs to allocate storage for private bucket-type-specific structures
(eg struct apr_bucket_file). But *all* bucket types should have this
pointer... did I miss one?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
I'm starting to look through the code in detail now, and one
aspect of the API bothers me:
How is apr_bucket_free() going to figure out which apr_bucket_alloc_t
to return the block to? All it has as input is a pointer to a
bucket, in void* form. Some of the bucket types contain an
apr_bucket_alloc_t*, but not all of them.
--Brian
Cliff Woolley wrote:
>Well, it's not finished yet, but it's something you can look at if you
>like. http://icarus.apache.org/~jwoolley/ .
>
>Things missing:
>
>(a) calls to apr_bucket_alloc_create() and _destroy() at strategic
>locations within the MPMs. I'm hoping that's something one of you guys
>can help me with, since I haven't exactly spent a lot of time in the MPMs
>lately.
>
>(b) the actually allocator itself -- right now, it's a wrapper around
>malloc() and free() (which is why we can get away without (a) at the
>moment) -- this part will fall easily into place as soon as Sander's pool
>API change is done. He and I talked about that today and it seems a
>perfect fit for the job.
>
>I've tested the patch as it is right now to compile and run and pass
>httpd-test, but more eyes looking for stupid mistakes in largely
>uninteresting but widespread changes would be much appreciated.
>
>Thanks,
>Cliff
>
>--------------------------------------------------------------
> Cliff Woolley
> cliffwoolley@yahoo.com
> Charlottesville, VA
>
>
>
Re: update for bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
>On Sat, 2 Mar 2002, Brian Pane wrote:
>
>>I had to add an allocator as an argument to
>>SPLIT_AND_PASS_PRETAG_BUCKETS to get mod_include
>>and mod_cgi to work (a patch against current CVS head
>>is attached)
>>
>
>Whoops, that must have gotten lost in the big cvs-conflict-resolution run.
>Don't know how I managed to get it to compile... <sigh>. Anyway, what I
>would have done it this way:
>
>
>Index: mod_include.h
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.h,v
>retrieving revision 1.29
>diff -u -d -r1.29 mod_include.h
>--- mod_include.h 23 Feb 2002 20:56:36 -0000 1.29
>+++ mod_include.h 2 Mar 2002 23:20:55 -0000
>@@ -208,7 +209,8 @@
> \
> tag_plus = apr_brigade_split(brgd, cntxt->head_start_bucket); \
> if (cntxt->output_flush) { \
>- APR_BRIGADE_INSERT_TAIL(brgd, apr_bucket_flush_create()); \
>+ APR_BRIGADE_INSERT_TAIL(brgd, \
>+ apr_bucket_flush_create(brgd->bucket_alloc)); \
> } \
> rc = ap_pass_brigade(next, brgd); \
> cntxt->bytes_parsed = 0; \
>
Makes sense; I like this approach a lot better than my patch to add an
extra arg
--Brian
Re: update for bucket api patch
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 2 Mar 2002, Brian Pane wrote:
> I had to add an allocator as an argument to
> SPLIT_AND_PASS_PRETAG_BUCKETS to get mod_include
> and mod_cgi to work (a patch against current CVS head
> is attached)
Whoops, that must have gotten lost in the big cvs-conflict-resolution run.
Don't know how I managed to get it to compile... <sigh>. Anyway, what I
would have done it this way:
Index: mod_include.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.h,v
retrieving revision 1.29
diff -u -d -r1.29 mod_include.h
--- mod_include.h 23 Feb 2002 20:56:36 -0000 1.29
+++ mod_include.h 2 Mar 2002 23:20:55 -0000
@@ -208,7 +209,8 @@
\
tag_plus = apr_brigade_split(brgd, cntxt->head_start_bucket); \
if (cntxt->output_flush) { \
- APR_BRIGADE_INSERT_TAIL(brgd, apr_bucket_flush_create()); \
+ APR_BRIGADE_INSERT_TAIL(brgd, \
+ apr_bucket_flush_create(brgd->bucket_alloc)); \
} \
rc = ap_pass_brigade(next, brgd); \
cntxt->bytes_parsed = 0; \
The whole reason for having a bucket_alloc stored with the brigade itself
is so that you can get at it when you are creating buckets that you know
you're just going to stick immediately into that brigade and be done with
them. This is the trick I needed to avoid redoing the entire
apr_brigade_printf() and friends API, which would have been a massive
undertaking (I know because I tried it once).
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
update for bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
I had to add an allocator as an argument to
SPLIT_AND_PASS_PRETAG_BUCKETS to get mod_include
and mod_cgi to work (a patch against current CVS head
is attached)
--Brian
Re: bucket api patch
Posted by Bill Stoddard <bi...@wstoddard.com>.
> Cliff Woolley wrote:
>
> >Well, it's not finished yet, but it's something you can look at if you
> >like. http://icarus.apache.org/~jwoolley/ .
> >
> >Things missing:
> >
> >(a) calls to apr_bucket_alloc_create() and _destroy() at strategic
> >locations within the MPMs. I'm hoping that's something one of you guys
> >can help me with, since I haven't exactly spent a lot of time in the MPMs
> >lately.
> >
>
> I can take on this part for prefork and worker. One question,
> though: how will apr_bucket_free() work once the real version
> is implemented? Will it actually free up the space within
> the allocator for reuse, or will it be a no-op?
>
> The strategy for placing the apr_bucket_alloc_create() and
> _destroy() calls depends a lot on how apr_bucket_free() works.
>
I can cover the Windows MPM.
Bill
Re: bucket api patch
Posted by Cliff Woolley <jw...@virginia.edu>.
On Thu, 28 Feb 2002, Brian Pane wrote:
> >(a) calls to apr_bucket_alloc_create() and _destroy() at strategic
> >locations within the MPMs. I'm hoping that's something one of you guys
> >can help me with, since I haven't exactly spent a lot of time in the MPMs
> >lately.
>
> I can take on this part for prefork and worker. One question,
> though: how will apr_bucket_free() work once the real version
> is implemented? Will it actually free up the space within
> the allocator for reuse, or will it be a no-op?
It is definitely a real _free() and not a noop. Otherwise we wouldn't get
much for freelists. :)
> The strategy for placing the apr_bucket_alloc_create() and
> _destroy() calls depends a lot on how apr_bucket_free() works.
The ideas I had in mind go like this:
prefork: create one bucket allocator for each child when the child
is born, and use that same bucket allocator in every connection
handled by that child
worker: similar to prefork, except there can either be one allocator
per worker thread or one per transaction pool (now that we're
reusing those pools, might as well associate a bucket allocator
with each one and reuse those too... don't know if that's a
benefit or not)
others: I guess it's one per worker thread? I know next to nothing
about these MPMs
Note that the future MPM that does async I/O and passes requests from one
thread to another mid-flight will *have to* have a group of bucket
allocators that it keeps around, assigning an available one to each
*connection* as it comes in -- ie, this one cannot be thread-private
like some of the others might be able to.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: bucket api patch
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
>Well, it's not finished yet, but it's something you can look at if you
>like. http://icarus.apache.org/~jwoolley/ .
>
>Things missing:
>
>(a) calls to apr_bucket_alloc_create() and _destroy() at strategic
>locations within the MPMs. I'm hoping that's something one of you guys
>can help me with, since I haven't exactly spent a lot of time in the MPMs
>lately.
>
I can take on this part for prefork and worker. One question,
though: how will apr_bucket_free() work once the real version
is implemented? Will it actually free up the space within
the allocator for reuse, or will it be a no-op?
The strategy for placing the apr_bucket_alloc_create() and
_destroy() calls depends a lot on how apr_bucket_free() works.
--Brian