httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <>
Subject Re: ap_r* performance patch
Date Tue, 23 Jan 2001 03:53:18 GMT
On Mon, Jan 22, 2001 at 07:38:10PM -0800, 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


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.


Greg Stein,

View raw message