[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apache-httpd-dev
Subject:    Re: ap_request_core_filter issues
From:       Ruediger Pluem <rpluem () apache ! org>
Date:       2018-10-23 18:53:46
Message-ID: 5ea1cdd2-733c-36c4-998f-335f9e7db7fc () apache ! org
[Download RAW message or body]



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@vodafone.com> wrote:
> > 
> > > -----Ursprüngliche Nachricht-----
> > > Von: Yann Ylavic <ylavic.dev@gmail.com>
> > > 
> > > 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.

Regards

Rüdiger


[prev in list] [next in list] [prev in thread] [next in thread]