httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: [PATCH] buckets take 2
Date Thu, 18 Jan 2001 06:10:06 GMT

> > 1) doesn't deal with network pressure. mod_autoindex will generate the whole
> >    response in memory before delivering a single byte.
> 
> Actually I have a fix in mind for this, but I wanted to begin to implement
> the idea before implementing the whole thing.  Expect a new patch within a
> few hours.

Okay, here is the new patch.  This basically passes the brigade down the
filter stack as soon as we move the buffer into a bucket.  This only
happens when using the ap_r* functions, any other API requires that the
caller pay attention and pass at appropriate times.  Before the patch,
allow me to show you the important part of two stack traces.

This is the writev call from the stack trace posted earlier today:

writev(8, [{"HTTP/1.1 200 OK\r\nDate: Thu, 18 J"..., 208}, {"<!DOCTYPE
HTML PUBLIC \"-//W3C//D"..., 3429}], 2) = 3637

The same writev call from a stack trace with this patch:

writev(8, [{"HTTP/1.1 200 OK\r\nDate: Thu, 18 J"..., 208}, {"<!DOCTYPE
HTML PUBLIC \"-//W3C//D"..., 4089}, {"index.html.se</A> 03-D"..., 392}],
3) = 4689

Notice that there is one more element that starts at about 4K into the
response.  What is happening, is that the bucket buffer size is 4K, so we
end up with a single brigade with three elements, 1)  The headers, 2)  The
first 4K of data, 3) The remaining data.  The first stack trace only
passed the brigade when ap_rflush was called.  This basically allows the
buckets to use the network to apply pressure so that we ensure we aren't
sending too much data.

Again, this patch doesn't modify any APIs, it just works, and it continues
to work with existing modules.

Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.129
diff -u -d -b -w -u -r1.129 httpd.h
--- include/httpd.h	2001/01/05 20:44:39	1.129
+++ include/httpd.h	2001/01/18 05:57:47
@@ -85,6 +85,7 @@
 #include "apr_pools.h"
 #include "apr_time.h"
 #include "apr_network_io.h"
+#include "ap_buckets.h"
 
 #ifdef CORE_PRIVATE
 
@@ -809,6 +810,8 @@
     /** A flag to determine if the eos bucket has been sent yet
      *  @defvar int eos_sent */
     int eos_sent;
+
+    ap_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_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.266
diff -u -d -b -w -u -r1.266 http_protocol.c
--- modules/http/http_protocol.c	2001/01/15 15:40:17	1.266
+++ modules/http/http_protocol.c	2001/01/18 05:57:51
@@ -2679,6 +2679,7 @@
         r = r->next;
     }
     /* tell the filter chain there is no more content coming */
+    ap_rflush(r);
     if (!r->eos_sent) {
         end_output_stream(r);
     }
@@ -2998,77 +2999,130 @@
 
 AP_DECLARE(int) ap_rputc(int c, request_rec *r)
 {
-    ap_bucket_brigade *bb = NULL;
     ap_bucket *b;
+    apr_status_t status;
     char c2 = (char)c;
 
     if (r->connection->aborted) {
 	return EOF;
     }
 
-    bb = ap_brigade_create(r->pool);
-    b = ap_bucket_create_transient(&c2, 1);
-    AP_BRIGADE_INSERT_TAIL(bb, b);
-    ap_pass_brigade(r->output_filters, bb);
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
+    }
 
-    return c;
+    b = AP_BRIGADE_LAST(r->bb);
+
+    status = ap_brigade_putc(r->bb, c2);
+
+    /* Okay, so this sucks, but we don't have many options.  What we
+     * are doing here is just checking to see if we just converted the
+     * buffer into a new bucket and put it at the end of the brigade.  If
+     * we did, we want to pass the brigade to the next filter.  If not,
+     * we just keep going.  This allows us to use the network to limit how
+     * much data we send at any one time.
+     */
+    if (b != AP_BRIGADE_LAST(r->bb)) {
+        ap_pass_brigade(r->output_filters, r->bb);
 }
 
+    return status;
+}
+
 AP_DECLARE(int) ap_rputs(const char *str, request_rec *r)
 {
-    ap_bucket_brigade *bb = NULL;
     ap_bucket *b;
-    apr_size_t len;
+    apr_status_t status;
 
     if (r->connection->aborted)
         return EOF;
     if (*str == '\0')
         return 0;
 
-    len = strlen(str);
-    bb = ap_brigade_create(r->pool);
-    b = ap_bucket_create_transient(str, len);
-    AP_BRIGADE_INSERT_TAIL(bb, b);
-    ap_pass_brigade(r->output_filters, bb);
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
+    }
 
-    return len;
+    b = AP_BRIGADE_LAST(r->bb);
+
+    status = ap_brigade_puts(r->bb, str);
+
+    /* Okay, so this sucks, but we don't have many options.  What we
+     * are doing here is just checking to see if we just converted the
+     * buffer into a new bucket and put it at the end of the brigade.  If
+     * we did, we want to pass the brigade to the next filter.  If not,
+     * we just keep going.  This allows us to use the network to limit how
+     * much data we send at any one time.
+     */
+    if (b != AP_BRIGADE_LAST(r->bb)) {
+        ap_pass_brigade(r->output_filters, r->bb);
+    }
+        
+    return status;
 }
 
 AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r)
 {
-    ap_bucket_brigade *bb = NULL;
     ap_bucket *b;
+    apr_status_t status;
 
     if (r->connection->aborted)
         return EOF;
     if (nbyte == 0)
         return 0;
 
-    bb = ap_brigade_create(r->pool);
-    b = ap_bucket_create_transient(buf, nbyte);
-    AP_BRIGADE_INSERT_TAIL(bb, b);
-    ap_pass_brigade(r->output_filters, bb);
-    return nbyte;
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
 }
 
+    b = AP_BRIGADE_LAST(r->bb);
+
+    status = ap_brigade_puts(r->bb, buf);
+
+    /* Okay, so this sucks, but we don't have many options.  What we
+     * are doing here is just checking to see if we just converted the
+     * buffer into a new bucket and put it at the end of the brigade.  If
+     * we did, we want to pass the brigade to the next filter.  If not,
+     * we just keep going.  This allows us to use the network to limit how
+     * much data we send at any one time.
+     */
+    if (b != AP_BRIGADE_LAST(r->bb)) {
+        ap_pass_brigade(r->output_filters, r->bb);
+    }
+        
+    return status;
+}
+
 AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
 {
-    ap_bucket_brigade *bb = NULL;
-    apr_size_t written;
+    ap_bucket *b;
+    apr_status_t status;
 
     if (r->connection->aborted)
         return EOF;
 
-    bb = ap_brigade_create(r->pool);
-    written = ap_brigade_vprintf(bb, fmt, va);
-    if (written != 0)
-        ap_pass_brigade(r->output_filters, bb);
-    return written;
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
 }
 
-/* TODO:  Make ap pa_bucket_vprintf that printfs directly into a
- * bucket.
+    b = AP_BRIGADE_LAST(r->bb);
+
+    status = ap_brigade_vprintf(r->bb, fmt, va);
+
+    /* Okay, so this sucks, but we don't have many options.  What we
+     * are doing here is just checking to see if we just converted the
+     * buffer into a new bucket and put it at the end of the brigade.  If
+     * we did, we want to pass the brigade to the next filter.  If not,
+     * we just keep going.  This allows us to use the network to limit how
+     * much data we send at any one time.
  */
+    if (b != AP_BRIGADE_LAST(r->bb)) {
+        ap_pass_brigade(r->output_filters, r->bb);
+    }
+        
+    return status;
+}
+
 AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt, ...)
 {
     va_list va;
@@ -3086,31 +3140,34 @@
 
 AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r, ...)
 {
-    ap_bucket_brigade *bb = NULL;
     apr_size_t written;
     va_list va;
 
     if (r->connection->aborted)
         return EOF;
-    bb = ap_brigade_create(r->pool);
+
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
+    }
+
     va_start(va, r);
-    written = ap_brigade_vputstrs(bb, va);
+    written = ap_brigade_vputstrs(r->bb, va);
     va_end(va);
-    if (written != 0)
-        ap_pass_brigade(r->output_filters, bb);
     return written;
 }
 
 AP_DECLARE(int) ap_rflush(request_rec *r)
 {
     /* we should be using a flush bucket to flush the stack, not buff code. */
-    ap_bucket_brigade *bb;
     ap_bucket *b;
 
-    bb = ap_brigade_create(r->pool);
     b = ap_bucket_create_flush();
-    AP_BRIGADE_INSERT_TAIL(bb, b);
-    ap_pass_brigade(r->output_filters, bb);
+    if (!r->bb) {
+        r->bb = ap_brigade_create(r->pool);
+    }
+    ap_brigade_standardize(r->bb);
+    AP_BRIGADE_INSERT_TAIL(r->bb, b);
+    ap_pass_brigade(r->output_filters, r->bb);
     return 0;
 }
 
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.42
diff -u -d -b -w -u -r1.42 util_filter.c
--- server/util_filter.c	2000/12/29 13:56:30	1.42
+++ server/util_filter.c	2001/01/18 05:57:52
@@ -237,6 +237,7 @@
              */
             next->r->eos_sent = 1;
         }
+        ap_brigade_standardize(bb);
         return next->frec->filter_func.out_func(next, bb);
     }
     return AP_NOBODY_WROTE;
Index: srclib/apr-util/include/ap_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/ap_buckets.h,v
retrieving revision 1.62
diff -u -d -b -w -u -r1.62 ap_buckets.h
--- srclib/apr-util/include/ap_buckets.h	2000/12/31 12:12:56	1.62
+++ srclib/apr-util/include/ap_buckets.h	2001/01/18 05:57:57
@@ -74,6 +74,8 @@
  * @package Bucket Brigades
  */
 
+#define APR_BUCKET_BUFF_SIZE  4096
+
 typedef enum {AP_BLOCK_READ, AP_NONBLOCK_READ} ap_read_type;
 
 /*
@@ -233,6 +235,8 @@
      *  the destroying function is responsible for killing the cleanup.
      */
     apr_pool_t *p;
+    char *start;
+    char *end;
     /** The buckets in the brigade are on this list. */
     /*
      * XXX: the ap_bucket_list structure doesn't actually need a name tag
@@ -620,9 +624,10 @@
 APU_DECLARE(int) ap_brigade_to_iovec(ap_bucket_brigade *b, 
 				    struct iovec *vec, int nvec);
 
+APU_DECLARE(void) ap_brigade_standardize(ap_bucket_brigade *b);
+
 /**
- * This function writes a list of strings into a bucket brigade.  We just 
- * allocate a new heap bucket for each string.
+ * This function writes a list of strings into a bucket brigade.
  * @param b The bucket brigade to add to
  * @param va A list of strings to add
  * @return The number of bytes added to the brigade
@@ -631,8 +636,25 @@
 APU_DECLARE(int) ap_brigade_vputstrs(ap_bucket_brigade *b, va_list va);
 
 /**
+ * This function writes an string into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param str The string to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int ap_brigade_puts(ap_bucket_brigade *b, const char *str)
+ */
+APU_DECLARE_NONSTD(int) ap_brigade_puts(ap_bucket_brigade *b, const char *str);
+
+/**
+ * This function writes a character into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param c The character to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int ap_brigade_putc(ap_bucket_brigade *b, const char c)
+ */
+APU_DECLARE_NONSTD(int) ap_brigade_putc(ap_bucket_brigade *b, const char c);
+
+/**
  * This function writes an unspecified number of strings into a bucket brigade.
- * We just allocate a new heap bucket for each string.
  * @param b The bucket brigade to add to
  * @param ... The strings to add
  * @return The number of bytes added to the brigade
@@ -641,7 +663,7 @@
 APU_DECLARE_NONSTD(int) ap_brigade_putstrs(ap_bucket_brigade *b, ...);
 
 /**
- * Evaluate a printf and put the resulting string into a bucket at the end 
+ * Evaluate a printf and put the resulting string at the end 
  * of the bucket brigade.
  * @param b The brigade to write to
  * @param fmt The format of the string to write
@@ -652,7 +674,7 @@
 APU_DECLARE_NONSTD(int) ap_brigade_printf(ap_bucket_brigade *b, const char *fmt, ...);
 
 /**
- * Evaluate a printf and put the resulting string into a bucket at the end 
+ * Evaluate a printf and put the resulting string at the end 
  * of the bucket brigade.
  * @param b The brigade to write to
  * @param fmt The format of the string to write
Index: srclib/apr-util/src/buckets/ap_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/src/buckets/ap_brigade.c,v
retrieving revision 1.2
diff -u -d -b -w -u -r1.2 ap_brigade.c
--- srclib/apr-util/src/buckets/ap_brigade.c	2001/01/17 04:29:26	1.2
+++ srclib/apr-util/src/buckets/ap_brigade.c	2001/01/18 05:57:57
@@ -99,6 +99,9 @@
 
     b = apr_palloc(p, sizeof(*b));
     b->p = p;
+
+    b->start = b->end = NULL;
+
     AP_RING_INIT(&b->list, ap_bucket, link);
 
     apr_register_cleanup(b->p, b, ap_brigade_cleanup, ap_brigade_cleanup);
@@ -185,12 +188,22 @@
     return vec - orig;
 }
 
+APU_DECLARE(void) ap_brigade_standardize(ap_bucket_brigade *b)
+{
+    ap_bucket *e;
+    apr_size_t i = b->end - b->start;
+
+    if (i) {
+        e = ap_bucket_create_pool(b->start, i, b->p);
+        AP_BRIGADE_INSERT_TAIL(b, e);
+        b->start = b->end = NULL;
+    }
+}
+
 APU_DECLARE(int) ap_brigade_vputstrs(ap_bucket_brigade *b, va_list va)
 {
-    ap_bucket *r;
     const char *x;
     int j, k;
-    apr_size_t i;
 
     for (k = 0;;) {
         x = va_arg(va, const char *);
@@ -198,20 +211,57 @@
             break;
         j = strlen(x);
        
-	/* XXX: copy or not? let the caller decide? */
-        r = ap_bucket_create_heap(x, j, 1, &i);
-        if (i != j) {
-            /* Do we need better error reporting?  */
-            return -1;
+        if (j > (APR_BUCKET_BUFF_SIZE - (b->end - b->start))) {
+            ap_brigade_standardize(b);
         }
-        k += i;
 
-        AP_BRIGADE_INSERT_TAIL(b, r);
+        if (!b->start) {
+            b->start = b->end = apr_palloc(b->p, APR_BUCKET_BUFF_SIZE);
     }
 
+        memcpy(b->end, x, j);
+        b->end += j;
+
+        k += j;
+    }
+
     return k;
 }
 
+APU_DECLARE(int) ap_brigade_putc(ap_bucket_brigade *b, const char c)
+{
+    if (1 > (APR_BUCKET_BUFF_SIZE - (b->end - b->start))) {
+        ap_brigade_standardize(b);
+    }
+    if (!b->start) {
+        b->start = b->end = apr_palloc(b->p, APR_BUCKET_BUFF_SIZE);
+    }
+
+    memcpy(b->end, &c, 1);
+    b->end++;
+
+    return 1;
+}
+
+APU_DECLARE(int) ap_brigade_puts(ap_bucket_brigade *b, const char *str)
+{
+    apr_size_t j;
+
+    j = strlen(str);
+   
+    if (j > (APR_BUCKET_BUFF_SIZE - (b->end - b->start))) {
+        ap_brigade_standardize(b);
+    }
+    if (!b->start) {
+        b->start = b->end = apr_palloc(b->p, APR_BUCKET_BUFF_SIZE);
+    }
+
+    memcpy(b->end, str, j);
+    b->end += j;
+
+    return j;
+}
+
 APU_DECLARE_NONSTD(int) ap_brigade_putstrs(ap_bucket_brigade *b, ...)
 {
     va_list va;
@@ -240,13 +290,8 @@
      * directly into a bucket.  I'm being lazy right now.  RBB
      */
     char buf[4096];
-    ap_bucket *r;
-    int res;
 
-    res = apr_vsnprintf(buf, 4096, fmt, va);
-
-    r = ap_bucket_create_heap(buf, strlen(buf), 1, NULL);
-    AP_BRIGADE_INSERT_TAIL(b, r);
+    apr_vsnprintf(buf, 4096, fmt, va);
 
-    return res;
+    return ap_brigade_puts(b, buf);
 }


Ryan

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


Mime
View raw message