[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ap_request_core_filter issues

On 10/23/2018 03:52 PM, Yann Ylavic wrote:
> On Tue, Oct 23, 2018 at 3:18 PM Plüm, Rüdiger, Vodafone Group
> <ruediger.pluem@xxxxxxxxxxxx> wrote:
>>> -----Ursprüngliche Nachricht-----
>>> Von: Yann Ylavic <ylavic.dev@xxxxxxxxx>
>>> I tried to implement that in the attached patch, WDYT?
>> 1. Why removing the META_BUCKET check when reading buckets of length -1? I don't see any sense in
>>    doing an apr_bucket_read on a META_BUCKET.
> META usually have a length of 0, not -1 (so we shouldn't read them either).
> We pretty much everywhere detect morphing buckets with -1 only, META
> or not, and honestly if a META has length == -1 we probably should
> consider it's a morphing META and let read figure out :)

You got me. I should have validated my assumption from memory. All good.

>> 2. Apart from 1. I think the flow is correct, but I am a little bit worried if calling ap_filter_reinstate_brigade
>>    inside the loop iterating over the brigade has a too large performance impact as ap_filter_reinstate_brigade
>>    iterates itself over the brigade. So we are in O(n^2) with n being the size of the brigade. But to be honest I have
>>    no good idea yet how to do without. The only thing I can think of is to duplicate and merge the logic of ap_filter_reinstate_brigade
>>    inside our loop to avoid this as we could gather the stats needed for deciding up to which bucket we need to write while
>>    doing our iteration of the brigade.
> Yeah, I have a version which checks only for
> (APR_BUCKET_IS_FLUSH(bucket) || cumulative_tmp_bb_len >=
> ap_get_core_module_config()->flush_max_threshold), but I thought I
> would not open code in the first version to show the idea...

Fair enough. The first patch made the idea more clear, but this one is the one that should be used for performance reasons.

> Ideally we'd have another ap_filter_xxx helper which could take a
> bucket (and offset) to start, and also account for f->next(s)'
> *bytes_in_brigade if asked to...

Sounds sensible.