Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
Precedence: bulk
Reply-To: new-httpd@apache.org
Date: Thu, 18 Jan 2001 22:36:12 -0800 (PST)
From: rbb@covalent.net
To: new-httpd@apache.org
Subject: Re: [PATCH] ap_r* performance patch
In-Reply-To: <20010118191701.Q7731@lyra.org>
Message-ID: <Pine.LNX.4.21.0101182212350.12850-100000@koj>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII


I have begun to review the patch, and I disagree that this solves the
problem of mixing the two APIs.  Take this code snippet (ignore the
bucket type, just look at the concept, any bucket type will have the same 
problem):

b = ap_bucket_create_heap(..., "mumble", ...);
AP_BRIGADE_INSERT_TAIL(bb, b);

ap_rputs(r, " foobar ");

b = ap_bucket_create_heap(..., "baz", ...);
AP_BRIGADE_INSERT_TAIL(bb, b);

ap_pass_brigade(r->output_filters, bb);

I would expect the output to be:

	mumble foobar baz

But it's going to be:

	foobar mumble baz

The problem is that there is nothing in this API to mark the distinction
between the bucket_brigade and the request API's.  Just move the ap_rputs
call into a separate function, and you have the exact case that you
described earlier today.

Now, take the same idea in my patch:

b = ap_bucket_create_heap(..., "mumble", ...);
AP_BRIGADE_INSERT_TAIL(r->bb, b);

ap_rputs(r, " foobar ");

ap_brigade_standardize(r->bb);   /* Could be removed, see below */

b = ap_bucket_create_heap(..., "baz", ...);
AP_BRIGADE_INSERT_TAIL(r->bb, b);

ap_pass_brigade(r->output_filters, bb);

This code does give:

	mumble foobar baz

The standardize call marks the difference between the two APIs.  However,
if you absolutely must, we could actually remove that call (I'm not
writing that code snippet, just remove that line.  :-)

The standardize call is removed by adding a function call to some of the
AP_BRIGADE_* macros.  I am currently thinking of AP_BRIGADE_INSERT_TAIL
and AP_BRIGADE_CONCATENATE.  It needs to be any macro that inserts a
bucket at the end of the brigade, those are the two I have seen.

Basically, it changes AP_BRIGADE_INSERT_TAIL from:

#define AP_BRIGADE_INSERT_TAIL(b, e)                                    \
        AP_RING_INSERT_TAIL(&(b)->list, (e), ap_bucket, link)

to:

#define AP_BRIGADE_INSERT_TAIL(b, e)                                    \
	ap_brigade_standardize(b)					\
        AP_RING_INSERT_TAIL(&(b)->list, (e), ap_bucket, link)

Now, I really dislike this.  I want people to realize that they are switch
APIs, because it forces them to consider what they are doing.

I am also assuming that ap_vrprintf would be re-written to not copy the
data before passing it to buffer_output.  I'm not really sure how you
would do that, but I am assuming you have some thoughts.  If not, then for
any ap_rprintf or ap_rvprintf call, you are copying twice before passing
the data to the filter stack, which I really dislike.

IMHO this patch has not solved the problem you were most interested in
solving, and it hides it in such a way that it is more difficult to find
and fix.  Also, this isn't a general fix, so it only works for Apache,
which means that it basically needs to be duplicated in every bucket
program, and potentially every protocol module (still need to finish
abstracting some stuff out of http_protocol).

I look forward to hearing your thoughts on this, this idea is very
interesting, and it is VERY different than what I originally envisioned
when you first posted what you were thinking.

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------