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

Re: ap_request_core_filter issues

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 :)

> 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...

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...

> Regards
> Rüdiger