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 Pane <bp...@pacbell.net> on 2001/11/10 06:20:23 UTC
[PATCH] performance fix for mod_include
In mod_include, we use apr_pcalloc to create an include_ctx_t,
which consists of about 80 bytes of integer and pointer fields
and a pair of character buffers used to hold strings. Each
of the character buffers is 8KB...
calloc'ing all this space accounts for about 9% of the total
usr-mode CPU time in the httpd.
This patch zero-fills just the integer and pointer fields in
the structure, plus the first byte of each of the string buffers.
With the patch, I'm seeing a 19% improvement in SSI throughput
in informal testing (ab running a single request at a time,
over the loopback on Linux).
--Brian
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Friday 09 November 2001 09:56 pm, William A. Rowe, Jr. wrote:
> From: "Brian Pane" <bp...@pacbell.net>
> Sent: Friday, November 09, 2001 11:20 PM
>
> > In mod_include, we use apr_pcalloc to create an include_ctx_t,
> > which consists of about 80 bytes of integer and pointer fields
> > and a pair of character buffers used to hold strings. Each
> > of the character buffers is 8KB...
> >
> > calloc'ing all this space accounts for about 9% of the total
> > usr-mode CPU time in the httpd.
> >
> > This patch zero-fills just the integer and pointer fields in
> > the structure, plus the first byte of each of the string buffers.
> > With the patch, I'm seeing a 19% improvement in SSI throughput
> > in informal testing (ab running a single request at a time,
> > over the loopback on Linux).
>
> +1... but I do question the size of those buffers... they seem
> inordinately large and wasteful. If we have an error, why not
> simply a char* that can be alloc'ed once needed?
I'll do you one better. The error string, if you look through the code, is copied
on the first call to the filter from a per-dir config structure. Then it is parsed,
and turned into a bucket. That seems incredibly wasteful. Why do we copy
it at all, the only time we use that string anywhere it is a const char *. We
could very easily just pass the string directly from the per-dir config structure.
BTW, all of that applies equally to the time_str variable too. Those string
copies are only done when they are needed, but still, why copy if we don't
need to. So, I am +1 for applying the patch, but I think there is a lot more work
that could be done to improve mod_include. :-)
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Brian Pane" <bp...@pacbell.net>
Sent: Friday, November 09, 2001 11:20 PM
> In mod_include, we use apr_pcalloc to create an include_ctx_t,
> which consists of about 80 bytes of integer and pointer fields
> and a pair of character buffers used to hold strings. Each
> of the character buffers is 8KB...
>
> calloc'ing all this space accounts for about 9% of the total
> usr-mode CPU time in the httpd.
>
> This patch zero-fills just the integer and pointer fields in
> the structure, plus the first byte of each of the string buffers.
> With the patch, I'm seeing a 19% improvement in SSI throughput
> in informal testing (ab running a single request at a time,
> over the loopback on Linux).
+1... but I do question the size of those buffers... they seem
inordinately large and wasteful. If we have an error, why not
simply a char* that can be alloc'ed once needed?
Re: [PATCH] performance fix for mod_include
Posted by Aaron Bannert <aa...@clove.org>.
On Fri, Nov 09, 2001 at 09:20:23PM -0800, Brian Pane wrote:
> In mod_include, we use apr_pcalloc to create an include_ctx_t,
> which consists of about 80 bytes of integer and pointer fields
> and a pair of character buffers used to hold strings. Each
> of the character buffers is 8KB...
>
> calloc'ing all this space accounts for about 9% of the total
> usr-mode CPU time in the httpd.
>
> This patch zero-fills just the integer and pointer fields in
> the structure, plus the first byte of each of the string buffers.
> With the patch, I'm seeing a 19% improvement in SSI throughput
> in informal testing (ab running a single request at a time,
> over the loopback on Linux).
+1 - I have this same pcalloc --> palloc+memset change in my tree, but
not the reduced memset length; that is a novel idea.
-aaron
Re: [PATCH] performance fix for mod_include
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Saturday, November 10, 2001 12:38 PM
> On Saturday 10 November 2001 10:34 am, Cliff Woolley wrote:
> >
> > memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
>
> No offense, but that is an ugly line of code. :-)
Guys, look at the mod_mime cmd table, we have some xt_offset style macros
that make this sort of cruft much more readable ;)
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 10 November 2001 01:16 pm, Brian Pane wrote:
> Ryan Bloom wrote:
> >On Saturday 10 November 2001 12:51 pm, Brian Pane wrote:
> >>Ryan Bloom wrote:
> >>[...]
> >>
> >>>>Later today I'll post a new patch that turns error_str and time_str
> >>>>into char*.
> >>>
> >>>Just remove them from the structure all together. They aren't necessary
> >>>in that structure.
> >>
> >>Where else would you put them? If they move out of the filter
> >>context struct, they'll have to move someplace where they're
> >>still accessible by the filter in subrequests (to handle the
> >>case of shtml files included within other shtml files).
> >
> >All we do with them is copy them into the filter context structure. Then
> >we use that copy when we parse the string. But, parsing is a
> > non-destructive operation. So, we can get rid of them from the
> > structure, and just use the copies in the directory structure.
>
> Huh? If you're referring to the ap_ssi_parse_string() calls in
> handle_config(), note that ctx->error_str isn't an input to the
> parsing operation, it's an output. When someone uses
> <!--#config errmsg="my error message" -->
>
> the new error message gets stored in ctx->error_str, where it's
> usable in the subrequests for included shtml files.
>
> My updated patch (the one I sent today) preserves these semantics
> but skips the malloc of error_str and time_str until/unless they're
> needed.
Yeah, I saw that a few minutes ago. :-(
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 10 November 2001 01:03 pm, Ryan Bloom wrote:
> On Saturday 10 November 2001 12:51 pm, Brian Pane wrote:
> > Ryan Bloom wrote:
> > [...]
> >
> > >>Later today I'll post a new patch that turns error_str and time_str
> > >>into char*.
> > >
> > >Just remove them from the structure all together. They aren't necessary
> > >in that structure.
> >
> > Where else would you put them? If they move out of the filter
> > context struct, they'll have to move someplace where they're
> > still accessible by the filter in subrequests (to handle the
> > case of shtml files included within other shtml files).
>
> All we do with them is copy them into the filter context structure. Then> we use that copy when we parse the string. But, parsing is a
> non-destructive operation. So, we can get rid of them from the structure,
> and just use the copies in the directory structure.
>
> Ryan
Damn, I counted the arguments to ap_parse_ssi_string incorrectly. I still
think we can get rid of those strings, but I need to look at the code a bit
closer.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by Brian Pane <bp...@pacbell.net>.
Ryan Bloom wrote:
>On Saturday 10 November 2001 12:51 pm, Brian Pane wrote:
>
>>Ryan Bloom wrote:
>>[...]
>>
>>>>Later today I'll post a new patch that turns error_str and time_str
>>>>into char*.
>>>>
>>>Just remove them from the structure all together. They aren't necessary
>>>in that structure.
>>>
>>Where else would you put them? If they move out of the filter
>>context struct, they'll have to move someplace where they're
>>still accessible by the filter in subrequests (to handle the
>>case of shtml files included within other shtml files).
>>
>
>All we do with them is copy them into the filter context structure. Then
>we use that copy when we parse the string. But, parsing is a non-destructive
>operation. So, we can get rid of them from the structure, and just use the
>copies in the directory structure.
>
Huh? If you're referring to the ap_ssi_parse_string() calls in
handle_config(), note that ctx->error_str isn't an input to the
parsing operation, it's an output. When someone uses
<!--#config errmsg="my error message" -->
the new error message gets stored in ctx->error_str, where it's
usable in the subrequests for included shtml files.
My updated patch (the one I sent today) preserves these semantics
but skips the malloc of error_str and time_str until/unless they're
needed.
--Brian
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 10 November 2001 12:51 pm, Brian Pane wrote:
> Ryan Bloom wrote:
> [...]
>
> >>Later today I'll post a new patch that turns error_str and time_str
> >>into char*.
> >
> >Just remove them from the structure all together. They aren't necessary
> >in that structure.
>
> Where else would you put them? If they move out of the filter
> context struct, they'll have to move someplace where they're
> still accessible by the filter in subrequests (to handle the
> case of shtml files included within other shtml files).
All we do with them is copy them into the filter context structure. Then
we use that copy when we parse the string. But, parsing is a non-destructive
operation. So, we can get rid of them from the structure, and just use the
copies in the directory structure.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by Brian Pane <bp...@pacbell.net>.
Ryan Bloom wrote:
[...]
>>Later today I'll post a new patch that turns error_str and time_str
>>into char*.
>>
>
>Just remove them from the structure all together. They aren't necessary
>in that structure.
>
Where else would you put them? If they move out of the filter
context struct, they'll have to move someplace where they're
still accessible by the filter in subrequests (to handle the
case of shtml files included within other shtml files).
--Brian
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 10 November 2001 10:51 am, Brian Pane wrote:
> Ryan Bloom wrote:
> [...]
>
> >>memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
> >
> >No offense, but that is an ugly line of code. :-) I really think that
> >error_str and time_str can just be removed from that structure. That
> > would allow us to not add the casts, and it would speed up that function.
> > I'll do that work sometime this week unless somebody beats me to it.
>
> I think a cast to size_t in that line is unnecesary (the result of
> the pointer subtraction is integral). But I just realized that
> there's another strong reason to just remove error_str and time_str:
> the fact that the structure is over 8KB basically guarantees that
> the apr_palloc code will have to malloc a new block from the system
> free list.
>
> Later today I'll post a new patch that turns error_str and time_str
> into char*.
Just remove them from the structure all together. They aren't necessary
in that structure.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by Brian Pane <bp...@pacbell.net>.
Ryan Bloom wrote:
[...]
>>memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
>>
>
>No offense, but that is an ugly line of code. :-) I really think that
>error_str and time_str can just be removed from that structure. That would
>allow us to not add the casts, and it would speed up that function. I'll do that
>work sometime this week unless somebody beats me to it.
>
I think a cast to size_t in that line is unnecesary (the result of
the pointer subtraction is integral). But I just realized that
there's another strong reason to just remove error_str and time_str:
the fact that the structure is over 8KB basically guarantees that
the apr_palloc code will have to malloc a new block from the system
free list.
Later today I'll post a new patch that turns error_str and time_str
into char*.
--Brian
Re: [PATCH] performance fix for mod_include
Posted by Cliff Woolley <cl...@yahoo.com>.
On Sat, 10 Nov 2001, Ryan Bloom wrote:
> > memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
>
> No offense, but that is an ugly line of code. :-)
None taken... I never said it was pretty, just more correct. :)
A prettier one looks like this:
memset(f->ctx, 0, APR_XtOffsetOf(include_ctx_t,error_str));
But...
> I really think that error_str and time_str can just be removed from
> that structure. That would allow us to not add the casts, and it
> would speed up that function. I'll do that work sometime this week
> unless somebody beats me to it.
I totally agree that this is a better way to go.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] performance fix for mod_include
Posted by Ryan Bloom <rb...@covalent.net>.
On Saturday 10 November 2001 10:34 am, Cliff Woolley wrote:
> On Fri, 9 Nov 2001, Brian Pane wrote:
> > This patch zero-fills just the integer and pointer fields in
> > the structure, plus the first byte of each of the string buffers.
> > With the patch, I'm seeing a 19% improvement in SSI throughput
> > in informal testing (ab running a single request at a time,
> > over the loopback on Linux).
> >
> >+ */
> >+ f->ctx = ctx = apr_palloc(f->c->pool, sizeof(*ctx));
> >+ memset(f->ctx, 0, (void *)&(ctx->error_str) - (void *)ctx);
> >+ ctx->error_str[0] = 0;
> >+ ctx->time_str[0] = 0;
> > if (ctx != NULL) {
>
> You can't do arithmetic on void pointers, and you're converting from a
> pointer to a size_t without a cast. I think it should be this:
>
> memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
No offense, but that is an ugly line of code. :-) I really think that
error_str and time_str can just be removed from that structure. That would
allow us to not add the casts, and it would speed up that function. I'll do that
work sometime this week unless somebody beats me to it.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] performance fix for mod_include
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 9 Nov 2001, Brian Pane wrote:
> This patch zero-fills just the integer and pointer fields in
> the structure, plus the first byte of each of the string buffers.
> With the patch, I'm seeing a 19% improvement in SSI throughput
> in informal testing (ab running a single request at a time,
> over the loopback on Linux).
>+ */
>+ f->ctx = ctx = apr_palloc(f->c->pool, sizeof(*ctx));
>+ memset(f->ctx, 0, (void *)&(ctx->error_str) - (void *)ctx);
>+ ctx->error_str[0] = 0;
>+ ctx->time_str[0] = 0;
> if (ctx != NULL) {
You can't do arithmetic on void pointers, and you're converting from a
pointer to a size_t without a cast. I think it should be this:
memset(f->ctx, 0, (size_t)((char *)&(ctx->error_str) - (char *)ctx));
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA