You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Akins <ba...@web.turner.com> on 2005/08/18 15:02:03 UTC
[PATCH] mod_disk_cache: speed up read_table
This one's kinda ugly.
Rather than rely on apr_file_gets, this stores the total length of the
tables, then the serialized table in store_table. In read_table, it
reads this length, allocs that amount, and reads the headers into the
buffer. Then it just uses memchr to "parse" it into a table.
I didn't mess with the EBCDIC stuff, so alot of the patch is just where
I commented all that out.
I also commented out where the headers file is opened buffered. It is
faster on my tests boxen (Linux 2.6) to use unbuffered (probably because
at lease pagesize if being buffered anyway). YMMV on this one.
Anyway, I got a nice 5-8% increase in all my benchmarks by using this
change.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <ba...@web.turner.com>.
Colm MacCarthaigh wrote:
>
> The optimisations wouldn't be removing buffering, they'd be using a
> different kind of buffering :-) If MMap is not available/enabled we can
> fail back to a buffered APR read.
>
I did some tinkering with mod_disk_cache. I encapsulated all the reads
of the headers file into its own functions (open, read, seek, close) to
test different methods. I've only tested to different ways. Both ways
do not use apr_file_gets in read_table/array, but rather use the memchr
stuff I posted earlier this week.
-suck whole header file into an apr_pcalloc'ed buffer. Performed 5-9%
better than stock, as expected from my earlier testing. This saves the
unnecessary memory copies when using APR_BUFFERED as well as the gets
overhead.
-mmap the headers file. On Linux 2.6, this performed about 20-30%
*worse* than stock. <shrug>
The patch is really, really ugly but it works. I can post it if anyone
cares to see it.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <br...@turner.com>.
Colm MacCarthaigh wrote:
> The optimisations wouldn't be removing buffering, they'd be using a
> different kind of buffering :-) If MMap is not available/enabled we can
> fail back to a buffered APR read.
So it's evolved even further...
I would be interested to see in a variety of cases which one is actually
faster:
-normal buffered apr read
-alloc entire buffer, read_full into it.
-mmap file.
Colm, are you already writing code? If not, I may give it a swing...
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Thu, Aug 18, 2005 at 10:43:45AM -0700, Justin Erenkrantz wrote:
> None of the code in mod_disk_cache used buffering before I turned it on.
> It gave significant speedups in my performance tests by reducing the
> syscall overhead. I also had identified and fixed some bugs in apr's
> buffering code to go along with these speedups that went out in APR 1.0.1.
> So, unless the profiles have changed (unlikely), removing buffering is a
> bad idea. -- justin
The optimisations wouldn't be removing buffering, they'd be using a
different kind of buffering :-) If MMap is not available/enabled we can
fail back to a buffered APR read.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <br...@turner.com>.
Justin Erenkrantz wrote:
> None of the code in mod_disk_cache used buffering before I turned it
> on. It gave significant speedups in my performance tests by reducing
> the syscall overhead. I also had identified and fixed some bugs in
> apr's buffering code to go along with these speedups that went out in
> APR 1.0.1. So, unless the profiles have changed (unlikely), removing
> buffering is a bad idea. -- justin
Colm's idea would eliminate the need for the buffering as it would read
the entire headers file in at one time and manipulate the memory in
place. In this case buffering would actually hurt performance as it
would have lots of extra memcpy's.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On August 18, 2005 1:34:56 PM -0400 Brian Akins <ba...@web.turner.com>
wrote:
> The optimizations that Colm is talking about should be helpful everywhere.
> Also, that's why we test to make sure it does "kill us." Also, I don't think
> we "know" it will kill us.
None of the code in mod_disk_cache used buffering before I turned it on. It
gave significant speedups in my performance tests by reducing the syscall
overhead. I also had identified and fixed some bugs in apr's buffering code
to go along with these speedups that went out in APR 1.0.1. So, unless the
profiles have changed (unlikely), removing buffering is a bad idea. -- justin
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <ba...@web.turner.com>.
Justin Erenkrantz wrote:
> But, that's why we do the buffered read. It almost always ends up being
> one read() for me as the header file is always less than 4k.
True.
> I don't believe we should necessarily optimize based on the results of
> one platform when we know its going to kill us everywhere else. -- justin
The optimizations that Colm is talking about should be helpful
everywhere. Also, that's why we test to make sure it does "kill us."
Also, I don't think we "know" it will kill us.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On August 18, 2005 3:59:05 PM +0100 Colm MacCarthaigh <co...@stdlib.net>
wrote:
> Thinking about it, everything is going into memory anyway, so why not
> just stat() the file and read it all in in one go?
>
> We know it's never going to be that big anyway. This completely
> minimises the number of read()'s, which is a big deal and if memory
> management is an issue, we can use a subpool for the buffer.
But, that's why we do the buffered read. It almost always ends up being one
read() for me as the header file is always less than 4k.
I don't believe we should necessarily optimize based on the results of one
platform when we know its going to kill us everywhere else. -- justin
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <br...@turner.com>.
Colm MacCarthaigh wrote:
> Do you mean as in using MAP_SHARED? I could see that being a
> race-condition nightmare. For example cache_info_t's pointers to the
> header tables would have to be local to the process, but the memory for
> the cache_info_t itself wouldn't be. Definitely to be avoided!
True. your comment about regular mmap are also true -- it would be
interesting to test.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Thu, Aug 18, 2005 at 12:11:56PM -0400, Brian Akins wrote:
> >Yep, but there are definite speed-up's to be had. I'm going to
> >canibalise your patch and try some of the above anyway.
>
> Cool. I'd be willing to help.
I'll be putting on-line all of my TODO's and patches-in-progress
shortly, I have some reading and non-code writing to do first, but then
back to being more productive ;-)
> On a side note, maybe interesting: what if the whole file was just
> mmaped? (use '\0' for delimiter in tables). This would allow every
> worker referencing an object to reference the same memory.
Do you mean as in using MAP_SHARED? I could see that being a
race-condition nightmare. For example cache_info_t's pointers to the
header tables would have to be local to the process, but the memory for
the cache_info_t itself wouldn't be. Definitely to be avoided!
Regular MMap, as long as it honoured EnableMMap would be fine though.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <ba...@web.turner.com>.
Colm MacCarthaigh wrote:
>
> Thinking about it, everything is going into memory anyway, so why not
> just stat() the file and read it all in in one go?
Yes! Brilliant. We need all that data anyway.
so:
open file.
length = get_info
read_full(file, length).
manipulate pointers for table (apr_table_addn)
> If we try really hard, a table_serialize() and table_unserialize() could
> just cut up the strings and user pointer-manipulation to escape even the
> need for a subpool destory.
That's basically what I was doing.
> Yep, but there are definite speed-up's to be had. I'm going to
> canibalise your patch and try some of the above anyway.
Cool. I'd be willing to help.
On a side note, maybe interesting: what if the whole file was just
mmaped? (use '\0' for delimiter in tables). This would allow every
worker referencing an object to reference the same memory. I did some
testing with our caching module and it was not very efficient, but it is
structure differently. Should be fairly easy to try: replace the alloc
and read with a mmap.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Thu, Aug 18, 2005 at 10:43:41AM -0400, Brian Akins wrote:
> >I can understand why this is faster, I'm guessing that you've enough RAM
> >that you're retrieving files from the vmcache and that the higher-layer
> >buffering is just overhead. This is probably going to be the majority
> >case, but would do with testing.
>
> Not to heap more configuration into it, but this could also be configurable.
Thinking about it, everything is going into memory anyway, so why not
just stat() the file and read it all in in one go?
We know it's never going to be that big anyway. This completely
minimises the number of read()'s, which is a big deal and if memory
management is an issue, we can use a subpool for the buffer.
If we try really hard, a table_serialize() and table_unserialize() could
just cut up the strings and user pointer-manipulation to escape even the
need for a subpool destory.
> >It's not nearly as neat a patch, but including the lengths of the tables
> >in the cache_info_t object is doable, and in fact would allow for
> >decreasing the number of read()'s to just 3. One for the format, one
> >more for the cache_info_t, and one more the tables. This would probably
> >produce another similar speed improvement.
>
> That's what I wanted to do, but it was messy. I would like to have the
> length of resp_hdrs, req_hdrs, and the data (would save a stat...) in
> the disk_cache_info_t, but that is a much more major thing from what I
> can tell.
>
> Basically would have to rearrange store_headers completely...
Yep, but there are definite speed-up's to be had. I'm going to
canibalise your patch and try some of the above anyway.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <br...@turner.com>.
Colm MacCarthaigh wrote:
> I can understand why this is faster, I'm guessing that you've enough RAM
> that you're retrieving files from the vmcache and that the higher-layer
> buffering is just overhead. This is probably going to be the majority
> case, but would do with testing.
Not to heap more configuration into it, but this could also be configurable.
> It's not nearly as neat a patch, but including the lengths of the tables
> in the cache_info_t object is doable, and in fact would allow for
> decreasing the number of read()'s to just 3. One for the format, one
> more for the cache_info_t, and one more the tables. This would probably
> produce another similar speed improvement.
That's what I wanted to do, but it was messy. I would like to have the
length of resp_hdrs, req_hdrs, and the data (would save a stat...) in
the disk_cache_info_t, but that is a much more major thing from what I
can tell.
Basically would have to rearrange store_headers completely...
>>-#if APR_CHARSET_EBCDIC
>
>
> Well that won't fly ;-)
>
I know, I know :) I just wanted to get the idea out there without
having to muck with that. Obviously the final patch should consider
EBDCID as well.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Thu, Aug 18, 2005 at 09:22:13AM -0400, Brian Akins wrote:
> Thanks to all who reminded me what a dumb-a## I am this morning...
>
> I forgot the patch. Here it is.
Cool.
> diff -ru httpd-trunk.orig/modules/cache/mod_disk_cache.c httpd-trunk.new2/modules/cache/mod_disk_cache.c
> --- httpd-trunk.orig/modules/cache/mod_disk_cache.c 2005-08-09 11:51:09.473251000 -0400
> +++ httpd-trunk.new2/modules/cache/mod_disk_cache.c 2005-08-18 08:52:33.346214476 -0400
> @@ -51,7 +51,7 @@
> */
>
> #define VARY_FORMAT_VERSION 3
> -#define DISK_FORMAT_VERSION 4
> +#define DISK_FORMAT_VERSION 5
There should be a mod_disk_cache.h, which htcacheclean can include. It
only need contain these define's and the disk_cache_info_t structure,
but it prevents the silly out-of-sync problems that keep cropping up :)
> @@ -414,7 +414,7 @@
> dobj->prefix = NULL;
>
> dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
> - flags = APR_READ|APR_BINARY|APR_BUFFERED;
> + flags = APR_READ|APR_BINARY/*|APR_BUFFERED*/;
I can understand why this is faster, I'm guessing that you've enough RAM
that you're retrieving files from the vmcache and that the higher-layer
buffering is just overhead. This is probably going to be the majority
case, but would do with testing.
> + /*read length first*/
> + len = sizeof(length);
> +
> + if((rv = apr_file_read_full(file, &length, len, &len)) != APR_SUCCESS) {
> + return rv;
> + }
> +
> + buffer = apr_palloc(r->pool, length);
Embedding more integers like this in the file doesn't make much sense to
me, especially when using unbuffered IO, the read() seems like
unneccessary pain.
It's not nearly as neat a patch, but including the lengths of the tables
in the cache_info_t object is doable, and in fact would allow for
decreasing the number of read()'s to just 3. One for the format, one
more for the cache_info_t, and one more the tables. This would probably
produce another similar speed improvement.
I'll add that to what I'm tinkering on.
> -#if APR_CHARSET_EBCDIC
Well that won't fly ;-)
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Brian Akins <ba...@web.turner.com>.
Thanks to all who reminded me what a dumb-a## I am this morning...
I forgot the patch. Here it is.
Caffiene level much to low...
Brian Akins wrote:
> This one's kinda ugly.
>
> Rather than rely on apr_file_gets, this stores the total length of the
> tables, then the serialized table in store_table. In read_table, it
> reads this length, allocs that amount, and reads the headers into the
> buffer. Then it just uses memchr to "parse" it into a table.
>
> I didn't mess with the EBCDIC stuff, so alot of the patch is just where
> I commented all that out.
>
> I also commented out where the headers file is opened buffered. It is
> faster on my tests boxen (Linux 2.6) to use unbuffered (probably because
> at lease pagesize if being buffered anyway). YMMV on this one.
>
>
> Anyway, I got a nice 5-8% increase in all my benchmarks by using this
> change.
>
>
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_disk_cache: speed up read_table
Posted by Jeff Trawick <tr...@gmail.com>.
On 8/18/05, Brian Akins <ba...@web.turner.com> wrote:
> I didn't mess with the EBCDIC stuff, so alot of the patch is just where
> I commented all that out.
The EBCDIC stuff came here:
http://svn.apache.org/viewcvs.cgi?rev=105236&view=rev
Apparently Justin needed logic similar to ap_scan_script_header_err()
and extracted a lot of that code to add here.
ap_scan_script_header_err() on an EBCDIC platform is set up to allow a
CGI script the capability to write a response header in ASCII (giant
shrug).
Is there a way to get response header written to the cache in ASCII?
If not, the code can be scrapped. Maybe the folks who tweaked the
code later to actually compile on EBCDIC considered that, maybe not.
It just seems really odd to me.
(Caveat: cache-ignorant; never used mod_*_cache on EBCDIC)