httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] updated ap_r* patch
Date Mon, 22 Jan 2001 20:51:31 GMT
On Mon, Jan 22, 2001 at 02:59:48PM -0500, Jeff Trawick wrote:
> Greg Stein <gstein@lyra.org> writes:
> 
> > Attached is an updated version of my patch to fix ap_r* performance.
> 
> in ap_rputs(), I don't see why we need to check for *str == '\0'...
> this is a rare occurence and is checked for again in buffer_output()
> anyway

It was there to start with, and I just left it to avoid a function call. At
one point, I considered whether to have the check in buffer_output(), but
resolved that would not be as safe if I were to remove it. So, I left the
buffer_output check and left the ap_rputs check (for safety and for speed,
respectively). The buffer_output check is primarily used for the ap_rvputs
and ap_rwrite cases. +0 for removing the ap_rputs check.

> in buffer_output...  can you use the core's module config for this
> request where you say "record some flags in the request_rec..." ?

Kind of. I looked at doing exactly that, but the problem is that
r->request_config is indexed by (say) core_module. Thus, any data in there
is accessible across all of the core module. Not necessarily that big of a
deal, but I wasn't entirely sure that I wanted to open it like that (IOW,
keeping it very private to the OLD_WRITE filter was "nice").

Then again, it would behoove us to formalize the thing and shove a ton of
gunk into there to remove it from the request_rec public API.

[ and to fix the name... it has nothing to do with "config" ]

The "most private" mechanism is to use a userdata-based structure. But I
wasn't up for looking at the exact performance implications of looking up
that data, so I just punted for now with a note about futures.

[ request_config is way fast; I think it is even imp'd as a macro ]

> ap_r* functions are not consistent on reporting a buffer_output()
> failure
> 
> e.g., ap_rputs() needs something like
> 
>   if (buffer_output() == APR_SUCCESS) {
>     return len;
>   else
>     return -1;

Good point. I was retaining the [lack of] error reporting from the
"original" brigade solution, and didn't reflect on improving the error
handling. I'm definitely +1 on fixing that stuff before applying the patch
(and volunteer to do so).

[ I can't think of anybody that checks those return values, but we may as
  well implement the doc'd semantics (just in case) ]

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message