httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r717867 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_buffer.xml docs/manual/mod/mod_buffer.xml.meta modules/filters/config.m4 modules/filters/mod_buffer.c
Date Sat, 15 Nov 2008 20:38:47 GMT


On 11/15/2008 04:49 PM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Sat Nov 15 07:49:28 2008
> New Revision: 717867
> 
> URL: http://svn.apache.org/viewvc?rev=717867&view=rev
> Log:
> mod_buffer: Optional support for buffering of the input and output
> filter stacks. Can collapse many small buckets into fewer larger
> buckets, and prevents excessively small chunks being sent over
> the wire.
> 
> Added:
>     httpd/httpd/trunk/docs/manual/mod/mod_buffer.xml
>     httpd/httpd/trunk/docs/manual/mod/mod_buffer.xml.meta
>     httpd/httpd/trunk/modules/filters/mod_buffer.c
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/filters/config.m4
> 

> Added: httpd/httpd/trunk/modules/filters/mod_buffer.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_buffer.c?rev=717867&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_buffer.c (added)
> +++ httpd/httpd/trunk/modules/filters/mod_buffer.c Sat Nov 15 07:49:28 2008
> @@ -0,0 +1,288 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements.  See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License.  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +/*
> + * mod_buffer.c --- Buffer the input and output filter stacks, collapse
> + *                  many small buckets into fewer large buckets.
> + */
> +
> +#include "apr.h"
> +#include "apr_strings.h"
> +#include "apr_buckets.h"
> +#include "apr_lib.h"
> +
> +#include "ap_config.h"
> +#include "util_filter.h"
> +#include "httpd.h"
> +#include "http_config.h"
> +#include "http_log.h"
> +#include "http_request.h"
> +
> +static const char bufferFilterName[] = "BUFFER";
> +module AP_MODULE_DECLARE_DATA buffer_module;
> +
> +#define DEFAULT_BUFFER_SIZE 128*1024
> +
> +typedef struct buffer_conf {
> +    apr_off_t size; /* size of the buffer */
> +    int size_set; /* has the size been set */
> +} buffer_conf;
> +
> +typedef struct buffer_ctx {
> +    apr_bucket_brigade *bb;
> +    buffer_conf *conf;
> +} buffer_ctx;
> +
> +/**
> + * Buffer buckets being written to the output filter stack.
> + */
> +static apr_status_t buffer_out_filter(ap_filter_t *f, apr_bucket_brigade *bb) {
> +    apr_bucket *e;
> +    request_rec *r = f->r;
> +    buffer_ctx *ctx = f->ctx;
> +    apr_status_t rv = APR_SUCCESS;
> +
> +    /* first time in? create a context */
> +    if (!ctx) {
> +
> +        /* buffering won't work on subrequests, it would be nice if
> +         * it did. Within subrequests, we have no EOS to check for,
> +         * so we don't know when to flush the buffer to the network
> +         */
> +        if (!ap_is_initial_req(f->r)) {
> +            ap_remove_output_filter(f);
> +            return ap_pass_brigade(f->next, bb);
> +        }
> +
> +        ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> +        ctx->bb = apr_brigade_create(r->pool, f->c->bucket_alloc);
> +        ctx->conf = ap_get_module_config(f->r->per_dir_config, &buffer_module);
> +
> +    }
> +
> +    /* Do nothing if asked to filter nothing. */
> +    if (APR_BRIGADE_EMPTY(bb)) {
> +        return ap_pass_brigade(f->next, bb);
> +    }

Hm. This changes the order in which we sent the brigades (provided we have already buffered
something).
I am not sure if this causes any harm down the chain or not.
Wouldn't it be more sane in this case to "add" the empty brigade to our buffer?

> +
> +    while (APR_SUCCESS == rv && !APR_BRIGADE_EMPTY(bb)) {
> +        const char *data;
> +        apr_off_t len;
> +        apr_size_t size;
> +
> +        e = APR_BRIGADE_FIRST(bb);
> +
> +        /* EOS means we are done. */
> +        if (APR_BUCKET_IS_EOS(e)) {
> +
> +            /* should we add an etag? */
> +
> +            /* pass the EOS across */
> +            APR_BUCKET_REMOVE(e);
> +            APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
> +
> +            /* pass what we have down the chain */
> +            rv = ap_pass_brigade(f->next, ctx->bb);

For sanity reasons I would add an apr_brigade_cleanup(ctx->bb) here.

> +            continue;
> +        }
> +
> +        /* A flush takes precedence over buffering */
> +        if (APR_BUCKET_IS_FLUSH(e)) {
> +
> +            /* pass the flush bucket across */
> +            APR_BUCKET_REMOVE(e);
> +            APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
> +
> +            /* pass what we have down the chain */
> +            rv = ap_pass_brigade(f->next, ctx->bb);

For sanity reasons I would add an apr_brigade_cleanup(ctx->bb) here.
BTW: Couldn't we handle EOS and flush buckets in the same block?

> +            continue;
> +        }
> +
> +        /* metadata buckets are preserved as is */
> +        if (APR_BUCKET_IS_METADATA(e)) {
> +            /*
> +             * Remove meta data bucket from old brigade and insert into the
> +             * new.
> +             */
> +            APR_BUCKET_REMOVE(e);
> +            APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
> +            continue;
> +        }
> +
> +        /* is our buffer full?
> +         * If so, send what we have down the filter chain. If the buffer
> +         * gets full, we can no longer compute a content length.
> +         */
> +        apr_brigade_length(ctx->bb, 1, &len);
> +        if (len > ctx->conf->size) {
> +
> +            /* pass what we have down the chain */
> +            rv = ap_pass_brigade(f->next, ctx->bb);

For sanity reasons I would add an apr_brigade_cleanup(ctx->bb) here.

> +        }
> +
> +        /* at this point we are ready to buffer.
> +         * Buffering takes advantage of an optimisation in the handling of
> +         * bucket brigades. Heap buckets are always created at a fixed
> +         * size, regardless of the size of the data placed into them.
> +         * The apr_brigade_write() call will first try and pack the data
> +         * into any free space in the most recent heap bucket, before
> +         * allocating a new bucket if necessary.
> +         */
> +        if (APR_SUCCESS == (rv = apr_bucket_read(e, &data, &size,
> +                APR_BLOCK_READ))) {
> +            apr_brigade_write(ctx->bb, NULL, NULL, data, size);

I think this is suboptimal in many ways:

1. We should only do this on heap buckets. For other buckets we don't even know
   that we waste memory and in this case we create unneeded memory copy operations.
   This part gets extremely bad when we have a file bucket. Yes, I understand the
   intentions of this module and have knowledge about buckets and brigades, but the
   normal user has not. So the user might think it is a good idea to use this filter
   in case of static files. So at least a warning should be added to the documentation
   that this is counter productive when serving files.

2. If we have a large heap bucket (>= APR_BUCKET_BUFF_SIZE) or one that doesn't fit into
   the remaining buffer of the last heap heap bucket in ctx->bb apr_bucket_write creates
   a new heap bucket and thus causes the memory to be copied over from the old bucket to
   the new bucket. We should set an intelligent flush function for apr_brigade_write
   that prevents this and just moves over this bucket from bb to ctx->bb (the transient
   bucket created by apr_brigade_write is not a big loss in this case).

3. If ctx->bb is empty we do copy the memory of the the first bucket in a new heap bucket.
   This is unnecessary. If ctx->bb is empty we should just move over the bucket from bb
   to ctx->bb.


> +        }
> +
> +        apr_bucket_delete(e);
> +    }
> +
> +    return rv;
> +
> +}
> +
> +/**
> + * Buffer buckets being read from the input filter stack.
> + */
> +static apr_status_t buffer_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
> +        ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes) {
> +    apr_bucket *e;
> +    apr_bucket_brigade *tmp;
> +    apr_status_t rv;
> +    buffer_conf *c;
> +    apr_off_t remaining;
> +
> +    /* buffer on main requests only */
> +    if (!ap_is_initial_req(f->r)) {
> +        ap_remove_input_filter(f);
> +        return ap_get_brigade(f->next, bb, mode, block, readbytes);
> +    }
> +
> +    /* just get out of the way of things we don't want. */
> +    if (mode != AP_MODE_READBYTES) {
> +        return ap_get_brigade(f->next, bb, mode, block, readbytes);
> +    }
> +
> +    c = ap_get_module_config(f->r->per_dir_config, &buffer_module);
> +
> +    tmp = apr_brigade_create(f->r->pool, f->c->bucket_alloc);

This is a memory leak if pass this input filter quite often.

> +
> +    remaining = readbytes;
> +    while (remaining > 0) {
> +        const char *data;
> +        apr_off_t len;
> +        apr_size_t size;
> +
> +        rv = ap_get_brigade(f->next, tmp, mode, block, remaining);
> +
> +        /* if an error was received, bail out now */
> +        if (rv != APR_SUCCESS) {
> +            APR_BRIGADE_CONCAT(bb, tmp);
> +            return rv;
> +        }
> +
> +        apr_brigade_length(tmp, 1, &len);

This can lead to a blocking read even if we requested a non blocking mode.

> +        remaining -= len;

The current approach can create an endless loop and a CPU spin in non blocking
mode as some input filters do not return EAGAIN, but an empty brigade and
APR_SUCCESS in the case when no data was available.
And even if they return EAGAIN we might do the wrong thing, because if a previous
ap_get_brigade was successful, we would return EAGAIN as return code which is wrong.
We should return APR_SUCCESS in this case as we have actually read some data.
If we are in blocking mode we can get stuck here for quite a long time without
giving any data back to the caller. I think in the case of a blocking mode it
would be better to switch to non blocking mode after the first ap_get_brigade
and add to the tmp brigade as long as we get data but return with what we have
as soon as we get an EAGAIN / empty brigade.


> +
> +        for (e = APR_BRIGADE_FIRST(tmp); e != APR_BRIGADE_SENTINEL(tmp); e
> +                = APR_BUCKET_NEXT(e)) {
> +
> +            /* if we see an EOS, we are done */
> +            if (APR_BUCKET_IS_EOS(e)) {
> +                APR_BUCKET_REMOVE(e);
> +                APR_BRIGADE_INSERT_TAIL(bb, e);
> +                remaining = 0;
> +                break;
> +            }
> +
> +            /* pass flush buckets through */
> +            if (APR_BUCKET_IS_FLUSH(e)) {
> +                APR_BUCKET_REMOVE(e);
> +                APR_BRIGADE_INSERT_TAIL(bb, e);
> +                continue;
> +            }
> +
> +            /* pass metadata buckets through */
> +            if (APR_BUCKET_IS_METADATA(e)) {
> +                APR_BUCKET_REMOVE(e);
> +                APR_BRIGADE_INSERT_TAIL(bb, e);
> +                continue;
> +            }

I guess we need no special handling for flush and eos buckets here.
And for eos buckets we can have an additional check that just sets remaining to 0.

> +
> +            /* read */
> +            if (APR_SUCCESS == (rv = apr_bucket_read(e, &data, &size,
> +                    APR_BLOCK_READ))) {
> +                apr_brigade_write(bb, NULL, NULL, data, size);

Basicly the same arguments regarding apr_brigade_write as above are valid here as well.

> +            }
> +
> +        }
> +        apr_brigade_cleanup(tmp);
> +    }
> +
> +    return APR_SUCCESS;
> +}

Regards

RĂ¼diger


Mime
View raw message