You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2018/10/18 11:52:49 UTC
ap_request_core_filter issues
On Thu, Oct 18, 2018 at 12:51:06PM +0200, Ruediger Pluem wrote:
> >>>
> >>> Curiously inefficient writev use when stracing the process, though,
> >>> dunno what's going on there (trunk/prefork):
> >>>
> >>> writev(46, [{iov_base="\r\n", iov_len=2}], 1) = 2
> >>> writev(46, [{iov_base="1f84\r\n", iov_len=6}], 1) = 6
> >>> writev(46, [{iov_base="<D:lockdiscovery/>\n<D:getcontent"..., iov_len=7820}], 1) = 7820
> >>> writev(46, [{iov_base="<D:supportedlock>\n<D:lockentry>\n"..., iov_len=248}], 1) = 248
> >>
> >> The reason is ap_request_core_filter. It iterates over the brigade and hands over each bucket alone to
> >> ap_core_output_filter. IMHO a bug.
> >
> > How about the attached patch for fixing?
>
> Scratch that. I guess it is much more complex.
Hmm, well it works for me and looks correct, it batches up those the
writev(,,1) calls as expected, but it seems YMMV :)
I notice the function is doing unconditional blocking reads without
flushing first - rule 10 of the golden rules of output filters! --
status = apr_bucket_read(bucket, &data, &size, APR_BLOCK_READ);
Re: ap_request_core_filter issues
Posted by Ruediger Pluem <rp...@apache.org>.
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
> <ru...@vodafone.com> wrote:
>>
>>> -----Ursprüngliche Nachricht-----
>>> Von: Yann Ylavic <yl...@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
Re: ap_request_core_filter issues
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Oct 23, 2018 at 3:52 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> 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)
See attached.
Re: ap_request_core_filter issues
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Oct 23, 2018 at 3:18 PM Plüm, Rüdiger, Vodafone Group
<ru...@vodafone.com> wrote:
>
> > -----Ursprüngliche Nachricht-----
> > Von: Yann Ylavic <yl...@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 :)
> 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,
Yann.
>
>
> Regards
>
> Rüdiger
>
AW: ap_request_core_filter issues
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic <yl...@gmail.com>
> Gesendet: Dienstag, 23. Oktober 2018 13:09
> An: httpd-dev <de...@httpd.apache.org>
> Betreff: Re: ap_request_core_filter issues
>
> On Thu, Oct 18, 2018 at 1:52 PM Joe Orton <jo...@redhat.com> wrote:
> >
> > On Thu, Oct 18, 2018 at 12:51:06PM +0200, Ruediger Pluem wrote:
> > > >>>
> > > >>> Curiously inefficient writev use when stracing the process,
> though,
> > > >>> dunno what's going on there (trunk/prefork):
> > > >>>
> > > >>> writev(46, [{iov_base="\r\n", iov_len=2}], 1) = 2
> > > >>> writev(46, [{iov_base="1f84\r\n", iov_len=6}], 1) = 6
> > > >>> writev(46, [{iov_base="<D:lockdiscovery/>\n<D:getcontent"...,
> iov_len=7820}], 1) = 7820
> > > >>> writev(46, [{iov_base="<D:supportedlock>\n<D:lockentry>\n"...,
> iov_len=248}], 1) = 248
> > > >>
> > > >> The reason is ap_request_core_filter. It iterates over the
> brigade and hands over each bucket alone to
> > > >> ap_core_output_filter. IMHO a bug.
> > > >
> > > > How about the attached patch for fixing?
> > >
> > > Scratch that. I guess it is much more complex.
> >
> > Hmm, well it works for me and looks correct, it batches up those the
> > writev(,,1) calls as expected, but it seems YMMV :)
>
> After a closer look into this, Rüdiger's patch seems indeed incomplete.
> What we possibly want is keep buckets in tmp_bb until it's time to
> pass it down the chain, per ap_filter_reinstate_brigade() logic (i.e.
> FLUSH bucket, size limit, ...).
>
> 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.
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.
Regards
Rüdiger
Re: ap_request_core_filter issues
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 18, 2018 at 1:52 PM Joe Orton <jo...@redhat.com> wrote:
>
> On Thu, Oct 18, 2018 at 12:51:06PM +0200, Ruediger Pluem wrote:
> > >>>
> > >>> Curiously inefficient writev use when stracing the process, though,
> > >>> dunno what's going on there (trunk/prefork):
> > >>>
> > >>> writev(46, [{iov_base="\r\n", iov_len=2}], 1) = 2
> > >>> writev(46, [{iov_base="1f84\r\n", iov_len=6}], 1) = 6
> > >>> writev(46, [{iov_base="<D:lockdiscovery/>\n<D:getcontent"..., iov_len=7820}], 1) = 7820
> > >>> writev(46, [{iov_base="<D:supportedlock>\n<D:lockentry>\n"..., iov_len=248}], 1) = 248
> > >>
> > >> The reason is ap_request_core_filter. It iterates over the brigade and hands over each bucket alone to
> > >> ap_core_output_filter. IMHO a bug.
> > >
> > > How about the attached patch for fixing?
> >
> > Scratch that. I guess it is much more complex.
>
> Hmm, well it works for me and looks correct, it batches up those the
> writev(,,1) calls as expected, but it seems YMMV :)
After a closer look into this, Rüdiger's patch seems indeed incomplete.
What we possibly want is keep buckets in tmp_bb until it's time to
pass it down the chain, per ap_filter_reinstate_brigade() logic (i.e.
FLUSH bucket, size limit, ...).
I tried to implement that in the attached patch, WDYT?
>
> I notice the function is doing unconditional blocking reads without
> flushing first - rule 10 of the golden rules of output filters! --
The patch also addresses this.
Regards,
Yann.