You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Cody Sherr <cs...@covalent.net> on 2001/08/16 01:12:57 UTC
[PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
appropriate, and using local buffer.
Thanks for the feedback.
--
Cody Sherr
Engineer
Covalent Technologies
phone: (415)536-5292
email: csherr@covalent.net
Index: protocol.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/protocol.c,v
retrieving revision 1.38
diff -u -r1.38 protocol.c
--- protocol.c 2001/08/07 16:40:25 1.38
+++ protocol.c 2001/08/15 23:04:07
@@ -1133,19 +1133,63 @@
return nbyte;
}
+struct ap_vrprintf_data {
+ apr_vformatter_buff_t vbuff;
+ request_rec *r;
+ char *buff;
+};
+
+static apr_status_t r_flush(apr_vformatter_buff_t *buff)
+{
+ /* callback function passed to ap_vformatter to be called when
+ * vformatter needs to write into buff and buff.curpos > buff.endpos */
+
+ /* ap_vrprintf_data passed as a apr_vformatter_buff_t, which is then
+ * "downcast" to an ap_vrprintf_data */
+ struct ap_vrprintf_data *vd = (struct ap_vrprintf_data*)buff;
+
+ if (vd->r->connection->aborted)
+ return -1;
+
+ /* r_flush is called when vbuff is completely full */
+ if (buffer_output(vd->r, vd->buff, AP_IOBUFSIZE)) {
+ return -1;
+ }
+
+ /* reset the buffer position */
+ vd->vbuff.curpos = vd->buff;
+ vd->vbuff.endpos = vd->buff + AP_IOBUFSIZE;
+
+ return APR_SUCCESS;
+}
+
AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
{
- char buf[4096];
apr_size_t written;
+ struct ap_vrprintf_data vd;
+ static char vrprintf_buf[AP_IOBUFSIZE];
+
+ vd.vbuff.curpos = vrprintf_buf;
+ vd.vbuff.endpos = vrprintf_buf + AP_IOBUFSIZE;
+ vd.r = r;
+ vd.buff = vrprintf_buf;
if (r->connection->aborted)
return -1;
- /* ### fix this mechanism to allow more than 4K of output */
- written = apr_vsnprintf(buf, sizeof(buf), fmt, va);
+ written = apr_vformatter(r_flush, &vd.vbuff, fmt, va);
+ /* tack on null terminator on remaining string */
+ *(vd.vbuff.curpos) = '\0';
- if (buffer_output(r, buf, written) != APR_SUCCESS)
- return -1;
+ if (written != -1) {
+ int n = vd.vbuff.curpos - vrprintf_buf;
+
+ /* last call to buffer_output, to finish clearing the buffer */
+ if (buffer_output(r, vrprintf_buf,n) != APR_SUCCESS)
+ return -1;
+
+ written += n;
+ }
return written;
}
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 16 August 2001 06:40, Greg Marr wrote:
> At 03:37 AM 08/16/2001, Ryan Bloom wrote:
> >Applied and committed. :-)
> >
> >Ryan
> >
> >On Wednesday 15 August 2001 16:12, Cody Sherr wrote:
> > > Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
> > > appropriate, and using local buffer.
>
> IIRC, the buffer was local, but still static, so it has the same
> problems as the static buffer outside the function.
This has been fixed. Thanks for keeping an eye on me. :-)
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] resubmit new and improved ap_vrprintf fix to
handle > 4K
Posted by Greg Marr <gr...@alum.wpi.edu>.
At 03:37 AM 08/16/2001, Ryan Bloom wrote:
>Applied and committed. :-)
>
>Ryan
>
>
>On Wednesday 15 August 2001 16:12, Cody Sherr wrote:
> > Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
> > appropriate, and using local buffer.
IIRC, the buffer was local, but still static, so it has the same
problems as the static buffer outside the function.
--
Greg Marr
gregm@alum.wpi.edu
"We thought you were dead."
"I was, but I'm better now." - Sheridan, "The Summoning"
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Ryan Bloom <rb...@covalent.net>.
Applied and committed. :-)
Ryan
On Wednesday 15 August 2001 16:12, Cody Sherr wrote:
> Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
> appropriate, and using local buffer.
>
> Thanks for the feedback.
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle >
4K
Posted by dean gaudet <de...@arctic.org>.
On Mon, 10 Sep 2001, Ryan Bloom wrote:
>
> > > /* this will typically exit on the first test */
> > > for (f = r->output_filters; f != NULL; f = f->next)
> > > if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> > > break;
> > >
> > > i'm puking. strcasecmp???? strings??
> >
> > Yah :-) It could be optimized by recording a flag in the request_rec,
> > stating that the filter was inserted (rather than looking for it). The
> > strcasecmp() shouldn't be necessary because we uppercase f->frec->name
> > nowadays.
> >
> > Not that it will improve things overall, but there ya go. :-)
>
> This could actually be improved even more, by just checking for the actual
> pointer. f->frec is actually a global filter_rec, that we could easily assign to
> a global variable when the filter is registered. Then, that strcasecmp becomes
>
> if (f->frec == old_write_filter_rec)
>
> That could actually be a pretty big improvement, because we remove all string
> comparisons.
that'd be much better :)
-dean
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Ryan Bloom <rb...@covalent.net>.
> > /* this will typically exit on the first test */
> > for (f = r->output_filters; f != NULL; f = f->next)
> > if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> > break;
> >
> > i'm puking. strcasecmp???? strings??
>
> Yah :-) It could be optimized by recording a flag in the request_rec,
> stating that the filter was inserted (rather than looking for it). The
> strcasecmp() shouldn't be necessary because we uppercase f->frec->name
> nowadays.
>
> Not that it will improve things overall, but there ya go. :-)
This could actually be improved even more, by just checking for the actual
pointer. f->frec is actually a global filter_rec, that we could easily assign to
a global variable when the filter is registered. Then, that strcasecmp becomes
if (f->frec == old_write_filter_rec)
That could actually be a pretty big improvement, because we remove all string
comparisons.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Ryan Bloom <rb...@covalent.net>.
> > > i don't get it. this new ap_rsprintf is less efficient than the
> > > ap_bsprintf in apache-1.3.
> >
> > I don't doubt it. The primary intent for ap_r* was compatibility; the
> > most optimal mechanism for delivering content is through the new brigade
> > mechanisms (down thru the filter chain).
>
> are other methods for people to write easy putc/puts/printf-style code in
> 2.0?
Yep. Take a look at ap_fwrite/putc/puts/printf, and apr_brigade_write/putc/puts/printf.
Those are meant to be more performance aware. The functions allocate memory
only when necessary, but once the buffer has been allocated, we do a single copy
into that buffer.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle >
4K
Posted by dean gaudet <de...@arctic.org>.
On Mon, 10 Sep 2001, Greg Stein wrote:
> On Mon, Sep 10, 2001 at 09:13:46PM -0700, dean gaudet wrote:
> > buhhh?
> >
> > you guys built a zero-copy infrastructure and you're putting in a one-copy
> > sprintf?
>
> sprintf varieties must be at least one-copy, based on our vformatter code
> (which BUFF used, too). It seems you're implying sprintf should be zero-copy
> also. ??
yeah sorry i was just thinking of the copies before write(2). 1.3
vformatter goes direct into the BUFF buffer which then goes to write(2)...
2.0 goes into a stack buffer then a heap buffer then to write(2).
> > i don't get it. this new ap_rsprintf is less efficient than the
> > ap_bsprintf in apache-1.3.
>
> I don't doubt it. The primary intent for ap_r* was compatibility; the most
> optimal mechanism for delivering content is through the new brigade
> mechanisms (down thru the filter chain).
are other methods for people to write easy putc/puts/printf-style code in
2.0?
> > also, wow, ap_rputc really sucks now. that's a lot of code (in
> > buffer_output) compared to:
> >
> > *buf++ = c;
> >
> > guess all your abstraction has hidden the real buffer and you can't do any
> > useful optimisations?
>
> At this point, we are not trying to optimize "around" the filter chain.
> Since we can't know what is next in the chain, buffer_output we must use
> ap_fwrite() to buffer the data.
in the case of a simple module just wanting to do putc/puts/printf-style
output i don't see why it can't always have a buffer mechanism similar to
the old BUFF. even if it's being filtered or whatever, those steps happen
after the BUFF has flushed.
> In the future? Yah. Maybe we'll be able to do more, but that won't happen
> for a while.
>
> >...
> > oh ouch i gotta stop looking:
> >
> > /* this will typically exit on the first test */
> > for (f = r->output_filters; f != NULL; f = f->next)
> > if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> > break;
> >
> > i'm puking. strcasecmp???? strings??
>
> Yah :-) It could be optimized by recording a flag in the request_rec,
> stating that the filter was inserted (rather than looking for it). The
> strcasecmp() shouldn't be necessary because we uppercase f->frec->name
> nowadays.
>
> Not that it will improve things overall, but there ya go. :-)
well, the flag wouldn't be necessary if the module itself made the choice
of having the new fancy (but complicated to use) output mechanisms, or if
it just wants a basic old-school BUFF.
-dean
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Sep 10, 2001 at 09:13:46PM -0700, dean gaudet wrote:
> buhhh?
>
> you guys built a zero-copy infrastructure and you're putting in a one-copy
> sprintf?
sprintf varieties must be at least one-copy, based on our vformatter code
(which BUFF used, too). It seems you're implying sprintf should be zero-copy
also. ??
> i don't get it. this new ap_rsprintf is less efficient than the
> ap_bsprintf in apache-1.3.
I don't doubt it. The primary intent for ap_r* was compatibility; the most
optimal mechanism for delivering content is through the new brigade
mechanisms (down thru the filter chain).
> also, wow, ap_rputc really sucks now. that's a lot of code (in
> buffer_output) compared to:
>
> *buf++ = c;
>
> guess all your abstraction has hidden the real buffer and you can't do any
> useful optimisations?
At this point, we are not trying to optimize "around" the filter chain.
Since we can't know what is next in the chain, buffer_output we must use
ap_fwrite() to buffer the data.
In the future? Yah. Maybe we'll be able to do more, but that won't happen
for a while.
>...
> oh ouch i gotta stop looking:
>
> /* this will typically exit on the first test */
> for (f = r->output_filters; f != NULL; f = f->next)
> if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> break;
>
> i'm puking. strcasecmp???? strings??
Yah :-) It could be optimized by recording a flag in the request_rec,
stating that the filter was inserted (rather than looking for it). The
strcasecmp() shouldn't be necessary because we uppercase f->frec->name
nowadays.
Not that it will improve things overall, but there ya go. :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
Posted by Ryan Bloom <rb...@covalent.net>.
Yeah, the OLD_WRITE filter removes a lot of the optimizations we should
be able to get with the filtering logic. If you take a look at the apr_brigade_*
functions, you will see a more direct way to do this stuff. We get direct access
to the buffers, and assuming the buffer was already allocated, it is easy to
just append to it.
IMHO, the OLD_WRITE filter was a mistake, but the group decided that it was
the right way to go. I would be interested in seeing just how the performance
of the server changes by making ap_r* simple wrappers around apr_brigade_*.
The code was written to work as macros for most of the functions, so it should
be easy to implement.
Ryan
On Monday 10 September 2001 21:13, dean gaudet wrote:
> buhhh?
>
> you guys built a zero-copy infrastructure and you're putting in a one-copy
> sprintf? i don't get it. this new ap_rsprintf is less efficient than the
> ap_bsprintf in apache-1.3.
>
> also, wow, ap_rputc really sucks now. that's a lot of code (in
> buffer_output) compared to:
>
> *buf++ = c;
>
> guess all your abstraction has hidden the real buffer and you can't do any
> useful optimisations?
>
> this is one of the reasons i kept saying there's a reason there's a FILE *
> interface and a read(2)/write(2) interface :)
>
> oh ouch i gotta stop looking:
>
> /* this will typically exit on the first test */
> for (f = r->output_filters; f != NULL; f = f->next)
> if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> break;
>
> i'm puking. strcasecmp???? strings??
>
> ok enough new-httpd for a while :)
>
> -dean
>
> On Wed, 15 Aug 2001, Cody Sherr wrote:
> > Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
> > appropriate, and using local buffer.
> >
> > Thanks for the feedback.
> >
> > --
> > Cody Sherr
> >
> > Engineer
> > Covalent Technologies
> >
> > phone: (415)536-5292
> > email: csherr@covalent.net
> >
> >
> > Index: protocol.c
> > ===================================================================
> > RCS file: /home/cvspublic/httpd-2.0/server/protocol.c,v
> > retrieving revision 1.38
> > diff -u -r1.38 protocol.c
> > --- protocol.c 2001/08/07 16:40:25 1.38
> > +++ protocol.c 2001/08/15 23:04:07
> > @@ -1133,19 +1133,63 @@
> > return nbyte;
> > }
> >
> > +struct ap_vrprintf_data {
> > + apr_vformatter_buff_t vbuff;
> > + request_rec *r;
> > + char *buff;
> > +};
> > +
> > +static apr_status_t r_flush(apr_vformatter_buff_t *buff)
> > +{
> > + /* callback function passed to ap_vformatter to be called when
> > + * vformatter needs to write into buff and buff.curpos > buff.endpos
> > */ +
> > + /* ap_vrprintf_data passed as a apr_vformatter_buff_t, which is then
> > + * "downcast" to an ap_vrprintf_data */
> > + struct ap_vrprintf_data *vd = (struct ap_vrprintf_data*)buff;
> > +
> > + if (vd->r->connection->aborted)
> > + return -1;
> > +
> > + /* r_flush is called when vbuff is completely full */
> > + if (buffer_output(vd->r, vd->buff, AP_IOBUFSIZE)) {
> > + return -1;
> > + }
> > +
> > + /* reset the buffer position */
> > + vd->vbuff.curpos = vd->buff;
> > + vd->vbuff.endpos = vd->buff + AP_IOBUFSIZE;
> > +
> > + return APR_SUCCESS;
> > +}
> > +
> > AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
> > {
> > - char buf[4096];
> > apr_size_t written;
> > + struct ap_vrprintf_data vd;
> > + static char vrprintf_buf[AP_IOBUFSIZE];
> > +
> > + vd.vbuff.curpos = vrprintf_buf;
> > + vd.vbuff.endpos = vrprintf_buf + AP_IOBUFSIZE;
> > + vd.r = r;
> > + vd.buff = vrprintf_buf;
> >
> > if (r->connection->aborted)
> > return -1;
> >
> > - /* ### fix this mechanism to allow more than 4K of output */
> > - written = apr_vsnprintf(buf, sizeof(buf), fmt, va);
> > + written = apr_vformatter(r_flush, &vd.vbuff, fmt, va);
> > + /* tack on null terminator on remaining string */
> > + *(vd.vbuff.curpos) = '\0';
> >
> > - if (buffer_output(r, buf, written) != APR_SUCCESS)
> > - return -1;
> > + if (written != -1) {
> > + int n = vd.vbuff.curpos - vrprintf_buf;
> > +
> > + /* last call to buffer_output, to finish clearing the buffer */
> > + if (buffer_output(r, vrprintf_buf,n) != APR_SUCCESS)
> > + return -1;
> > +
> > + written += n;
> > + }
> >
> > return written;
> > }
--
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle >
4K
Posted by dean gaudet <dg...@arctic.org>.
buhhh?
you guys built a zero-copy infrastructure and you're putting in a one-copy
sprintf? i don't get it. this new ap_rsprintf is less efficient than the
ap_bsprintf in apache-1.3.
also, wow, ap_rputc really sucks now. that's a lot of code (in
buffer_output) compared to:
*buf++ = c;
guess all your abstraction has hidden the real buffer and you can't do any
useful optimisations?
this is one of the reasons i kept saying there's a reason there's a FILE *
interface and a read(2)/write(2) interface :)
oh ouch i gotta stop looking:
/* this will typically exit on the first test */
for (f = r->output_filters; f != NULL; f = f->next)
if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
break;
i'm puking. strcasecmp???? strings??
ok enough new-httpd for a while :)
-dean
On Wed, 15 Aug 2001, Cody Sherr wrote:
>
>
> Small changes, AP_IOBUFSIZE macro (8K) for the buffer size, seemed
> appropriate, and using local buffer.
>
> Thanks for the feedback.
>
> --
> Cody Sherr
>
> Engineer
> Covalent Technologies
>
> phone: (415)536-5292
> email: csherr@covalent.net
>
>
> Index: protocol.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/server/protocol.c,v
> retrieving revision 1.38
> diff -u -r1.38 protocol.c
> --- protocol.c 2001/08/07 16:40:25 1.38
> +++ protocol.c 2001/08/15 23:04:07
> @@ -1133,19 +1133,63 @@
> return nbyte;
> }
>
> +struct ap_vrprintf_data {
> + apr_vformatter_buff_t vbuff;
> + request_rec *r;
> + char *buff;
> +};
> +
> +static apr_status_t r_flush(apr_vformatter_buff_t *buff)
> +{
> + /* callback function passed to ap_vformatter to be called when
> + * vformatter needs to write into buff and buff.curpos > buff.endpos */
> +
> + /* ap_vrprintf_data passed as a apr_vformatter_buff_t, which is then
> + * "downcast" to an ap_vrprintf_data */
> + struct ap_vrprintf_data *vd = (struct ap_vrprintf_data*)buff;
> +
> + if (vd->r->connection->aborted)
> + return -1;
> +
> + /* r_flush is called when vbuff is completely full */
> + if (buffer_output(vd->r, vd->buff, AP_IOBUFSIZE)) {
> + return -1;
> + }
> +
> + /* reset the buffer position */
> + vd->vbuff.curpos = vd->buff;
> + vd->vbuff.endpos = vd->buff + AP_IOBUFSIZE;
> +
> + return APR_SUCCESS;
> +}
> +
> AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
> {
> - char buf[4096];
> apr_size_t written;
> + struct ap_vrprintf_data vd;
> + static char vrprintf_buf[AP_IOBUFSIZE];
> +
> + vd.vbuff.curpos = vrprintf_buf;
> + vd.vbuff.endpos = vrprintf_buf + AP_IOBUFSIZE;
> + vd.r = r;
> + vd.buff = vrprintf_buf;
>
> if (r->connection->aborted)
> return -1;
>
> - /* ### fix this mechanism to allow more than 4K of output */
> - written = apr_vsnprintf(buf, sizeof(buf), fmt, va);
> + written = apr_vformatter(r_flush, &vd.vbuff, fmt, va);
> + /* tack on null terminator on remaining string */
> + *(vd.vbuff.curpos) = '\0';
>
> - if (buffer_output(r, buf, written) != APR_SUCCESS)
> - return -1;
> + if (written != -1) {
> + int n = vd.vbuff.curpos - vrprintf_buf;
> +
> + /* last call to buffer_output, to finish clearing the buffer */
> + if (buffer_output(r, vrprintf_buf,n) != APR_SUCCESS)
> + return -1;
> +
> + written += n;
> + }
>
> return written;
> }
>
>