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 Sun, 16 Nov 2008 15:14:01 GMT


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

Mime
View raw message