You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2008/11/15 21:38:47 UTC
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
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
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
Posted by Graham Leggett <mi...@sharp.fm>.
Ruediger Pluem wrote:
> Thanks for clarification. Nevertheless we should make it more clear in the
> documentation that this module can make performance worse if used in the
> wrong context, e.g. with static files. Otherwise users might think that
> buffering files via mod_buffer could be a cool performance boost which
> it is not.
I have added a warning to the docs to better explain just what the
buffer does, and that it is not going to help in cases where the
response is already efficient.
> You can have a mixture of both because there can be include files in your
> nested include files that have a larger chunk while many other are as small
> as you describe. IMHO by taking care of this case you don't loose any of your
> buffer features as the resulting brigade is nearly the same with the difference
> that we just recreated and mem copied the buckets we had before. The only
> difference that matters appears in the case of a small file bucket that did
> not fit in the previous heap bucket.
> IMHO you put too much burden on the administrator to know the internals of
> httpd in order to make a good decision whether to use mod_buffer or not.
In theory the administrator can try mod_buffer, and if it makes it
faster, all well and good, if it doesn't oh well.
I took quite a while looking at this, most specifically a potential
optimisation where if the heap bucket you were adding was already 8k in
size, just add the bucket and don't repack. But whichever way I looked
at it, I could see potential for the optimisation to be lost.
I think further optimisations should probably come with switches to
control how they work.
>> The point behind the buffer is to convert whatever buckets are present
>> into the smallest number of heap buckets as possible, and do this until
>> the buffer is full, or the response is complete, whichever comes first.
>>
>> Think of a potentially expensive bucket of known but long length - we
>> certainly don't want to blindly move the expensive bucket over in this
>> case, as the bucket will only get read at some point in the distance
>> future when the slow client gets round to read the bucket, and the whole
>> purpose of buffering is lost.
>
> Could you please elaborate of which kind of expensive bucket you are talking
> about?
One example is data reverse proxied from an expensive backend and then
built into a page using mod_include. By reading the full response in
before doing the blocking sending it to the client, you give your
backend a fighting chance to release resources immediately rather than
ages later when the network eventually gets round to accepting the data.
When the proxy is optimised to release the backend connection when the
EOS is read from the backend, rather than after the EOS is successfully
delivered downstream, this will help for normal reverse proxied data too.
> My assumption is that we deal with heap buckets in most cases, but anyway
> this problem can be solved by checking
>
> APR_BRIGADE_EMPTY(ctx->bb) && APR_BUCKET_IS_HEAP(e)
>
> and proceed in the current way if this is not true.
I looked at this again, we can extend the optimisation beyond just the
first bucket on condition the buckets stay heap buckets, and stay
APR_BUCKET_BUFF_SIZE in length. This way if someone buffers a file (or
other potentially large bucket), the copy is avoided, and we stick with
a move.
Regards,
Graham
--
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
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/16/2008 12:58 AM, Graham Leggett wrote:
> Ruediger Pluem wrote:
>
>>> + /* 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?
>
> In theory no, because the brigade you are passing is empty. You can send
> empty brigades in any order, it will make no difference. Adding an empty
> brigade to our buffer is effectively a noop.
>
> Thinking about this, sending an empty brigade down the filter stack
> might be pointless as well, could we just return APR_SUCCESS? (Unless
> there is an expectation that an empty brigade should hit all the filters
> in the stack, in which case the code is correct as it is.)
I would tend to say that an empty brigade should hit the filters as well.
I would say lets continue with the code as is and see if we hit any problems.
>
>>> + /* 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.
>
> Hmm... I am not sure. If the rest of the filter stack chose not to
> consume the whole brigade for whatever reason, the unconsumed content
> would remain in the buffer ready for the next pass, or would be cleaned
> up when the request is aborted.
>
> I could find no precedent in any of the other filters for proactively
> emptying the brigade once it had been passed down the stack. In 99.9% of
> cases, you would be emptying an empty brigade, which is a waste.
As said it is a sanity measure. If a filter decides not to consume the whole
brigade it must set it aside and store it somewhere in the context.
apr_brigade_cleanup ensures that nothing bad happens if the filter down
the chain forgot to remove the set aside buckets from the passed brigade.
Yes, this would be a bug in downstream filter.
I admit that filters normally don't do this and only handlers implement
this safety measure. So it is debatable whether to do it or not.
>
>> 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.
>
> I disagree.
>
> The purpose of this module was to handle a page that consists of many
> (tens to hundreds) of mod_include files, in a nested fashion. The output
> on the wire consists of lots of short (< 10 bytes) file buckets, and
> lots of short file buckets that have been morphed into short heap
> buckets by mod_include (thank you indentation). This ends up on the wire
> as lots of chunks, each a few bytes long, taking up 8k * number of
> few-bytes-chunks per request in RAM, and wasting lots of bandwidth in
> chunk headers.
>
> If file buckets were passed as is, much of the optimisation would be
> lost, because you would have a short-and-inefficient heap bucket,
> followed by a short file bucket, followed by a short-and-inefficient
> heap bucket. Using the current implementation, all three buckets are
> collapsed down to just one.
>
> To be clear on what the filter does, it implements a buffer, and it
> works like a buffer according got the principle of least astonishment.
> It is completely optional, and not enabled by default.
>
> The module does not pretend to be relevant in every use case, and is
> particularly not relevant for cases that are efficient already (simple
> static files + sendfile, for example).
Thanks for clarification. Nevertheless we should make it more clear in the
documentation that this module can make performance worse if used in the
wrong context, e.g. with static files. Otherwise users might think that
buffering files via mod_buffer could be a cool performance boost which
it is not.
>
>> 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).
>
> Again, we are trying to second guess what the admin is trying to do.
>
> If the response contained large heap buckets, then the correct course of
> action is for the admin to have not added the buffer filter in the first
> place.
You can have a mixture of both because there can be include files in your
nested include files that have a larger chunk while many other are as small
as you describe. IMHO by taking care of this case you don't loose any of your
buffer features as the resulting brigade is nearly the same with the difference
that we just recreated and mem copied the buckets we had before. The only
difference that matters appears in the case of a small file bucket that did
not fit in the previous heap bucket.
IMHO you put too much burden on the administrator to know the internals of
httpd in order to make a good decision whether to use mod_buffer or not.
>
> The point behind the buffer is to convert whatever buckets are present
> into the smallest number of heap buckets as possible, and do this until
> the buffer is full, or the response is complete, whichever comes first.
>
> Think of a potentially expensive bucket of known but long length - we
> certainly don't want to blindly move the expensive bucket over in this
> case, as the bucket will only get read at some point in the distance
> future when the slow client gets round to read the bucket, and the whole
> purpose of buffering is lost.
Could you please elaborate of which kind of expensive bucket you are talking
about?
>
> The contract of mod_buffer is simple: pack the response (whatever it is)
> into as few heap buckets as possible to guarantee that expensive buckets
> are read as soon as possible, and that as few chunks appear on the wire
> as possible. The purpose of the buffer is to save RAM and network
> bandwidth, not save CPU time.
>
> I think trying to second guess the optimisation already present in
> apr_brigade_write is a bad idea.
>
>> 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.
>
> Again, this only makes sense if the bucket is already a heap bucket. If
> it is anything else, then we want to read it in immediately.
My assumption is that we deal with heap buckets in most cases, but anyway
this problem can be solved by checking
APR_BRIGADE_EMPTY(ctx->bb) && APR_BUCKET_IS_HEAP(e)
and proceed in the current way if this is not true.
>
>> 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.
>
> We are trying to be a buffer, if we just returned data as we read it,
> the buffer would be a pure passthrough and point behind buffering will
> be lost.
IMHO this deserves a note in the documentation to make clear that long
pause times can happen.
Regards
Rüdiger
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
Posted by Graham Leggett <mi...@sharp.fm>.
Ruediger Pluem wrote:
>> + /* 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?
In theory no, because the brigade you are passing is empty. You can send
empty brigades in any order, it will make no difference. Adding an empty
brigade to our buffer is effectively a noop.
Thinking about this, sending an empty brigade down the filter stack
might be pointless as well, could we just return APR_SUCCESS? (Unless
there is an expectation that an empty brigade should hit all the filters
in the stack, in which case the code is correct as it is.)
>> + /* 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.
Hmm... I am not sure. If the rest of the filter stack chose not to
consume the whole brigade for whatever reason, the unconsumed content
would remain in the buffer ready for the next pass, or would be cleaned
up when the request is aborted.
I could find no precedent in any of the other filters for proactively
emptying the brigade once it had been passed down the stack. In 99.9% of
cases, you would be emptying an empty brigade, which is a waste.
>> + /* 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?
I am still looking at the possibility of optionally adding an Etag when
missing to requests that fit in the buffer, and when that happens, it
would go in the EOS path, and not the flush path.
> 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.
I disagree.
The purpose of this module was to handle a page that consists of many
(tens to hundreds) of mod_include files, in a nested fashion. The output
on the wire consists of lots of short (< 10 bytes) file buckets, and
lots of short file buckets that have been morphed into short heap
buckets by mod_include (thank you indentation). This ends up on the wire
as lots of chunks, each a few bytes long, taking up 8k * number of
few-bytes-chunks per request in RAM, and wasting lots of bandwidth in
chunk headers.
If file buckets were passed as is, much of the optimisation would be
lost, because you would have a short-and-inefficient heap bucket,
followed by a short file bucket, followed by a short-and-inefficient
heap bucket. Using the current implementation, all three buckets are
collapsed down to just one.
To be clear on what the filter does, it implements a buffer, and it
works like a buffer according got the principle of least astonishment.
It is completely optional, and not enabled by default.
The module does not pretend to be relevant in every use case, and is
particularly not relevant for cases that are efficient already (simple
static files + sendfile, for example).
> 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).
Again, we are trying to second guess what the admin is trying to do.
If the response contained large heap buckets, then the correct course of
action is for the admin to have not added the buffer filter in the first
place.
The point behind the buffer is to convert whatever buckets are present
into the smallest number of heap buckets as possible, and do this until
the buffer is full, or the response is complete, whichever comes first.
Think of a potentially expensive bucket of known but long length - we
certainly don't want to blindly move the expensive bucket over in this
case, as the bucket will only get read at some point in the distance
future when the slow client gets round to read the bucket, and the whole
purpose of buffering is lost.
The contract of mod_buffer is simple: pack the response (whatever it is)
into as few heap buckets as possible to guarantee that expensive buckets
are read as soon as possible, and that as few chunks appear on the wire
as possible. The purpose of the buffer is to save RAM and network
bandwidth, not save CPU time.
I think trying to second guess the optimisation already present in
apr_brigade_write is a bad idea.
> 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.
Again, this only makes sense if the bucket is already a heap bucket. If
it is anything else, then we want to read it in immediately.
>> + 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.
This should go in a context, you're right. Will fix.
>> +
>> + 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.
This is a bug, will fix.
>> + 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.
Will fix.
> 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.
We are trying to be a buffer, if we just returned data as we read it,
the buffer would be a pure passthrough and point behind buffering will
be lost.
The EAGAIN case isn't currently handled properly though, I will fix this.
Regards,
Graham
--