You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/01/19 21:07:40 UTC
ap_r* performance patch
We have two basic patches that have been posted to the list, and
performance for both is basically equivalent. We need to decide which
patch to use. I would like to suggest that we just vote and make a
decision. Since my goal is to get one of these patches committed ASAP, I
suggest that the vote should end Sunday morning, and the patch should be
committed Sunday afternoon.
so, we have basically two patches:
[ ] ap_brigade_* functions (rbb's)
[ ] old_write_filter (gstein's)
The goal is not to surpress discussion, it is simply to speed it up a
bit. If anybody has any questions/comments about the two patches, please
ask.
I will tally the votes on Sunday morning, and commit either patch Sunday
afternoon. Once it is committed, we can fix up any small problems that
might exist. The goal right now is to just decide on a direction and
start moving that way.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> I will tally the votes on Sunday morning, and commit either patch Sunday
> afternoon. Once it is committed, we can fix up any small problems that
> might exist. The goal right now is to just decide on a direction and
> start moving that way.
I'll try to get to this by Sunday noon EST. I'm a bit distracted by
some other stuff at the moment...
Jeff
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> They could just return to ap_rwrite() which simplifies their 1.3
> compatibility. They'd get compat and they'd get speed.
How does that help with filters?
This is why a general solution is needed. Even within Apache, we are
already seeing modules implement this buffering themselves. By fixing
Apache without fixing the brigade API, we don't even solve our own
problem.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jan 22, 2001 at 12:52:58PM -0800, rbb@covalent.net wrote:
>
> > The generality thing is nice but not a huge deal to me, because AFAIK we
> > have nobody else asking for/needing buffering outside of ap_r* today. I
> > do like how gstein caught the out-of-order data problem and provided a
> > fix before it bit us.
>
> Oh BTW, mod_perl and mod_snake have already written the buffering logic
> for themselves. My patch allows them to remove that logic, AFAICT, Greg's
> does not.
They could just return to ap_rwrite() which simplifies their 1.3
compatibility. They'd get compat and they'd get speed.
But it is true: my patch does not assist with people that want to create a
lot of small buckets (or, to put it another way, to create a brigade bit by
tiny bit). It fixes the ap_r* problem, in a way that retains ordering with
effort on the part of the coder. I would advocate the tail-bucket variant of
your patch to solve the bit-by-bit-brigade issue for brigade users.
So if they want to upgrade to the brigade API, and they want to deal with
possibly-small buckets, then they can use the tail-bucket APIs.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: ap_r* performance patch
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Doug MacEachern" <do...@covalent.net>
Sent: Monday, January 22, 2001 5:35 PM
> the more i think about it, the more i really like apr_brigade_*
> being usable outside of the request_rec and outside of httpd. libapr is
> rad, i've used it to write 3 libraries so far that work outside of httpd,
> an http/s client, rewrite of cybersource's client library and a generic
> credit card clearing house api. filtering in the http/s client would be
> useful and i could use apr_brigade_* instead of BIO_* in the cybersource
> client.
Ack... after looking at this, we have three patches that require some slight
touchup (each of them), so I'm still +1 for applying rbb's more generic and
portable implementation and fix what folks feel needs to be fixed.
It's great to see apr actually gaining converts, I think the even bigger
success of 2.0 may be systems that leverage apr on several fronts.
Bill
Re: ap_r* performance patch
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> Just trying to get a tally.
>
> So far I have:
>
> greg's patch: gstein, gregames
> rbb's patch: rbb, dougm?, jtrawick?
>
> I am including Jeff because he said a few days ago he was leaning toward
> my patch.
Okay, I've been poked... But note that I didn't intend to say I was
leaning towards your patch (and in fact I am leaning towards Greg's).
Sorry for the delay, but various physical ailments have slowed me down for
the last four or five days.
I need to look at the Ryan patch some more... It was pretty quick for
me to determine that Greg's behaved in a way that I am comfortable
with. I'll spend some more time with Ryan's before voting.
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> On Mon, Jan 22, 2001 at 09:14:35PM -0800, rbb@covalent.net wrote:
> >...
> > > 2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
> > > specifically: how will you avoid copying the content into the brigade?
> >
> > Please cross #2 off this list. The following patch modifies
> > default_handler to print a string of 100,000 'a's using the ap_write
> > API. This is done with NO copies, and no heap corruption.
>
> Consider the following (pseudo) code:
>
> void do_output()
> {
> char buf[10000]; /* larger than 9k */
>
> bb = apr_brigade_create();
>
> memset(buf, 'a', sizeof(buf));
> apr_brigade_write(bb, buf, sizeof(buf));
>
> memset(buf, 'b', sizeof(buf));
> apr_brigade_write(bb, buf, sizeof(buf));
>
> ap_pass_brigade(r->output_filters, bb);
> }
This will actually generate 10k of a's, and it will insert those into the
brigade, it will then overwrite those 10k of a's with 10k of b's
(modifying the original a's in the process). The output will end up being
20k of b's, yes. In other words, if somebody writes bad code, we won't
work properly. This hardly seems like a good argument for not using a
patch that performs better and solves the problem in a much more general
way. Regardless, please drop this. The patch has been removed, so commit
yours and move on.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Greg Stein <gs...@lyra.org>.
Yes, I realize that Ryan withdrew his patch, but I want to make it clear to
people that there are specific problems with that direction of development.
People have lined up behind apr_brigade_write without understanding its
drawbacks. I've tried to explain the issue and to make it clear, and it kind
of peeves me when people don't stop to listen.
Here is one more shot at explaining the large-write problem with the
apr_brigade_write style of interface. If we ever want to add this kind of
interface to APR (which is fine), then I want to ensure that we all
understand its limitations.
On Mon, Jan 22, 2001 at 09:14:35PM -0800, rbb@covalent.net wrote:
>...
> > 2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
> > specifically: how will you avoid copying the content into the brigade?
>
> Please cross #2 off this list. The following patch modifies
> default_handler to print a string of 100,000 'a's using the ap_write
> API. This is done with NO copies, and no heap corruption.
Consider the following (pseudo) code:
void do_output()
{
char buf[10000]; /* larger than 9k */
bb = apr_brigade_create();
memset(buf, 'a', sizeof(buf));
apr_brigade_write(bb, buf, sizeof(buf));
memset(buf, 'b', sizeof(buf));
apr_brigade_write(bb, buf, sizeof(buf));
ap_pass_brigade(r->output_filters, bb);
}
This will generate 20k of 'b'.
I said it quite clearly: the apr_brigade_write/puts/putstrs/etc cannot use
transient buckets. The transient thing happens to work (in this case) for
ap_r* because of a fluke in their code flow. But apr_brigade_write/* are
intrinsically broken. That will lead to breakage for users of that API, or
possible problems for ap_r* if their control flow changes.
Sigh. I tried to describe the issue in my recent emails. apr_brigade_write/*
must copy everything passed in, even when it is large. Therefore, to avoid
the copying, ap_r* has to have conditional logic which shoves a lot of
processing back into ap_r*. At that point, this patch will be building up
two hunks of code: one in ap_r* and one in apr_brigade. I'd rather see one
group in ap_r* to deal with its specific problems.
> The
> ap_vrprintf can't be done without copying content easily, but the concept
> is there. BTW, this patch fixes all of them, except for ap_vrprintf,
> mainly because I didn't want to spend much time on this today.
Not a problem :-) Dealing with apr_vformatter is going to be a bitch.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: ap_r* performance patch
Posted by Greg Stein <gs...@lyra.org>.
[ some design-style wrap-up (mostly) unrelated to the patches ]
On Mon, Jan 22, 2001 at 08:08:56PM -0800, rbb@covalent.net wrote:
>...
> The problem here is one of priorities. I find it to be much more
> important that the solution works for modules other than the core. Mine
> does, your's can't.
If you want to find that solution for modules, and make it available, then
great. Hopefully, it can be used for ap_r*, too. I don't believe that
apr_brigade_write is a panacea for this -- it needs more work.
I will *gladly* rip out OLD_WRITE when a lower-level solution arises which
can also solve ap_r* cleanly.
> The ordering isn't a problem with a bit of diligence.
Requiring diligence == adding bugs. Not requiring diligence means your bugs
come from elsewhere.
> The large-write
> issue is either solved, or the solution is obvious from my last patch.
Sorry, but I disagree.
>...
> Except, buckets are defined as being read-only, so what you are saying is
> buckets are read-only, oh except for this special one that is
> read/write. That is a poor API design, and it is inviting problems for
> module writers.
Give me a break. You're trying to latch onto semantics to support your
position. There is nothing that says we cannot have a bucket that changes
its data over time. And if/when we do, that does not impact the overall
design or the other bucket types.
> > All that aside: if you aren't intending to insert an apr_brigade_flush()
> > call into various brigade macros, then just *how* are you hoping to solve
> > the issue?
> >
> > a) inserting calls inside the macros doesn't feel right
> > b) pushing the flush back onto the module author leads to problems
>
> b is not a problem. It is a standard way of doing things. I have never
> seen a buffering API that tries to figure out how to switch from buffered
> to non-buffered without help from the programmer. Why are we trying to?
We aren't trying to solve buffered vs non-buffered. We're trying to solve
the ap_r* problem. Your patch introduces the b-vs-nb discrepancy and imposes
add'l requirements on module authors to keep things working.
But the point is moot for now. At some point, when we add some
bucket/brigade APIs to deal with small writes to a brigade, we can discuss
the implementation strategy then.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> I can certainly raise issues until the patch is applied and even afterwards.
> I'm not trying to change the rules of the game, but I don't see that some of
> the problems are being addressed adequately (so I restated them in a
> (hopefully) clear list). The ordering and large-write issues exist and are
> clearly present; why is it a problem to bring them up?
The problem here is one of priorities. I find it to be much more
important that the solution works for modules other than the core. Mine
does, your's can't.
The ordering isn't a problem with a bit of diligence. The large-write
issue is either solved, or the solution is obvious from my last patch.
> So? There is a lot of code that we didn't have before, yet we still end up
> writing it :-) Lack of something doesn't prove it isn't needed :-)
>
> I could say that we don't have brigades that we can write to, so we
> shouldn't do that :-) [we create and insert buckets instead]
Except, buckets are defined as being read-only, so what you are saying is
buckets are read-only, oh except for this special one that is
read/write. That is a poor API design, and it is inviting problems for
module writers.
> All that aside: if you aren't intending to insert an apr_brigade_flush()
> call into various brigade macros, then just *how* are you hoping to solve
> the issue?
>
> a) inserting calls inside the macros doesn't feel right
> b) pushing the flush back onto the module author leads to problems
b is not a problem. It is a standard way of doing things. I have never
seen a buffering API that tries to figure out how to switch from buffered
to non-buffered without help from the programmer. Why are we trying to?
> > > I suspect that when you go to solve (2), the ap_r* functions are going to
> > > get quite a bit hairier, and the solution to those will be Apache specific
> > > -- exactly what you are lobbying your patch avoids.
> >
> > Please actually look at the code. This has already been solved.
>
> Huh? I haven't seen any handling in your patch for a 100k ap_rwrite. In
> fact, the last patch that you posted corrupts the heap if you do that.
I'll take a closer look, but I don't see that problem. I see a potential
optimization that will remove a single copy, but it is just that an
optimization.
> I will gladly review your solution to large writes, and clear my "large
> write" issue off the table. However, I suspect you're going to end up with a
> good chunk of code in ap_r* to deal with large writes.
I'll have code in about an hour, or I may fix a seg fault before I do
this, I haven't decided yet.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jan 22, 2001 at 07:38:10PM -0800, rbb@covalent.net wrote:
> > > If I missed anybody, please stand up. I would really like to call this a
> > > vote at 8:00 PM PCT. That is about 3hr 20m from now. We have been
> > > holding this vote since last thursday or friday, so it is time we made a
> > > decision already. If anybody thinkgs I should wait longer, I am perfectly
> > > willing to wait until tomorrow morning at 7:00am, or suggest a different
> > > time.
> >
> > Your patch needs to solve these things first:
>
> You can't dictate problems after a vote and decision has been made.
1) we're voting
2) no decision has been made
and:
3) I've been stating the ordering problem since you first posted your patch,
and the large-write issue came up in connection with OtherBill's query.
I can certainly raise issues until the patch is applied and even afterwards.
I'm not trying to change the rules of the game, but I don't see that some of
the problems are being addressed adequately (so I restated them in a
(hopefully) clear list). The ordering and large-write issues exist and are
clearly present; why is it a problem to bring them up?
> > 1) ordering
>...
> > You've said (1) is possible (and you provided the "how") but don't want to
> > do it that way. I agree that calling apr_brigade_flush() inside the brigade
> > macros is bogus. I've also described the tail-bucket approach that will
> > completely eliminate the apr_brigade_flush() problem. If you switch to that,
> > it will completely remove the necessity for brigade_flush.
>
> I completely disagree that we should create a bucket that can be written
> to. We currently do not have any buckets that we can write to. I
So? There is a lot of code that we didn't have before, yet we still end up
writing it :-) Lack of something doesn't prove it isn't needed :-)
I could say that we don't have brigades that we can write to, so we
shouldn't do that :-) [we create and insert buckets instead]
> thoroughly dislike adding a new bucket API that only works in certain
> situations.
IMO, it is an elegant way to solve the problem you're after: small writes
into a brigade (while maintaining ordering within that brigade).
All that aside: if you aren't intending to insert an apr_brigade_flush()
call into various brigade macros, then just *how* are you hoping to solve
the issue?
a) inserting calls inside the macros doesn't feel right
b) pushing the flush back onto the module author leads to problems
[---- for reference, inserting problem (2) here ----]
> > 2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
> > specifically: how will you avoid copying the content into the brigade?
[---- ... ]
> > I suspect that when you go to solve (2), the ap_r* functions are going to
> > get quite a bit hairier, and the solution to those will be Apache specific
> > -- exactly what you are lobbying your patch avoids.
>
> Please actually look at the code. This has already been solved.
Huh? I haven't seen any handling in your patch for a 100k ap_rwrite. In
fact, the last patch that you posted corrupts the heap if you do that.
I will gladly review your solution to large writes, and clear my "large
write" issue off the table. However, I suspect you're going to end up with a
good chunk of code in ap_r* to deal with large writes.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> > If I missed anybody, please stand up. I would really like to call this a
> > vote at 8:00 PM PCT. That is about 3hr 20m from now. We have been
> > holding this vote since last thursday or friday, so it is time we made a
> > decision already. If anybody thinkgs I should wait longer, I am perfectly
> > willing to wait until tomorrow morning at 7:00am, or suggest a different
> > time.
>
> Your patch needs to solve these things first:
You can't dictate problems after a vote and decision has been made.
> 1) ordering
>
> 2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
> specifically: how will you avoid copying the content into the brigade?
>
>
> You've said (1) is possible (and you provided the "how") but don't want to
> do it that way. I agree that calling apr_brigade_flush() inside the brigade
> macros is bogus. I've also described the tail-bucket approach that will
> completely eliminate the apr_brigade_flush() problem. If you switch to that,
> it will completely remove the necessity for brigade_flush.
I completely disagree that we should create a bucket that can be written
to. We currently do not have any buckets that we can write to. I
thoroughly dislike adding a new bucket API that only works in certain
situations.
> I suspect that when you go to solve (2), the ap_r* functions are going to
> get quite a bit hairier, and the solution to those will be Apache specific
> -- exactly what you are lobbying your patch avoids.
Please actually look at the code. This has already been solved.
> Your solutions for the above two problems are needed before applying your
> patch. I don't understand how you intend to solve them; my own thoughts on
> how to do it within your framework lead me to believe that your patch won't
> be as confined to APR as you believe.
>
> Application of either patch should be held until the solution is evident.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> 1) ordering
>
> 2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
> specifically: how will you avoid copying the content into the brigade?
Please cross #2 off this list. The following patch modifies
default_handler to print a string of 100,000 'a's using the ap_write
API. This is done with NO copies, and no heap corruption. The
ap_vrprintf can't be done without copying content easily, but the concept
is there. BTW, this patch fixes all of them, except for ap_vrprintf,
mainly because I didn't want to spend much time on this today. Sorry it
took so long. I had this patch done over an hour ago, but I could't get a
diff. :-(
Ryan
Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.131
diff -u -d -b -w -u -r1.131 httpd.h
--- include/httpd.h 2001/01/20 06:05:14 1.131
+++ include/httpd.h 2001/01/23 05:03:27
@@ -85,6 +85,7 @@
#include "apr_pools.h"
#include "apr_time.h"
#include "apr_network_io.h"
+#include "apr_buckets.h"
#ifdef CORE_PRIVATE
@@ -809,6 +810,8 @@
/** A flag to determine if the eos bucket has been sent yet
* @defvar int eos_sent */
int eos_sent;
+
+ apr_bucket_brigade *bb;
/* Things placed at the end of the record to avoid breaking binary
* compatibility. It would be nice to remember to reorder the entire
Index: modules/http/http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.240
diff -u -d -b -w -u -r1.240 http_core.c
--- modules/http/http_core.c 2001/01/21 22:14:15 1.240
+++ modules/http/http_core.c 2001/01/23 05:03:29
@@ -2948,6 +2948,9 @@
int errstatus;
apr_file_t *fd = NULL;
apr_status_t status;
+ char foobar[100000];
+ int i;
+
/* XXX if/when somebody writes a content-md5 filter we either need to
* remove this support or coordinate when to use the filter vs.
* when to use this code
@@ -3021,6 +3024,7 @@
ap_md5digest(r->pool, fd));
}
+#if 0
bb = apr_brigade_create(r->pool);
e = apr_bucket_create_file(fd, 0, r->finfo.size);
@@ -3029,6 +3033,13 @@
APR_BRIGADE_INSERT_TAIL(bb, e);
return ap_pass_brigade(r->output_filters, bb);
+#endif
+
+ for (i=0;i<100000;i++) {
+ foobar[i] = 'a';
+ }
+ ap_rwrite(foobar, 100000, r);
+ return 0;
}
/*
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.269
diff -u -d -b -w -u -r1.269 http_protocol.c
--- modules/http/http_protocol.c 2001/01/22 21:57:57 1.269
+++ modules/http/http_protocol.c 2001/01/23 05:03:32
@@ -1619,13 +1619,17 @@
static void end_output_stream(request_rec *r)
{
- apr_bucket_brigade *bb;
apr_bucket *b;
- bb = apr_brigade_create(r->pool);
+ if (r->bb) {
+ apr_brigade_flush(r->bb);
+ }
+ else {
+ r->bb = apr_brigade_create(r->pool);
+ }
b = apr_bucket_create_eos();
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
+ APR_BRIGADE_INSERT_TAIL(r->bb, b);
+ ap_pass_brigade(r->output_filters, r->bb);
}
void ap_finalize_sub_req_protocol(request_rec *sub)
@@ -2998,77 +3002,130 @@
AP_DECLARE(int) ap_rputc(int c, request_rec *r)
{
- apr_bucket_brigade *bb = NULL;
apr_bucket *b;
+ apr_status_t status;
char c2 = (char)c;
if (r->connection->aborted) {
return EOF;
}
- bb = apr_brigade_create(r->pool);
- b = apr_bucket_create_transient(&c2, 1);
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
+ }
- return c;
+ b = APR_BRIGADE_LAST(r->bb);
+
+ status = apr_brigade_putc(r->bb, c2);
+
+ /* Okay, so this sucks, but we don't have many options. What we
+ * are doing here is just checking to see if we just converted the
+ * buffer into a new bucket and put it at the end of the brigade. If
+ * we did, we want to pass the brigade to the next filter. If not,
+ * we just keep going. This allows us to use the network to limit how
+ * much data we send at any one time.
+ */
+ if (b != APR_BRIGADE_LAST(r->bb)) {
+ ap_pass_brigade(r->output_filters, r->bb);
+ }
+
+ return status;
}
AP_DECLARE(int) ap_rputs(const char *str, request_rec *r)
{
- apr_bucket_brigade *bb = NULL;
apr_bucket *b;
- apr_size_t len;
+ apr_status_t status;
if (r->connection->aborted)
return EOF;
if (*str == '\0')
return 0;
- len = strlen(str);
- bb = apr_brigade_create(r->pool);
- b = apr_bucket_create_transient(str, len);
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
+ }
- return len;
+ b = APR_BRIGADE_LAST(r->bb);
+
+ status = apr_brigade_puts(r->bb, str);
+
+ /* Okay, so this sucks, but we don't have many options. What we
+ * are doing here is just checking to see if we just converted the
+ * buffer into a new bucket and put it at the end of the brigade. If
+ * we did, we want to pass the brigade to the next filter. If not,
+ * we just keep going. This allows us to use the network to limit how
+ * much data we send at any one time.
+ */
+ if (b != APR_BRIGADE_LAST(r->bb)) {
+ ap_pass_brigade(r->output_filters, r->bb);
}
+ return status;
+}
+
AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r)
{
- apr_bucket_brigade *bb = NULL;
apr_bucket *b;
+ apr_status_t status;
if (r->connection->aborted)
return EOF;
if (nbyte == 0)
return 0;
- bb = apr_brigade_create(r->pool);
- b = apr_bucket_create_transient(buf, nbyte);
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
- return nbyte;
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
+ }
+
+ b = APR_BRIGADE_LAST(r->bb);
+
+ status = apr_brigade_write(r->bb, buf, nbyte);
+
+ /* Okay, so this sucks, but we don't have many options. What we
+ * are doing here is just checking to see if we just converted the
+ * buffer into a new bucket and put it at the end of the brigade. If
+ * we did, we want to pass the brigade to the next filter. If not,
+ * we just keep going. This allows us to use the network to limit how
+ * much data we send at any one time.
+ */
+ if (b != APR_BRIGADE_LAST(r->bb)) {
+ ap_pass_brigade(r->output_filters, r->bb);
}
+ return status;
+}
+
AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
{
- apr_bucket_brigade *bb = NULL;
- apr_size_t written;
+ apr_bucket *b;
+ apr_status_t status;
if (r->connection->aborted)
return EOF;
- bb = apr_brigade_create(r->pool);
- written = apr_brigade_vprintf(bb, fmt, va);
- if (written != 0)
- ap_pass_brigade(r->output_filters, bb);
- return written;
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
}
-/* TODO: Make ap pa_bucket_vprintf that printfs directly into a
- * bucket.
+ b = APR_BRIGADE_LAST(r->bb);
+
+ status = apr_brigade_vprintf(r->bb, fmt, va);
+
+ /* Okay, so this sucks, but we don't have many options. What we
+ * are doing here is just checking to see if we just converted the
+ * buffer into a new bucket and put it at the end of the brigade. If
+ * we did, we want to pass the brigade to the next filter. If not,
+ * we just keep going. This allows us to use the network to limit how
+ * much data we send at any one time.
*/
+ if (b != APR_BRIGADE_LAST(r->bb)) {
+ ap_pass_brigade(r->output_filters, r->bb);
+ }
+
+ return status;
+}
+
AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt, ...)
{
va_list va;
@@ -3086,31 +3143,34 @@
AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r, ...)
{
- apr_bucket_brigade *bb = NULL;
apr_size_t written;
va_list va;
if (r->connection->aborted)
return EOF;
- bb = apr_brigade_create(r->pool);
+
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
+ }
+
va_start(va, r);
- written = apr_brigade_vputstrs(bb, va);
+ written = apr_brigade_vputstrs(r->bb, va);
va_end(va);
- if (written != 0)
- ap_pass_brigade(r->output_filters, bb);
return written;
}
AP_DECLARE(int) ap_rflush(request_rec *r)
{
/* we should be using a flush bucket to flush the stack, not buff code. */
- apr_bucket_brigade *bb;
apr_bucket *b;
- bb = apr_brigade_create(r->pool);
b = apr_bucket_create_flush();
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
+ if (!r->bb) {
+ r->bb = apr_brigade_create(r->pool);
+ }
+ apr_brigade_flush(r->bb);
+ APR_BRIGADE_INSERT_TAIL(r->bb, b);
+ ap_pass_brigade(r->output_filters, r->bb);
return 0;
}
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.43
diff -u -d -b -w -u -r1.43 util_filter.c
--- server/util_filter.c 2001/01/19 07:04:29 1.43
+++ server/util_filter.c 2001/01/23 05:03:33
@@ -237,6 +237,7 @@
*/
next->r->eos_sent = 1;
}
+ apr_brigade_flush(bb);
return next->frec->filter_func.out_func(next, bb);
}
return AP_NOBODY_WROTE;
Index: srclib/apr-util/buckets/apr_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.3
diff -u -d -b -w -u -r1.3 apr_brigade.c
--- srclib/apr-util/buckets/apr_brigade.c 2001/01/19 07:01:59 1.3
+++ srclib/apr-util/buckets/apr_brigade.c 2001/01/23 05:03:46
@@ -99,6 +99,9 @@
b = apr_palloc(p, sizeof(*b));
b->p = p;
+
+ b->start = b->end = NULL;
+
APR_RING_INIT(&b->list, apr_bucket, link);
apr_register_cleanup(b->p, b, apr_brigade_cleanup, apr_brigade_cleanup);
@@ -185,12 +188,41 @@
return vec - orig;
}
+static int check_brigade_flush(char *start, char **end, const char *str,
+ apr_size_t *n, apr_bucket_brigade *b)
+{
+ if (*n > (APR_BUCKET_BUFF_SIZE - (*end - start))) {
+ if (start) {
+ memcpy(*end, str, APR_BUCKET_BUFF_SIZE - (*end - start));
+ *n -= (APR_BUCKET_BUFF_SIZE - (*end - start));
+ *end += (APR_BUCKET_BUFF_SIZE - (*end - start));
+ apr_brigade_flush(b);
+ }
+ if (*n > APR_BUCKET_BUFF_SIZE) {
+ apr_bucket *e = apr_bucket_create_transient(str, *n);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ return 1;
+ }
+ }
+ return 0;
+}
+
+APU_DECLARE(void) apr_brigade_flush(apr_bucket_brigade *b)
+{
+ apr_bucket *e;
+ apr_size_t i = b->end - b->start;
+
+ if (i) {
+ e = apr_bucket_create_heap(b->start, i, 0, NULL);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ b->start = b->end = NULL;
+ }
+}
+
APU_DECLARE(int) apr_brigade_vputstrs(apr_bucket_brigade *b, va_list va)
{
- apr_bucket *r;
const char *x;
int j, k;
- apr_size_t i;
for (k = 0;;) {
x = va_arg(va, const char *);
@@ -198,20 +230,52 @@
break;
j = strlen(x);
- /* XXX: copy or not? let the caller decide? */
- r = apr_bucket_create_heap(x, j, 1, &i);
- if (i != j) {
- /* Do we need better error reporting? */
- return -1;
+ k += apr_brigade_write(b, x, j);
}
- k += i;
+ return k;
+}
- APR_BRIGADE_INSERT_TAIL(b, r);
+APU_DECLARE(int) apr_brigade_putc(apr_bucket_brigade *b, const char c)
+{
+ apr_size_t nbyte = 1;
+
+ if (check_brigade_flush(b->start, &b->end, &c, &nbyte, b)) {
+ return 1;
}
- return k;
+ if (!b->start) {
+ b->start = b->end = malloc(APR_BUCKET_BUFF_SIZE);
+ }
+
+ memcpy(b->end, &c, 1);
+ b->end++;
+
+ return 1;
+}
+
+APU_DECLARE(int) apr_brigade_write(apr_bucket_brigade *b, const char *str, apr_size_t nbyte)
+{
+ apr_size_t n = nbyte;
+
+ if (check_brigade_flush(b->start, &b->end, str, &nbyte, b)) {
+ return n;
+ }
+
+ if (!b->start) {
+ b->start = b->end = malloc(APR_BUCKET_BUFF_SIZE);
+ }
+
+ memcpy(b->end, str, nbyte);
+ b->end += nbyte;
+
+ return n;
}
+APU_DECLARE(int) apr_brigade_puts(apr_bucket_brigade *b, const char *str)
+{
+ return apr_brigade_write(b, str, strlen(str));
+}
+
APU_DECLARE_NONSTD(int) apr_brigade_putstrs(apr_bucket_brigade *b, ...)
{
va_list va;
@@ -240,13 +304,8 @@
* directly into a bucket. I'm being lazy right now. RBB
*/
char buf[4096];
- apr_bucket *r;
- int res;
-
- res = apr_vsnprintf(buf, 4096, fmt, va);
- r = apr_bucket_create_heap(buf, strlen(buf), 1, NULL);
- APR_BRIGADE_INSERT_TAIL(b, r);
+ apr_vsnprintf(buf, 4096, fmt, va);
- return res;
+ return apr_brigade_puts(b, buf);
}
Index: srclib/apr-util/include/apr_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.65
diff -u -d -b -w -u -r1.65 apr_buckets.h
--- srclib/apr-util/include/apr_buckets.h 2001/01/22 23:11:32 1.65
+++ srclib/apr-util/include/apr_buckets.h 2001/01/23 05:03:47
@@ -78,6 +78,8 @@
* @package Bucket Brigades
*/
+#define APR_BUCKET_BUFF_SIZE 9000
+
typedef enum {APR_BLOCK_READ, APR_NONBLOCK_READ} apr_read_type_e;
/*
@@ -237,6 +239,8 @@
* the destroying function is responsible for killing the cleanup.
*/
apr_pool_t *p;
+ char *start;
+ char *end;
/** The buckets in the brigade are on this list. */
/*
* XXX: the apr_bucket_list structure doesn't actually need a name tag
@@ -628,9 +632,10 @@
APU_DECLARE(int) apr_brigade_to_iovec(apr_bucket_brigade *b,
struct iovec *vec, int nvec);
+APU_DECLARE(void) apr_brigade_flush(apr_bucket_brigade *b);
+
/**
- * This function writes a list of strings into a bucket brigade. We just
- * allocate a new heap bucket for each string.
+ * This function writes a list of strings into a bucket brigade.
* @param b The bucket brigade to add to
* @param va A list of strings to add
* @return The number of bytes added to the brigade
@@ -639,8 +644,34 @@
APU_DECLARE(int) apr_brigade_vputstrs(apr_bucket_brigade *b, va_list va);
/**
+ * This function writes an string into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param str The string to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_write(ap_bucket_brigade *b, const char *str)
+ */
+APU_DECLARE(int) apr_brigade_write(apr_bucket_brigade *b, const char *str, apr_size_t nbyte);
+
+/**
+ * This function writes an string into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param str The string to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_puts(ap_bucket_brigade *b, const char *str)
+ */
+APU_DECLARE(int) apr_brigade_puts(apr_bucket_brigade *b, const char *str);
+
+/**
+ * This function writes a character into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param c The character to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_putc(apr_bucket_brigade *b, const char c)
+ */
+APU_DECLARE(int) apr_brigade_putc(apr_bucket_brigade *b, const char c);
+
+/**
* This function writes an unspecified number of strings into a bucket brigade.
- * We just allocate a new heap bucket for each string.
* @param b The bucket brigade to add to
* @param ... The strings to add
* @return The number of bytes added to the brigade
@@ -649,7 +680,7 @@
APU_DECLARE_NONSTD(int) apr_brigade_putstrs(apr_bucket_brigade *b, ...);
/**
- * Evaluate a printf and put the resulting string into a bucket at the end
+ * Evaluate a printf and put the resulting string at the end
* of the bucket brigade.
* @param b The brigade to write to
* @param fmt The format of the string to write
@@ -661,7 +692,7 @@
const char *fmt, ...);
/**
- * Evaluate a printf and put the resulting string into a bucket at the end
+ * Evaluate a printf and put the resulting string at the end
* of the bucket brigade.
* @param b The brigade to write to
* @param fmt The format of the string to write
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jan 22, 2001 at 04:42:12PM -0800, rbb@covalent.net wrote:
>...
> If I missed anybody, please stand up. I would really like to call this a
> vote at 8:00 PM PCT. That is about 3hr 20m from now. We have been
> holding this vote since last thursday or friday, so it is time we made a
> decision already. If anybody thinkgs I should wait longer, I am perfectly
> willing to wait until tomorrow morning at 7:00am, or suggest a different
> time.
Your patch needs to solve these things first:
1) ordering
2) large writes in ap_rputs, ap_rvputs, ap_rwrite, and ap_vrprintf
specifically: how will you avoid copying the content into the brigade?
You've said (1) is possible (and you provided the "how") but don't want to
do it that way. I agree that calling apr_brigade_flush() inside the brigade
macros is bogus. I've also described the tail-bucket approach that will
completely eliminate the apr_brigade_flush() problem. If you switch to that,
it will completely remove the necessity for brigade_flush.
[ it *does* leave the ap_sync_output issue to deal with, tho; I don't have
an answer for that one except to provide my patch. ]
I suspect that when you go to solve (2), the ap_r* functions are going to
get quite a bit hairier, and the solution to those will be Apache specific
-- exactly what you are lobbying your patch avoids.
Your solutions for the above two problems are needed before applying your
patch. I don't understand how you intend to solve them; my own thoughts on
how to do it within your framework lead me to believe that your patch won't
be as confined to APR as you believe.
Application of either patch should be held until the solution is evident.
Thanks,
-g
--
Greg Stein, http://www.lyra.org/
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> > greg's patch: gstein, gregames
> > rbb's patch: rbb, dougm?, jtrawick?
> >
> > I am including Jeff because he said a few days ago he was leaning toward
> > my patch. I have put a ? by his name, because he hasn't really voted yet,
> > so I am not really counting him. That is more of a poke to get him to
> > respond. Jeff????
>
> You missed my hand yesterday. Yes, Greg has several potentially valid
> points, but I remain convinced that we need to continue to build upon apr
> where it's truly useful outside of apr. This is.
Okay, that changes this to:
greg's patch: gstein, gregames
rbb's patch: rbb, dougm, wrowe, jtrawick?
If I missed anybody, please stand up. I would really like to call this a
vote at 8:00 PM PCT. That is about 3hr 20m from now. We have been
holding this vote since last thursday or friday, so it is time we made a
decision already. If anybody thinkgs I should wait longer, I am perfectly
willing to wait until tomorrow morning at 7:00am, or suggest a different
time.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: <rb...@covalent.net>
Sent: Monday, January 22, 2001 5:47 PM
> So far I have:
>
> greg's patch: gstein, gregames
> rbb's patch: rbb, dougm?, jtrawick?
>
> I am including Jeff because he said a few days ago he was leaning toward
> my patch. I have put a ? by his name, because he hasn't really voted yet,
> so I am not really counting him. That is more of a poke to get him to
> respond. Jeff????
You missed my hand yesterday. Yes, Greg has several potentially valid
points, but I remain convinced that we need to continue to build upon apr
where it's truly useful outside of apr. This is.
Re: ap_r* performance patch
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 22 Jan 2001 rbb@covalent.net wrote:
> So far you have it 99% correct. The one thing that everybody keeps
> leaving out, is that I have detailed how to avoid that ap_brigade_flush
> 99.9% of the time. I personally disagree that is a good idea, but I am
> willing to be persuaded.
ok, having to call it or not isn't too important to me.
> Does this mean that you are voting for my patch?
yes, +1
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> > Yeah, but you were strictly testing a handler. Try doing that with a
> > filter and Greg's patch. The filter is registered as an
> > FTYPE_CONTENT + 1, so it MUST be at the very top of the filter stack, it
> > is not possible to insert it after a mod_perl filter.
>
> so this means i'd still have to use the modperl buffer thingy.
> and i would have to use it for connection-level filters (where there is no
> r or r->output_filters). but with rbb.patch i can use apr_brigade_*
> anywhere. ap_r* improvements are not useful to me if i can't throw away
> the current modperl buffer api.
Exactly!
> seems to me that:
> - rbb's design is more flexable/usable
> - both are very close performance wise
> - rbb's might require a call to ap_sync_output() or apr_brigade_flush()
>
> if i'm following correctly, the last is greg's major argument against
> rbb's, is this still true? personally, i don't see the big deal with
> having to call a flush or sync function. yeah, it might end up biting
> somebody, but that's not enough imho to sacrifice flexabilty.
So far you have it 99% correct. The one thing that everybody keeps
leaving out, is that I have detailed how to avoid that ap_brigade_flush
99.9% of the time. I personally disagree that is a good idea, but I am
willing to be persuaded.
> the more i think about it, the more i really like apr_brigade_*
> being usable outside of the request_rec and outside of httpd. libapr is
> rad, i've used it to write 3 libraries so far that work outside of httpd,
> an http/s client, rewrite of cybersource's client library and a generic
> credit card clearing house api. filtering in the http/s client would be
> useful and i could use apr_brigade_* instead of BIO_* in the cybersource
> client.
Does this mean that you are voting for my patch?
Just trying to get a tally.
So far I have:
greg's patch: gstein, gregames
rbb's patch: rbb, dougm?, jtrawick?
I am including Jeff because he said a few days ago he was leaning toward
my patch. I have put a ? by his name, because he hasn't really voted yet,
so I am not really counting him. That is more of a poke to get him to
respond. Jeff????
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 22 Jan 2001 rbb@covalent.net wrote:
> Yeah, but you were strictly testing a handler. Try doing that with a
> filter and Greg's patch. The filter is registered as an
> FTYPE_CONTENT + 1, so it MUST be at the very top of the filter stack, it
> is not possible to insert it after a mod_perl filter.
so this means i'd still have to use the modperl buffer thingy.
and i would have to use it for connection-level filters (where there is no
r or r->output_filters). but with rbb.patch i can use apr_brigade_*
anywhere. ap_r* improvements are not useful to me if i can't throw away
the current modperl buffer api.
seems to me that:
- rbb's design is more flexable/usable
- both are very close performance wise
- rbb's might require a call to ap_sync_output() or apr_brigade_flush()
if i'm following correctly, the last is greg's major argument against
rbb's, is this still true? personally, i don't see the big deal with
having to call a flush or sync function. yeah, it might end up biting
somebody, but that's not enough imho to sacrifice flexabilty.
the more i think about it, the more i really like apr_brigade_*
being usable outside of the request_rec and outside of httpd. libapr is
rad, i've used it to write 3 libraries so far that work outside of httpd,
an http/s client, rewrite of cybersource's client library and a generic
credit card clearing house api. filtering in the http/s client would be
useful and i could use apr_brigade_* instead of BIO_* in the cybersource
client.
Re: ap_r* performance patch
Posted by rb...@covalent.net.
On Mon, 22 Jan 2001, Doug MacEachern wrote:
> On Mon, 22 Jan 2001 rbb@covalent.net wrote:
>
> > Oh BTW, mod_perl and mod_snake have already written the buffering logic
> > for themselves. My patch allows them to remove that logic, AFAICT, Greg's
> > does not.
>
> the tests i ran yesterday (with greg.patch and rbb.patch) both used
> ap_rwrite() instead of the modperl buffer thingy.
Yeah, but you were strictly testing a handler. Try doing that with a
filter and Greg's patch. The filter is registered as an
FTYPE_CONTENT + 1, so it MUST be at the very top of the filter stack, it
is not possible to insert it after a mod_perl filter.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 22 Jan 2001 rbb@covalent.net wrote:
> Oh BTW, mod_perl and mod_snake have already written the buffering logic
> for themselves. My patch allows them to remove that logic, AFAICT, Greg's
> does not.
the tests i ran yesterday (with greg.patch and rbb.patch) both used
ap_rwrite() instead of the modperl buffer thingy.
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> The generality thing is nice but not a huge deal to me, because AFAIK we
> have nobody else asking for/needing buffering outside of ap_r* today. I
> do like how gstein caught the out-of-order data problem and provided a
> fix before it bit us.
Oh BTW, mod_perl and mod_snake have already written the buffering logic
for themselves. My patch allows them to remove that logic, AFAICT, Greg's
does not.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by rb...@covalent.net.
> Differences:
>
> o Generality
>
> * Victor's patch only buffers the Apache ap_r* APIs.
> * Greg's patch could be used by other users of Apache filters
This is not true. Because of how the filter is declared, it can only be
used by ap_r* functions. Making it any other way enhances the chance that
the filter will not be in the right place for ap_r*.
> o Out-of-order data problem with mixed APIs
>
> * Greg's patch is designed to accomodate mixing ap_r* and bucket
> brigade API calls without relying on the module explicitly flushing the
> buffer.
> * Victor and Ryan's patches both require explicit flushes.
This too is untrue. I have already explained how this could be done
without an explicit call.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: ap_r* performance patch
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
>
> We have two basic patches that have been posted to the list,
Actually, we have three patches...Victor's came first, and is a good
starting point for comparing designs. I'm pretty pleased with all of
them, so IMO we are talking details.
> and performance for both is basically equivalent.
yup...I'm happy with the overall performance of all three. All could
probably be scrubbed later, no big deal. And this patch battle has been
pretty civilized compared to the War of the Filters.
Here's how I compare the patches:
Common features:
* the ap_r* APIs are unchanged from 1.3 .
* small data is buffered with a small number of cycles burned in the
usual case.
* virtual memory usage is limited by the size of the buffer, and by
network back pressure for slow links/clients
Differences:
o Generality
* Victor's patch only buffers the Apache ap_r* APIs.
* Greg's patch could be used by other users of Apache filters
* Ryan's patch could be used by other users of APR buckets/brigades
o Out-of-order data problem with mixed APIs
* Greg's patch is designed to accomodate mixing ap_r* and bucket
brigade API calls without relying on the module explicitly flushing the
buffer.
* Victor and Ryan's patches both require explicit flushes.
The generality thing is nice but not a huge deal to me, because AFAIK we
have nobody else asking for/needing buffering outside of ap_r* today. I
do like how gstein caught the out-of-order data problem and provided a
fix before it bit us.
>
> so, we have basically two patches:
>
> [ ] ap_brigade_* functions (rbb's)
> [X] old_write_filter (gstein's)
>
Greg Ames