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/01/18 20:17:55 UTC
[PATCH] buckets take 3
This should apply and just work on the new tree.
Ryan
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/buckets/ap_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/ap_brigade.c,v
retrieving revision 1.2
diff -u -d -b -w -u -r1.2 ap_brigade.c
--- srclib/apr-util/buckets/ap_brigade.c 2001/01/17 04:29:26 1.2
+++ srclib/apr-util/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);
}
Re: [PATCH] buckets take 3
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
>
> > My strace looks as pretty as yours.
> >
> > ab results are pretty too:
> >
> > req/sec kb/sec
> > +--------+--------+
> > no patch | 20 | 160 |
> > +--------+--------+
> > patch #3 | 52 | 418 |
> > +--------+--------+
>
> Wow! I didn't think it would help that much.
Well, yah. When you're dealing with bazillions of tinygrams all going
in the same direction (i.e. no handshaking with your partner),
minimizing per-call overhead and path length are the key things. When
you've got huge data, the story changes. You can afford more per-call
overhead if it helps to minimize copies.
We used to use a rule of thumb like this: copying 5 bytes of data costs
1 "average" mainframe instruction. (YMMV of course, it's no doubt
different on current S/390s even.) So if you can avoid copying an
entire ~1500 byte ethernet packet, your budget is 300 instructions. If
you add any more extra instructions than that to avoid a copy, you
loose.
For a five byte tinygram, the break even point is 1 instruction. Can't
be done. Just copy it as soon as you can.
> What does 1.3 look like?
Sounds like a project for tomorrow. I'm sleep deprived - need to crash
(hopefully at home, not on the way there :-)
Greg
Re: [PATCH] buckets take 3
Posted by rb...@covalent.net.
> My strace looks as pretty as yours.
>
> ab results are pretty too:
>
> req/sec kb/sec
> +--------+--------+
> no patch | 20 | 160 |
> +--------+--------+
> patch #3 | 52 | 418 |
> +--------+--------+
Wow! I didn't think it would help that much. What does 1.3 look like?
> (autoindex of my htdocs, which has a bunch of extra .gifs in it)
>
> commit it already!
I'm waiting for Greg to give it at least a -0 first. It doesn't help us
to commit it if we are just going to back it out immediately, and I have a
feeling that Greg would veto it if I committed right now.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] buckets take 3
Posted by Doug MacEachern <do...@covalent.net>.
On Thu, 18 Jan 2001, Greg Ames wrote:
> rbb@covalent.net wrote:
> >
> > This should apply and just work on the new tree.
> >
> ...and it did.
>
> +1
nice, +1 here too, provided the only outstanding complaint is as ryan put
it: "that it requires that people actually realize that they are moving
from buffered to non-buffered". if greg or somebody else finds a better
way that does not require this, then it can go in later. ryan's patch is
clearly heaps better than what currently exists underneath ap_r*.
Re: [PATCH] buckets take 3
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
>
> This should apply and just work on the new tree.
>
...and it did.
+1
My strace looks as pretty as yours.
ab results are pretty too:
req/sec kb/sec
+--------+--------+
no patch | 20 | 160 |
+--------+--------+
patch #3 | 52 | 418 |
+--------+--------+
(autoindex of my htdocs, which has a bunch of extra .gifs in it)
commit it already!
Greg