[PATCH] core_filter buffering

[Bill Stoddard <st...@raleigh.ibm.com>]

The idea behind this patch (not fully implemented) is to save contents of bucket
brigades in an array of iovecs hung off of core_filter->ctx. When the eos bucket
arrives, the iovecs in core_filter->ctx are sent to the network using
apr_sendv().  The patch also has an optimization to send the iovecs if the
number of bytes represented by the iovecs exceeds some threshold value (8192
bytes).  I can easily add other triggers to do network i/o (do not exceed x
number of iovecs, etc.).

A few observations...
First, I believe the chunked filter has a bug. The chunking headers are
allocated off the stack. ap_create_bucket_transient() just saves away the
pointers passed in to it, so the bucket is really only valid as long as the
storage it points to. To fix this, I allocated the chunking headers out of the
f->r->pool.  I can't claim to completely understand the filter philosophy, so
I'm looking on comments on the appropriateness of this solution.

Second, auto_index generates huge numbers of small chunks. Seeing a response (in
a single buffer) with 50 encoded 'chunks' is not esthetically pleasing  :-).  It
seems that we need to introduce some sort of buffering heuristics (yea, with
data copies) somewhere higher up (than core_filter) in the filter chain.
Preferably in a place where it is transparently accessable to all modules.

Third, there are still problems with this patch, related to storage lifetimes I
suspect. I've run out of time today to continue debugging..

________________________________________________
Bill Stoddard stoddard@raleigh.ibm.com

Re: [PATCH] core_filter buffering

[Tony Finch <do...@dotat.at>]

Greg Ames <gr...@raleigh.ibm.com> wrote:
>
>Is there an existing #define constant that we can just bump up?

Not in 2.0 because this area is still in serious flux.

Tony.
-- 
en oeccget g mtcaa    f.a.n.finch
v spdlkishrhtewe y    dot@dotat.at
eatp o v eiti i d.    fanf@covalent.net

Re: [PATCH] core_filter buffering

[Greg Ames <gr...@raleigh.ibm.com>]

Tony Finch wrote:

> Tony Finch <do...@dotat.at> wrote:
> >Bill Stoddard <st...@raleigh.ibm.com> wrote:
> >>
> >>The patch also has an optimization to send the iovecs if the number
> >>of bytes represented by the iovecs exceeds some threshold value
> >>(8192 bytes).
>
> I was just thinking that 9000 bytes might be a better choice for the
> threshold value, since that is the size of a Gigabit Ethernet jumbo
> frame and 6 times the size of a regular frame... Anyone have any
> gigether to benchmark with?

I like your reasoning for going to at least 9000 bytes.  I'm guessing
that 8k was somewhat arbitrary or maybe based on some OS's default
socket buffer size.  But now the "LANscape" is likely to change as
people deploy GbE, so we should be ready.

Is there an existing #define constant that we can just bump up?

Greg

Re: [PATCH] core_filter buffering

[Tony Finch <do...@dotat.at>]

Tony Finch <do...@dotat.at> wrote:
>Bill Stoddard <st...@raleigh.ibm.com> wrote:
>>
>>The patch also has an optimization to send the iovecs if the number
>>of bytes represented by the iovecs exceeds some threshold value
>>(8192 bytes).

I was just thinking that 9000 bytes might be a better choice for the
threshold value, since that is the size of a Gigabit Ethernet jumbo
frame and 6 times the size of a regular frame... Anyone have any
gigether to benchmark with?

Tony.
-- 
en oeccget g mtcaa    f.a.n.finch
v spdlkishrhtewe y    dot@dotat.at
eatp o v eiti i d.    fanf@covalent.net

Re: [PATCH] core_filter buffering

[Tony Finch <do...@dotat.at>]

Jeff Trawick <tr...@bellsouth.net> wrote:
>Tony Finch <do...@dotat.at> writes:
>> Bill Stoddard <st...@raleigh.ibm.com> wrote:
>> >
>> >There is still a bug in the chunk_filter through: the scope of
>> >chunk_hdr is still wrong. I think Jeff was going to fix it. If not,
>> >I'll fix it later.
>> 
>> Sorry, I don't see the problem. Can you explain in more detail?
>
>just committed...  chunk_hdr[] was declared in the wrong block...  it
>went out of scope before the call to ap_pass_brigade()

Ah, I see. I knew there was a good reason for not liking Greg's change
in rev 1.116 :-)

Tony.
-- 
en oeccget g mtcaa    f.a.n.finch
v spdlkishrhtewe y    dot@dotat.at
eatp o v eiti i d.    fanf@covalent.net

Re: [PATCH] core_filter buffering

[Jeff Trawick <tr...@bellsouth.net>]

Tony Finch <do...@dotat.at> writes:

> Bill Stoddard <st...@raleigh.ibm.com> wrote:
> >
> >There is still a bug in the chunk_filter through: the scope of
> >chunk_hdr is still wrong. I think Jeff was going to fix it. If not,
> >I'll fix it later.
> 
> Sorry, I don't see the problem. Can you explain in more detail?

just committed...  chunk_hdr[] was declared in the wrong block...  it
went out of scope before the call to ap_pass_brigade()
-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Re: [PATCH] core_filter buffering

[Tony Finch <do...@dotat.at>]

Bill Stoddard <st...@raleigh.ibm.com> wrote:
>
>There is still a bug in the chunk_filter through: the scope of
>chunk_hdr is still wrong. I think Jeff was going to fix it. If not,
>I'll fix it later.

Sorry, I don't see the problem. Can you explain in more detail?

Tony.
-- 
en oeccget g mtcaa    f.a.n.finch
v spdlkishrhtewe y    dot@dotat.at
eatp o v eiti i d.    fanf@covalent.net

Re: [PATCH] core_filter buffering

[Bill Stoddard <st...@raleigh.ibm.com>]

> Bill Stoddard <st...@raleigh.ibm.com> wrote:
> >
> >The idea behind this patch (not fully implemented) is to save contents of
bucket
> >brigades in an array of iovecs hung off of core_filter->ctx. When the eos
bucket
> >arrives, the iovecs in core_filter->ctx are sent to the network using
> >apr_sendv().  The patch also has an optimization to send the iovecs if the
> >number of bytes represented by the iovecs exceeds some threshold value (8192
> >bytes).  I can easily add other triggers to do network i/o (do not exceed x
> >number of iovecs, etc.).
>
> If you get a small bucket brigade (less than the write threshold)
> it may be more efficient to copy all the data into one buffer than to
> save it as an iovec; it'll certainly be more efficient to save the
> bare bucket brigade than to make an iovec of it! Keep in mind the
> length restriction on iovecs on SysV.

Okay. I've considered several different ways to introduce 'buffering' (i.e.,
copying bytes into a larger buffer) and the one that looks the most fun is to
create a buffer_filter. So that's what I'm doing. The idea is to allow the
buffer filter to be inserted anywhere in the filter chain where it is needed.
The buffer filter in many common cases will not touch bytes at all; in other
cases it will coaleasce bytes across many small buckets into a single 9K byte
bucket (or flush when eos is received).

>
> I'm not happy with the current brigade/iovec conversion API; I'm
> inclined to think it is unnecessary and should be left to the core
> filter.
>

I agree. I did not use the brigade/iovec API in my work so far.

> >A few observations...
> >First, I believe the chunked filter has a bug. The chunking headers are
> >allocated off the stack. ap_create_bucket_transient() just saves away the
> >pointers passed in to it, so the bucket is really only valid as long as the
> >storage it points to. To fix this, I allocated the chunking headers out of
the
> >f->r->pool.  I can't claim to completely understand the filter philosophy, so
> >I'm looking on comments on the appropriateness of this solution.
>
> That's completely wrong.

Okay, I see now. No problems with the notes in ap_buckets.h. There is still a
bug in the chunk_filter through: the scope of chunk_hdr  is still wrong. I think
Jeff was going to fix it. If not, I'll fix it later.

Thanks for the comments.
Bill

Re: [PATCH] core_filter buffering

[Tony Finch <do...@dotat.at>]

[I haven't looked at the patch yet because mutt won't display
content-type octet-stream attachments; I'll probably fix this...]

Bill Stoddard <st...@raleigh.ibm.com> wrote:
>
>The idea behind this patch (not fully implemented) is to save contents of bucket
>brigades in an array of iovecs hung off of core_filter->ctx. When the eos bucket
>arrives, the iovecs in core_filter->ctx are sent to the network using
>apr_sendv().  The patch also has an optimization to send the iovecs if the
>number of bytes represented by the iovecs exceeds some threshold value (8192
>bytes).  I can easily add other triggers to do network i/o (do not exceed x
>number of iovecs, etc.).

If you get a small bucket brigade (less than the write threshold)
it may be more efficient to copy all the data into one buffer than to
save it as an iovec; it'll certainly be more efficient to save the
bare bucket brigade than to make an iovec of it! Keep in mind the
length restriction on iovecs on SysV.

I'm not happy with the current brigade/iovec conversion API; I'm
inclined to think it is unnecessary and should be left to the core
filter.

>A few observations...
>First, I believe the chunked filter has a bug. The chunking headers are
>allocated off the stack. ap_create_bucket_transient() just saves away the
>pointers passed in to it, so the bucket is really only valid as long as the
>storage it points to. To fix this, I allocated the chunking headers out of the
>f->r->pool.  I can't claim to completely understand the filter philosophy, so
>I'm looking on comments on the appropriateness of this solution.

That's completely wrong. Response body data must NOT be allocated from
a pool bedause that can cause horrendous memory leaks. This situation
is exactly what the setaside() method is designed to handle. Are the
comments in ap_buckets.h not clear? Can I improve them?

>Second, auto_index generates huge numbers of small chunks. Seeing a response (in
>a single buffer) with 50 encoded 'chunks' is not esthetically pleasing  :-).  It
>seems that we need to introduce some sort of buffering heuristics (yea, with
>data copies) somewhere higher up (than core_filter) in the filter chain.
>Preferably in a place where it is transparently accessable to all modules.

There is a vague plan to make the ap_rput stuff more bucket-friendly,
which would help here. We also need to re-write mod_autoindex to use
buckets directly.

I'm going to try and look at all this more closely soon; I have a
patch in the pipeline to finalize the buckets API (allowing us to
optimise the storage layout without affecting other code) and once
I've committed that I'll be going over bucket-using code to convert it
and clean it up if necessary.

Tony.
-- 
en oeccget g mtcaa    f.a.n.finch
v spdlkishrhtewe y    dot@dotat.at
eatp o v eiti i d.    fanf@covalent.net