[PATCH] updated ap_r* patch

[Greg Stein <gs...@lyra.org>]

Attached is an updated version of my patch to fix ap_r* performance.

- fixed bug found by Doug
- small tweak for clarify from Greg Marr
- updated against most recent CVS

Cheers,
-g

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

Re: [PATCH] updated ap_r* patch

[Greg Stein <gs...@lyra.org>]

On Mon, Jan 22, 2001 at 02:59:48PM -0500, Jeff Trawick wrote:
> Greg Stein <gs...@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/

Re: [PATCH] updated ap_r* patch

[Jeff Trawick <tr...@bellsouth.net>]

Greg Stein <gs...@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

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

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;

-- 
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: [PATCH] updated ap_r* patch

[Doug MacEachern <do...@covalent.net>]

cool, bug appears gone.  i will re-run benchmarks later if i make it back
in one piece from the fear factory show.