httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject Re: [PATCH] ap_r* performance patch
Date Fri, 19 Jan 2001 14:44:34 GMT

> > I would expect the output to be:
> > 
> > 	mumble foobar baz
> Not me. Brigades hold data. They aren't an output mechanism. Never have
> been, and never will be. The data that you accumulate into a brigade is
> completely separate from everything else, until you call a function to
> *send* that data.

That's fine, that isn't what you expect, but you said that bucket brigades
and ap_r* were going to work seamlessly together.  _I_ expected exactly
what I posted would work.  I have a feeling that if you tell module
authors that the two work seamlessly, they will expect the same thing.

> Nope. I never expect insertion-into-a-brigade to actually deliver data to
> the client. I have no idea where that idea came from.

It came from my understanding of what you were saying.  If I (as somebody
who is in the code everyday) misunderstood, what at the chances that
module authors will too?

> > Now, take the same idea in my patch:
> > 
> > b = ap_bucket_create_heap(..., "mumble", ...);
> Wait a sec. You're using r->bb. You're comparing apples to oranges. If you
> want an accurate and fair comparison, then your code should *also*
> manipulate "bb" rather than "r->bb".

This is an accurate and fair comparison.  With the shift I made to put a
brigade in the request_rec, this is how handlers would work.  We still
have to create it the same as we do in your patch, it is just stored
someplace else.

I guess you could do the same thing pretty easily, I hadn't really thought
that through last night.

> > I am also assuming that ap_vrprintf would be re-written to not copy the
> > data before passing it to buffer_output.
> Absolutely. ap_vrprintf() and apr_bucket_vprintf() will both need to be
> rewritten at some point to properly deal with more than 4K of data.
> At the moment, nobody actually uses apr_brigade_vprintf. If we use your
> patch, then we schedule a fix for that. If we don't use your patch, then we
> can decide to keep the function and fix it, or simply toss it.
> > I'm not really sure how you
> > would do that, but I am assuming you have some thoughts.
> Yes. We fix it the same way for both functions. Use the apr_vformatter
> function and hook up the flush_func to call buffer_output or to insert the
> value into a brigade.

But isn't that two copies?  apr_vformatter makes a copy, and then you will
have another copy in the old_write_filter functions (I lumped them all
together here).  If you just insert the bucket into the brigade, then
haven't you lost the coalesce features?  I guess you could create a new
function like flush_buffer, that does an apr_vformatter under the
covers.  I would need to really look at how this would be done.  This is
just a pointer to say "I don't see this", not a request for a change.

> >...
> > Also, this isn't a general fix, so it only works for Apache,
> It is a fix for our problems with the legacy ap_r* interface. It fixes it
> quite well :-)

I just thought of something else.  Mod_perl 2.0 has it's own set of
functions that basically do exactly what the ap_brigade_* functions
do.  This patch doesn't help modules like that at all.  So, now there is
duplicate coalesce code, one in the server and one in mod_perl.  I am
wondering if mod_perl could insert a copy of old_write_filter so that we
can remove some duplication there.

I am very unlikely to modify my patch at this point.  Not because I don't
think it is a useful patch, but because I have shown the concept.  If I
modify my patch to cover the details, then we are just going back and
forth on patches again.  The concepts are all in the patch, please review
the concepts, we can get the details perfect later.  I am not too
concerned about separating concept from detail here, since the prototype
has already proven itself.


Ryan Bloom               
406 29th St.
San Francisco, CA 94131

View raw message