You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/02/12 19:17:03 UTC

[PATCH] final part of the ap_r* solution

This is the final portion of the ap_r* patch.  All this does is remove the
OLD_WRITE filter, and replace the ap_r* calls with macros that use tha
ap_f* calls.  I would really like to finish this solution finally.

The only argument that I have heard against this patch, is that it allows
somebody who is using two different brigades to get the data out of
order.  In order for this to happen, somebody has to use both the ap_r*
calls and the native brigade calls, and they have to use some brigade
other than r->bb.

The other option is to re-write the OLD_WRITE filter to use the ap_f*
calls.  I dislike this option, because it means that we are solving the
same problem in two ways (brigade buffering and OLD_WRITE), and it takes
the control away from the module author.

If we re-write ap_r* to use ap_f*, then the module author has complete
control over when this does and does not work.  It is very easy to
document the one failure condition described above.  The failure in this
case is the data gets out of order.

If we continue to use the OLD_WRITE filter, then the failure condition
becomes that my handler is written correctly, but some other module has
inserted it's filter in front of the OLD_WRITE filter.  This takes control
away from the module author, and puts it in the hands of any other 3rd
party module.  The failure in this case is that the data is sent in a lot
of very small packets

The two failure conditions are different, and it can be argued which is
more important.  Can we please just come to a decision?

Ryan

Index: include/http_protocol.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/http_protocol.h,v
retrieving revision 1.52
diff -u -d -b -w -u -r1.52 http_protocol.h
--- include/http_protocol.h	2001/02/09 07:17:52	1.52
+++ include/http_protocol.h	2001/02/12 17:52:15
@@ -310,7 +310,11 @@
  * @return The number of bytes sent
  * @deffunc int ap_rputc(int c, request_rec *r)
  */
-AP_DECLARE(int) ap_rputc(int c, request_rec *r);
+#define ap_rputc(c, r) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fputc(r->output_filters, r->bb, c)
 /**
  * Output a string for the current request
  * @param str The string to output
@@ -318,7 +322,11 @@
  * @return The number of bytes sent
  * @deffunc int ap_rputs(const char *str, request_rec *r)
  */
-AP_DECLARE(int) ap_rputs(const char *str, request_rec *r);
+#define ap_rputs(str, r) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fputs(r->output_filters, r->bb, str)
 /**
  * Write a buffer for the current request
  * @param buf The buffer to write
@@ -327,7 +335,11 @@
  * @return The number of bytes sent
  * @deffunc int ap_rwrite(const void *buf, int nbyte, request_rec *r)
  */
-AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r);
+#define ap_rwrite(str, n, r) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fwrite(r->output_filters, r->bb, str, n)
 /**
  * Write an unspecified number of strings to the request
  * @param r The current request
@@ -335,7 +347,11 @@
  * @return The number of bytes sent
  * @deffunc int ap_rvputs(request_rec *r, ...)
  */
-AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r,...);
+#define ap_rvputs(r, args...) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fputstrs(r->output_filters, r->bb, ##args)
 /**
  * Output data to the client in a printf format
  * @param r The current request
@@ -344,7 +360,11 @@
  * @return The number of bytes sent
  * @deffunc int ap_vrprintf(request_rec *r, const char *fmt, va_list vlist)
  */
-AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list vlist);
+#define ap_vrprintf(r, fmt, vlist) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_vfprintf(r->output_filters, r->bb, fmt, vlist)
 /**
  * Output data to the client in a printf format
  * @param r The current request
@@ -353,15 +373,22 @@
  * @return The number of bytes sent
  * @deffunc int ap_rprintf(request_rec *r, const char *fmt, ...)
  */
-AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt,...)
-				__attribute__((format(printf,2,3)));
+#define ap_rprintf(r, fmt, args...) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fprintf(r->output_filters, r->bb, fmt, ##args)
 /**
  * Flush all of the data for the current request to the client
  * @param r The current request
  * @return The number of bytes sent
  * @deffunc int ap_rflush(request_rec *r)
  */
-AP_DECLARE(int) ap_rflush(request_rec *r);
+#define ap_rflush(r) \
+    if (!r->bb) { \
+        r->bb = apr_brigade_create(r->pool); \
+    } \
+    ap_fflush(r->output_filters, r->bb)
 
 /**
  * Index used in custom_responses array for a specific error code
Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.136
diff -u -d -b -w -u -r1.136 httpd.h
--- include/httpd.h	2001/02/12 15:44:36	1.136
+++ include/httpd.h	2001/02/12 17:52:15
@@ -82,6 +82,7 @@
 #include "apr_pools.h"
 #include "apr_time.h"
 #include "apr_network_io.h"
+#include "apr_buckets.h"
 
 #include "pcreposix.h"
 
@@ -751,6 +752,8 @@
     /** A flag to determine if the eos bucket has been sent yet
      *  @defvar int eos_sent */
     int eos_sent;
+
+    apr_bucket_brigade *bb;
 
 /* Things placed at the end of the record to avoid breaking binary
  * compatibility.  It would be nice to remember to reorder the entire
Index: modules/http/http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.258
diff -u -d -b -w -u -r1.258 http_core.c
--- modules/http/http_core.c	2001/02/11 00:10:30	1.258
+++ modules/http/http_core.c	2001/02/12 17:52:32
@@ -87,7 +87,6 @@
 
 #include "mod_core.h"
 
-
 /* LimitXMLRequestBody handling */
 #define AP_LIMIT_UNSET                  ((long) -1)
 #define AP_DEFAULT_LIMIT_XML_BODY       ((size_t)1000000)
@@ -3586,8 +3585,6 @@
                               AP_FTYPE_CONTENT);
     ap_register_output_filter("CHUNK", chunk_filter, AP_FTYPE_TRANSCODE);
     ap_register_output_filter("COALESCE", coalesce_filter, AP_FTYPE_CONTENT);
-    ap_register_output_filter("OLD_WRITE", ap_old_write_filter,
-                              AP_FTYPE_CONTENT - 1);
 }
 
 AP_DECLARE_DATA module core_module = {
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.296
diff -u -d -b -w -u -r1.296 http_protocol.c
--- modules/http/http_protocol.c	2001/02/11 01:09:02	1.296
+++ modules/http/http_protocol.c	2001/02/12 17:52:33
@@ -1580,13 +1580,15 @@
 
 static void end_output_stream(request_rec *r)
 {
-    apr_bucket_brigade *bb;
     apr_bucket *b;
 
-    bb = apr_brigade_create(r->pool);
+    if (!r->bb) {
+        r->bb = apr_brigade_create(r->pool);
+    }
+
     b = apr_bucket_eos_create();
-    APR_BRIGADE_INSERT_TAIL(bb, b);
-    ap_pass_brigade(r->output_filters, bb);
+    APR_BRIGADE_INSERT_TAIL(r->bb, b);
+    ap_pass_brigade(r->output_filters, r->bb);
 }
 
 void ap_finalize_sub_req_protocol(request_rec *sub)
@@ -2931,244 +2933,6 @@
     return mm->size; /* XXX - change API to report apr_status_t? */
 }
 #endif /* APR_HAS_MMAP */
-
-typedef struct {
-    char *buf;
-    char *cur;
-    apr_size_t avail;
-} old_write_filter_ctx;
-
-AP_CORE_DECLARE_NONSTD(apr_status_t) ap_old_write_filter(
-    ap_filter_t *f, apr_bucket_brigade *bb)
-{
-    old_write_filter_ctx *ctx = f->ctx;
-
-    AP_DEBUG_ASSERT(ctx);
-
-    if (ctx->buf != NULL) {
-        apr_size_t nbyte = ctx->cur - ctx->buf;
-
-        if (nbyte != 0) {
-            /* whatever is coming down the pipe (we don't care), we
-               can simply insert our buffered data at the front and
-               pass the whole bundle down the chain. */
-            apr_bucket *b = apr_bucket_heap_create(ctx->buf, nbyte, 0, NULL);
-            APR_BRIGADE_INSERT_HEAD(bb, b);
-            ctx->buf = NULL;
-        }
-    }
-
-    return ap_pass_brigade(f->next, bb);
-}
-
-static apr_status_t flush_buffer(request_rec *r, old_write_filter_ctx *ctx,
-                                 const char *extra, apr_size_t extra_len)
-{
-    apr_bucket_brigade *bb = apr_brigade_create(r->pool);
-    apr_size_t nbyte = ctx->cur - ctx->buf;
-    apr_bucket *b = apr_bucket_heap_create(ctx->buf, nbyte, 0, NULL);
-
-    APR_BRIGADE_INSERT_TAIL(bb, b);
-    ctx->buf = NULL;
-
-    /* if there is extra data, then send that, too */
-    if (extra != NULL) {
-        b = apr_bucket_transient_create(extra, extra_len);
-        APR_BRIGADE_INSERT_TAIL(bb, b);
-    }
-
-    return ap_pass_brigade(r->output_filters, bb);
-}
-
-static apr_status_t buffer_output(request_rec *r,
-                                  const char *str, apr_size_t len)
-{
-    ap_filter_t *f;
-    old_write_filter_ctx *ctx;
-    apr_size_t amt;
-    apr_status_t status;
-
-    if (len == 0)
-        return APR_SUCCESS;
-
-    /* ### future optimization: record some flags in the request_rec to
-       ### say whether we've added our filter, and whether it is first. */
-
-    /* this will typically exit on the first test */
-    for (f = r->output_filters; f != NULL; f = f->next)
-        if (strcmp("OLD_WRITE", f->frec->name) == 0)
-            break;
-    if (f == NULL) {
-        /* our filter hasn't been added yet */
-        ctx = apr_pcalloc(r->pool, sizeof(*ctx));
-        ap_add_output_filter("OLD_WRITE", ctx, r, r->connection);
-    }
-
-    /* if the first filter is not our buffering filter, then we have to
-       deliver the content through the normal filter chain */
-    if (strcmp("OLD_WRITE", r->output_filters->frec->name) != 0) {
-        apr_bucket_brigade *bb = apr_brigade_create(r->pool);
-        apr_bucket *b = apr_bucket_transient_create(str, len);
-        APR_BRIGADE_INSERT_TAIL(bb, b);
-
-        return ap_pass_brigade(r->output_filters, bb);
-    }
-
-    /* grab the context from our filter */
-    ctx = r->output_filters->ctx;
-
-    /* if there isn't a buffer in the context yet, put one there. */
-    if (ctx->buf == NULL) {
-        /* use the heap so it will get free'd after being flushed */
-        ctx->avail = AP_MIN_BYTES_TO_WRITE;
-        ctx->buf = ctx->cur = malloc(ctx->avail);
-    }
-
-    /* squeeze the data into the existing buffer */
-    if (len <= ctx->avail) {
-        memcpy(ctx->cur, str, len);
-        ctx->cur += len;
-        if ((ctx->avail -= len) == 0)
-            return flush_buffer(r, ctx, NULL, 0);
-        return APR_SUCCESS;
-    }
-
-    /* the new content can't fit in the existing buffer */
-
-    if (len >= AP_MIN_BYTES_TO_WRITE) {
-        /* it is really big. send what we have, and the new stuff. */
-        return flush_buffer(r, ctx, str, len);
-    }
-
-    /* the new data is small. put some into the current buffer, flush it,
-       and then drop the remaining into a new buffer. */
-    amt = ctx->avail;
-    memcpy(ctx->cur, str, amt);
-    ctx->cur += amt;
-    ctx->avail = 0;
-    if ((status = flush_buffer(r, ctx, NULL, 0)) != APR_SUCCESS)
-        return status;
-
-    ctx->buf = malloc(AP_MIN_BYTES_TO_WRITE);
-    memcpy(ctx->buf, str + amt, len - amt);
-    ctx->cur = ctx->buf + (len - amt);
-    ctx->avail = AP_MIN_BYTES_TO_WRITE - (len - amt);
-
-    return APR_SUCCESS;
-}
-
-AP_DECLARE(int) ap_rputc(int c, request_rec *r)
-{
-    char c2 = (char)c;
-
-    if (r->connection->aborted) {
-	return -1;
-    }
-
-    if (buffer_output(r, &c2, 1) != APR_SUCCESS)
-        return -1;
-
-    return c;
-}
-
-AP_DECLARE(int) ap_rputs(const char *str, request_rec *r)
-{
-    apr_size_t len;
-
-    if (r->connection->aborted)
-        return -1;
-
-    if (buffer_output(r, str, len = strlen(str)) != APR_SUCCESS)
-        return -1;
-
-    return len;
-}
-
-AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r)
-{
-    if (r->connection->aborted)
-        return -1;
-
-    if (buffer_output(r, buf, nbyte) != APR_SUCCESS)
-        return -1;
-
-    return nbyte;
-}
-
-AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
-{
-    char buf[4096];
-    apr_size_t written;
-
-    if (r->connection->aborted)
-        return -1;
-
-    /* ### fix this mechanism to allow more than 4K of output */
-    written = apr_vsnprintf(buf, sizeof(buf), fmt, va);
-    if (buffer_output(r, buf, written) != APR_SUCCESS)
-        return -1;
-
-    return written;
-}
-
-AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt, ...)
-{
-    va_list va;
-    int n;
-
-    if (r->connection->aborted)
-        return -1;
-
-    va_start(va, fmt);
-    n = ap_vrprintf(r, fmt, va);
-    va_end(va);
-
-    return n;
-}
-
-AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r, ...)
-{
-    va_list va;
-    const char *s;
-    apr_size_t len;
-    apr_size_t written = 0;
-
-    if (r->connection->aborted)
-        return -1;
-
-    /* ### TODO: if the total output is large, put all the strings
-       ### into a single brigade, rather than flushing each time we
-       ### fill the buffer */
-    va_start(va, r);
-    while (1) {
-        s = va_arg(va, const char *);
-        if (s == NULL)
-            break;
-
-        len = strlen(s);
-        if (buffer_output(r, s, len) != APR_SUCCESS) {
-            return -1;
-        }
-
-        written += len;
-    }
-    va_end(va);
-
-    return written;
-}
-
-AP_DECLARE(int) ap_rflush(request_rec *r)
-{
-    apr_bucket_brigade *bb;
-    apr_bucket *b;
-
-    bb = apr_brigade_create(r->pool);
-    b = apr_bucket_flush_create();
-    APR_BRIGADE_INSERT_TAIL(bb, b);
-    if (ap_pass_brigade(r->output_filters, bb) != APR_SUCCESS)
-        return -1;
-    return 0;
-}
 
 static const char *add_optional_notes(request_rec *r, 
                                       const char *prefix,


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




Re: [PATCH] final part of the ap_r* solution

Posted by rb...@covalent.net.
> > This patch does exactly that.  It puts the power into the hands of the
> > programmer.  The "perfectly reasonable alternative" removes that power
> > from the module author, and gives it to some other module.  I thoroughly
> > dislike that.
> 
> Power to do what? Misorder output?

Power over how their code runs.  With the ap_r* patch I have posted, the
module author has complete control over how their code operates.  With the
OLD_WRITE filter, any other module that comes along can affect how the
code operates.  Personally, I would rather spend time debugging my code,
that I have complete control over, and never have to worry about it again,
than debug my code, only to find that some other module has negatively
impacted my performance, and then realize that in order to solve the
problem, I have to re-write my code to use ap_f* instead of ap_r*.

My biggest complaint against OLD_WRITE, is that it solve the exact same
problem as the brigade buffering, but it does it in a non-portable method

> > I would suggest that we run some performance numbers with both patches,
> > and at least take those into account when making this decision.
> 
> The decision isn't a performance one. Both are going to run with the same
> performance characteristics. Neither patch has been optimized for
> performance, so that doesn't help things either.

Then tune your patch.  The whole reason this thing went in was for
performance.  To say that isn't an issue now doesn't make any sense to me.

> The decision is based on whether you want to make module authors consider
> whether output misordering might occur, and make them compensate for it.
> 
> They can compensate by doing one of:
> 1) always use r->bb for brigade style output
> 2) choose one output style and stick to it
> 
> (1) is inflexible and (2) induces coupling (all subsystems must use the same
> style; they cannot be independent).

I don't see the problem with (1).  It works and it makes sense.  Sometimes
too much flexibility is actually a bad thing.  What does it buy us to
allow people to use any brigade in the handler?

> The current ap_r* scheme using the OLD_WRITE filter does not impose either
> of those two burdens on the module author. Switching to the brigade
> buffering (rather than a private buffer) would have no material impact on
> the user of the ap_r* or ap_pass_brigade (or ap_f* now!) interfaces.

That's true.  However, if I do the following:

    ap_register_output_filter("RBB_FILTER", ap_rbb_filter,
                              AP_FTYPE_CONTENT - 2);

then I remove the performance improvement added by old write whenever that
filter is used.  How am I as a module author supposed to fix that
problem?  Are you going to just say never do that?  Doesn't that remove
flexibility?  How am I supposed to even find that this is my performance
problem?

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


Re: [PATCH] final part of the ap_r* solution

Posted by Greg Stein <gs...@lyra.org>.
On Tue, Feb 13, 2001 at 07:57:42AM -0800, rbb@covalent.net wrote:
>...
> > > > There will be some thought required to use legacy support.  Insisting that the
> > > > legacy user (or user of legacy ap_r* generating fns) rely on r->bb when they
> > > > want to practice bilingual design cannot be called an imposing burden.
> > > 
> > > I call it an "unnecessary burden" and don't like the gotchas. There is a
> > > perfectly reasonable alternative that doesn't require the programmer to
> > > compensate and track the different output mechanisms. I say, let the
> > > programmer choose the appropriate way to do output, and just make it work
> > > for them.
> 
> This patch does exactly that.  It puts the power into the hands of the
> programmer.  The "perfectly reasonable alternative" removes that power
> from the module author, and gives it to some other module.  I thoroughly
> dislike that.

Power to do what? Misorder output?

> I would suggest that we run some performance numbers with both patches,
> and at least take those into account when making this decision.

The decision isn't a performance one. Both are going to run with the same
performance characteristics. Neither patch has been optimized for
performance, so that doesn't help things either.

The decision is based on whether you want to make module authors consider
whether output misordering might occur, and make them compensate for it.

They can compensate by doing one of:
1) always use r->bb for brigade style output
2) choose one output style and stick to it

(1) is inflexible and (2) induces coupling (all subsystems must use the same
style; they cannot be independent).

The current ap_r* scheme using the OLD_WRITE filter does not impose either
of those two burdens on the module author. Switching to the brigade
buffering (rather than a private buffer) would have no material impact on
the user of the ap_r* or ap_pass_brigade (or ap_f* now!) interfaces.

Cheers,
-g

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

Re: [PATCH] final part of the ap_r* solution

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Tuesday, February 13, 2001 7:58 PM


> On Tue, Feb 13, 2001 at 09:42:00AM -0600, William A. Rowe, Jr. wrote:
> > From: "Greg Stein" <gs...@lyra.org>
> >...
> > > If we want it in before beta, then I can build the patch. The ap_r* code
> > > already works fine, so we certainly aren't gaining functionality for the
> > > beta. It should be changed in the long run, though.
> > 
> > The 'beta' is the wakeup call to module authors to "get your ass over here.'
> > Whatever we do - we had better remain fairly consistent from here on out to release.
> 
> Ryan's patch changes the API, so it would have to go in before beta.
> 
> My (theoretical) patch to switch OLD_WRITE from an internal buffer to use
> the new brigade buffering has no impact on the API at all. It can happen
> before or after beta.

Please apply said patch ASAP.  We will not keep OLD_WRITE using the internal buffer,
so if this patch performs over the r->bb, or the moral majority believes that r->bb
is a wrong solution, it would have to happen anyways.

I am still firmly against yet another filter (a la coalesce) to do what the top/bottom
end designs should already handle.  But if the OLD_WRITE filter stays, and for benchmarking
apples to apples, it clearly needs to be updated to the new r/w bucket mechanism.

> Unrelated: I believe we can change APIs after the beta if we want. We're
> trying to build a great product; if an API change is needed to do that, then
> we should. We certainly don't want to change broad architecture -- that
> would definitely screw third-party module developers, but API changes are
> manageable.

We should change the API if we -NEED- to, but I'd like to know that we are strongly
encouraging module authors to start building solid 2.0-ready modules, and that we
firmly back them :-)

Bill


Re: [PATCH] final part of the ap_r* solution

Posted by David Reid <dr...@jetnet.co.uk>.
> Unrelated: I believe we can change APIs after the beta if we want. We're
> trying to build a great product; if an API change is needed to do that,
then
> we should. We certainly don't want to change broad architecture -- that
> would definitely screw third-party module developers, but API changes are
> manageable.

Agreed, but we should be wary of changing it without VERY good reasons after
the beta.  people need to have faith that we are delivering a stable product
they can rely on.  Also we should slap ourselves around the face with wet
halibut if we do make changes :)

It's late and I'm going to bed!

david



Re: [PATCH] final part of the ap_r* solution

Posted by Greg Stein <gs...@lyra.org>.
On Tue, Feb 13, 2001 at 09:42:00AM -0600, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gs...@lyra.org>
>...
> > If we want it in before beta, then I can build the patch. The ap_r* code
> > already works fine, so we certainly aren't gaining functionality for the
> > beta. It should be changed in the long run, though.
> 
> The 'beta' is the wakeup call to module authors to "get your ass over here.'
> Whatever we do - we had better remain fairly consistent from here on out to release.

Ryan's patch changes the API, so it would have to go in before beta.

My (theoretical) patch to switch OLD_WRITE from an internal buffer to use
the new brigade buffering has no impact on the API at all. It can happen
before or after beta.


Unrelated: I believe we can change APIs after the beta if we want. We're
trying to build a great product; if an API change is needed to do that, then
we should. We certainly don't want to change broad architecture -- that
would definitely screw third-party module developers, but API changes are
manageable.

Cheers,
-g

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

Re: [PATCH] final part of the ap_r* solution

Posted by rb...@covalent.net.
> > > > This is the final portion of the ap_r* patch.  All this does is remove the
> > > > OLD_WRITE filter, and replace the ap_r* calls with macros that use tha
> > > > ap_f* calls.
> > > 
> > > +1, I'd like to see this happen before we beta.
> > > 
> > > There will be some thought required to use legacy support.  Insisting that the
> > > legacy user (or user of legacy ap_r* generating fns) rely on r->bb when they
> > > want to practice bilingual design cannot be called an imposing burden.
> > 
> > I call it an "unnecessary burden" and don't like the gotchas. There is a
> > perfectly reasonable alternative that doesn't require the programmer to
> > compensate and track the different output mechanisms. I say, let the
> > programmer choose the appropriate way to do output, and just make it work
> > for them.

This patch does exactly that.  It puts the power into the hands of the
programmer.  The "perfectly reasonable alternative" removes that power
from the module author, and gives it to some other module.  I thoroughly
dislike that.

I would suggest that we run some performance numbers with both patches,
and at least take those into account when making this decision.

Ryan

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


Re: [PATCH] final part of the ap_r* solution

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Greg Stein" <gs...@lyra.org>
> On Tue, Feb 13, 2001 at 12:36:19AM -0600, William A. Rowe, Jr. wrote:
> > From: <rb...@covalent.net>
> > Sent: Monday, February 12, 2001 12:17 PM
> > 
> > 
> > > This is the final portion of the ap_r* patch.  All this does is remove the
> > > OLD_WRITE filter, and replace the ap_r* calls with macros that use tha
> > > ap_f* calls.
> > 
> > +1, I'd like to see this happen before we beta.
> > 
> > There will be some thought required to use legacy support.  Insisting that the
> > legacy user (or user of legacy ap_r* generating fns) rely on r->bb when they
> > want to practice bilingual design cannot be called an imposing burden.
> 
> I call it an "unnecessary burden" and don't like the gotchas. There is a
> perfectly reasonable alternative that doesn't require the programmer to
> compensate and track the different output mechanisms. I say, let the
> programmer choose the appropriate way to do output, and just make it work
> for them.
> 
> If we want it in before beta, then I can build the patch. The ap_r* code
> already works fine, so we certainly aren't gaining functionality for the
> beta. It should be changed in the long run, though.

The 'beta' is the wakeup call to module authors to "get your ass over here.'
Whatever we do - we had better remain fairly consistent from here on out to release.


Re: [PATCH] final part of the ap_r* solution

Posted by Greg Stein <gs...@lyra.org>.
On Tue, Feb 13, 2001 at 12:36:19AM -0600, William A. Rowe, Jr. wrote:
> From: <rb...@covalent.net>
> Sent: Monday, February 12, 2001 12:17 PM
> 
> 
> > This is the final portion of the ap_r* patch.  All this does is remove the
> > OLD_WRITE filter, and replace the ap_r* calls with macros that use tha
> > ap_f* calls.
> 
> +1, I'd like to see this happen before we beta.
> 
> There will be some thought required to use legacy support.  Insisting that the
> legacy user (or user of legacy ap_r* generating fns) rely on r->bb when they
> want to practice bilingual design cannot be called an imposing burden.

I call it an "unnecessary burden" and don't like the gotchas. There is a
perfectly reasonable alternative that doesn't require the programmer to
compensate and track the different output mechanisms. I say, let the
programmer choose the appropriate way to do output, and just make it work
for them.

If we want it in before beta, then I can build the patch. The ap_r* code
already works fine, so we certainly aren't gaining functionality for the
beta. It should be changed in the long run, though.

Cheers,
-g

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

Re: [PATCH] final part of the ap_r* solution

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: <rb...@covalent.net>
Sent: Monday, February 12, 2001 12:17 PM


> This is the final portion of the ap_r* patch.  All this does is remove the
> OLD_WRITE filter, and replace the ap_r* calls with macros that use tha
> ap_f* calls.

+1, I'd like to see this happen before we beta.

There will be some thought required to use legacy support.  Insisting that the
legacy user (or user of legacy ap_r* generating fns) rely on r->bb when they
want to practice bilingual design cannot be called an imposing burden.