[Posting for comments before I commit, because the
core filter code is somewhat complex...]
With the current core_output_filter() implementation,
if the client requests a file smaller than 8KB, the
filter reads the file bucket, makes a copy of the
contents, and sets the copy aside in hopes of being
able to concatenate it with the next response.
This copying results in a performance penalty when
using keepalive requests for small files.
The attached patch removes the copy in the common case
where there's only one file bucket in the brigade being
set aside. Instead of copying the file contents, the patch
just sets aside the file bucket as-is, so that the next
invocation of core_output_filter can call sendfile if there's
not another response immediately following this one. (If
there are multiple file buckets in the brigade to be set
aside, though, this code turns all but the first of them
into mmap buckets to ensure that we can't run out of file
descriptors in extreme use cases.)
Brian
Index: server/core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.198
diff -u -r1.198 core.c
--- server/core.c 6 Aug 2002 20:02:07 -0000 1.198
+++ server/core.c 16 Aug 2002 07:09:46 -0000
@@ -3782,46 +3782,42 @@
* we want to process to second request fully.
*/
if (APR_BUCKET_IS_EOS(last_e)) {
- apr_bucket *bucket = NULL;
- /* If we are in here, then this request is a keepalive. We
- * need to be certain that any data in a bucket is valid
- * after the request_pool is cleared.
- */
- if (ctx->b == NULL) {
- ctx->b = apr_brigade_create(net->c->pool,
- net->c->bucket_alloc);
- }
-
- APR_BRIGADE_FOREACH(bucket, b) {
- const char *str;
- apr_size_t n;
+ apr_bucket *bucket;
+ int file_bucket_saved = 0;
+ APR_BUCKET_REMOVE(last_e);
+ for (bucket = APR_BRIGADE_FIRST(b);
+ bucket != APR_BRIGADE_SENTINEL(b);
+ bucket = APR_BUCKET_NEXT(bucket)) {
- rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
-
- /* This apr_brigade_write does not use a flush function
- because we assume that we will not write enough data
- into it to cause a flush. However, if we *do* write
- "too much", then we could end up with transient
- buckets which would suck. This works for now, but is
- a bit shaky if changes are made to some of the
- buffering sizes. Let's do an assert to prevent
- potential future problems... */
- AP_DEBUG_ASSERT(AP_MIN_BYTES_TO_WRITE <=
- APR_BUCKET_BUFF_SIZE);
- if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_ERR, rv, c->base_server,
- "core_output_filter: Error reading from
bucket.");
- return HTTP_INTERNAL_SERVER_ERROR;
+ /* Do a read on each bucket to pull in the
+ * data from pipe and socket buckets, so
+ * that we don't leave their file descriptors
+ * open indefinitely. Do the same for file
+ * buckets, with one exception: allow the
+ * first file bucket in the brigade to remain
+ * a file bucket, so that we don't end up
+ * doing an mmap+memcpy every time a client
+ * requests a <8KB file over a keepalive
+ * connection.
+ */
+ if (APR_BUCKET_IS_FILE(bucket) && !file_bucket_saved) {
+ file_bucket_saved = 1;
+ }
+ else {
+ const char *buf;
+ apr_size_t len = 0;
+ rv = apr_bucket_read(bucket, &buf, &len,
+ APR_BLOCK_READ);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, rv,
+ c->base_server, "core_output_filter:"
+ " Error reading from bucket.");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
}
-
- apr_brigade_write(ctx->b, NULL, NULL, str, n);
}
-
- apr_brigade_destroy(b);
- }
- else {
- ap_save_brigade(f, &ctx->b, &b, c->pool);
}
+ ap_save_brigade(f, &ctx->b, &b, c->pool);
return APR_SUCCESS;
}
