Re: [PATCH] Avoid unnecessary brigade splits on core input and outputfilters. WAS: EOS or FLUSH buckets
On Thu, 12 Jun 2003, Justin Erenkrantz wrote: > What would happen if e weren't there and it were passed to > APR_RING_SPLIT_TAIL? -- justin By "weren't there," I assume you mean "were in some other brigade". Then the supposed sequence of buckets represented by APR_BRIGADE_FIRST(ctx->b)...APR_BUCKET_PREV(e) would be broken, since if bucket e is not in ctx->b then neither is APR_BUCKET_PREV(e). Inserting this into brigade b would cause b to be a broken ring, and the fixup code would end up halfway inserting e into ctx->b. In other words, it would be bad. :) --Cliff
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Title: RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets Cliff, I tested your code in core.c output filter and it works well. Juan -Original Message- From: Cliff Woolley [mailto:[EMAIL PROTECTED]] Sent: Thursday, June 12, 2003 5:41 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets On Thu, 12 Jun 2003, Justin Erenkrantz wrote: > I suggest that this be created as a macro called > APR_BRIGADE_CONCAT_UNTIL() as I don't think understanding apr_ring.h > should be a pre-req for writing filters. ;-) -- justin I'm fine with that... I'll try to find some time tonight to add it to apr_ring.h and apr_brigade.h and test it. --Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Thu, Jun 12, 2003 at 05:55:10PM -0400, Cliff Woolley wrote: > I should point out that the above code does one thing that my code does > not: it allows for bucket e to be completely absent from ctx->b. (In > which case ctx->b would be left completely empty at the end of the loop.) > My code assumes that e is guaranteed to be somewhere in brigade ctx->b. > (In which case ctx->b will have at least one bucket (e) left in it at the > end of the loop.) What would happen if e weren't there and it were passed to APR_RING_SPLIT_TAIL? -- justin
Re: [PATCH] Avoid unnecessary brigade splits on core input and outputfilters. WAS: EOS or FLUSH buckets
> > On Thu, 12 Jun 2003, Justin Erenkrantz wrote:
> >
> > > for (bucket = APR_BUCKET_FIRST(ctx->b);
> > > bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
> > > bucket = APR_BUCKET_NEXT(bucket)) {
> > >apr_bucket_remove(bucket);
> > >APR_BRIGADE_INSERT_TAIL(b, bucket);
> > > }
PS: if you *were* going to write it this way, then it would actually be:
for (bucket = APR_BRIGADE_FIRST(ctx->b);
bucket != e && bucket != APR_BRIGADE_SENTINEL(ctx->b);
bucket = APR_BRIGADE_FIRST(ctx->b))
{
apr_bucket_remove(bucket);
APR_BRIGADE_INSERT_TAIL(b, bucket);
}
The second line has to test against the sentinel, not APR_BRIGADE_LAST(),
because otherwise the last bucket in the brigade could never be copied.
The third line has to always use APR_BRIGADE_FIRST(), because after an
iteration of the loop, APR_BUCKET_NEXT(bucket) now points to
APR_BRIGADE_SENTINEL(b) [since bucket is now the last bucket in b], not to
the bucket you really want, which is the one that's now the first bucket
in ctx->b.
I should point out that the above code does one thing that my code does
not: it allows for bucket e to be completely absent from ctx->b. (In
which case ctx->b would be left completely empty at the end of the loop.)
My code assumes that e is guaranteed to be somewhere in brigade ctx->b.
(In which case ctx->b will have at least one bucket (e) left in it at the
end of the loop.)
--Cliff
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Thu, 12 Jun 2003, Juan Rivera wrote: > Is your code assuming that b is empty? If so, I'm not sure we can make that > assumption. Nope, that is not an assumption. It works regardless of how many buckets are in brigade b. --Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and outputfilters. WAS: EOS or FLUSH buckets
On Thu, 12 Jun 2003, Justin Erenkrantz wrote: > I suggest that this be created as a macro called > APR_BRIGADE_CONCAT_UNTIL() as I don't think understanding apr_ring.h > should be a pre-req for writing filters. ;-) -- justin I'm fine with that... I'll try to find some time tonight to add it to apr_ring.h and apr_brigade.h and test it. --Cliff
RE: [PATCH] Avoid unnecessary brigade splits on core input and ou tput filters. WAS: EOS or FLUSH buckets
Title: RE: [PATCH] Avoid unnecessary brigade splits on core input and output
filters. WAS: EOS or FLUSH buckets
My bad, never mind. You are fixing up the
ctx->b pointers. I'm going to try it out.
-Original Message-
From: Juan Rivera
[mailto:[EMAIL PROTECTED]
Sent: Thursday, June 12, 2003 5:25
PM
To: '[EMAIL PROTECTED]'
Subject: RE: [PATCH] Avoid
unnecessary brigade splits on core input and ou tput filters. WAS: EOS or FLUSH
buckets
Cliff,
Is your code assuming that b is empty? If so, I'm not
sure we can make that assumption.
-Original Message-
From: Cliff Woolley [mailto:[EMAIL PROTECTED]]
Sent: Thursday, June 12, 2003 5:08
PM
To: [EMAIL PROTECTED]
Subject: Re: [PATCH] Avoid
unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH
buckets
On Thu, 12 Jun 2003, Justin Erenkrantz wrote:
> for (bucket = APR_BUCKET_FIRST(ctx->b);
>
bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
>
bucket = APR_BUCKET_NEXT(bucket)) {
>
apr_bucket_remove(bucket);
>
APR_BRIGADE_INSERT_TAIL(b, bucket);
> }
No! Bad!! The whole beauty of the ring
data structure is that all of
these operations can be done in
constant time, no loops. Bear with me,
and I'll explain.
> If you understand the type safety checks it is
attempting, you are a far
> more intelligent person than
I. =)
Well, I dunno about that... I do understand them but
then again I've
focused on them since literally the
day I started contributing to this lil
ole web server.
The following code assumes that ctx->b has at least
one bucket in it,
namely e.
if (APR_BRIGADE_FIRST(ctx->b) != e)
{
/* move the
sequence into the new brigade */
APR_RING_SPLICE_TAIL(&b->list,
APR_BRIGADE_FIRST(ctx->b),
APR_BUCKET_PREV(e),
apr_bucket, link);
/* fixup the dangling pointers in
ctx->b */
APR_BRIGADE_FIRST(ctx->b) = e;
APR_BUCKET_PREV(e) = APR_BRIGADE_SENTINEL(ctx->b);
}
Lovely, eh? I didn't actually test this to make
sure it's 100% right, but
conceptually I believe it should do
the trick.
--Cliff
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Title: RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Cliff,
Is your code assuming that b is empty? If so, I'm not sure we can make that assumption.
-Original Message-
From: Cliff Woolley [mailto:[EMAIL PROTECTED]]
Sent: Thursday, June 12, 2003 5:08 PM
To: [EMAIL PROTECTED]
Subject: Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Thu, 12 Jun 2003, Justin Erenkrantz wrote:
> for (bucket = APR_BUCKET_FIRST(ctx->b);
> bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
> bucket = APR_BUCKET_NEXT(bucket)) {
> apr_bucket_remove(bucket);
> APR_BRIGADE_INSERT_TAIL(b, bucket);
> }
No! Bad!! The whole beauty of the ring data structure is that all of
these operations can be done in constant time, no loops. Bear with me,
and I'll explain.
> If you understand the type safety checks it is attempting, you are a far
> more intelligent person than I. =)
Well, I dunno about that... I do understand them but then again I've
focused on them since literally the day I started contributing to this lil
ole web server.
The following code assumes that ctx->b has at least one bucket in it,
namely e.
if (APR_BRIGADE_FIRST(ctx->b) != e)
{
/* move the sequence into the new brigade */
APR_RING_SPLICE_TAIL(&b->list,
APR_BRIGADE_FIRST(ctx->b),
APR_BUCKET_PREV(e),
apr_bucket, link);
/* fixup the dangling pointers in ctx->b */
APR_BRIGADE_FIRST(ctx->b) = e;
APR_BUCKET_PREV(e) = APR_BRIGADE_SENTINEL(ctx->b);
}
Lovely, eh? I didn't actually test this to make sure it's 100% right, but
conceptually I believe it should do the trick.
--Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Thu, Jun 12, 2003 at 05:07:32PM -0400, Cliff Woolley wrote:
> On Thu, 12 Jun 2003, Justin Erenkrantz wrote:
>
> > for (bucket = APR_BUCKET_FIRST(ctx->b);
> > bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
> > bucket = APR_BUCKET_NEXT(bucket)) {
> >apr_bucket_remove(bucket);
> >APR_BRIGADE_INSERT_TAIL(b, bucket);
> > }
>
> No! Bad!! The whole beauty of the ring data structure is that all of
> these operations can be done in constant time, no loops. Bear with me,
> and I'll explain.
Heh. I didn't say it was optimal, but that's the code I'd write. But,
if APR_RING_SPLICE_TAIL can produce equivalent results, then great.
> if (APR_BRIGADE_FIRST(ctx->b) != e)
> {
> /* move the sequence into the new brigade */
> APR_RING_SPLICE_TAIL(&b->list,
> APR_BRIGADE_FIRST(ctx->b),
> APR_BUCKET_PREV(e),
> apr_bucket, link);
>
> /* fixup the dangling pointers in ctx->b */
> APR_BRIGADE_FIRST(ctx->b) = e;
> APR_BUCKET_PREV(e) = APR_BRIGADE_SENTINEL(ctx->b);
> }
I suggest that this be created as a macro called
APR_BRIGADE_CONCAT_UNTIL() as I don't think understanding apr_ring.h
should be a pre-req for writing filters. ;-) -- justin
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Thu, 12 Jun 2003, Justin Erenkrantz wrote:
> for (bucket = APR_BUCKET_FIRST(ctx->b);
> bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
> bucket = APR_BUCKET_NEXT(bucket)) {
>apr_bucket_remove(bucket);
>APR_BRIGADE_INSERT_TAIL(b, bucket);
> }
No! Bad!! The whole beauty of the ring data structure is that all of
these operations can be done in constant time, no loops. Bear with me,
and I'll explain.
> If you understand the type safety checks it is attempting, you are a far
> more intelligent person than I. =)
Well, I dunno about that... I do understand them but then again I've
focused on them since literally the day I started contributing to this lil
ole web server.
The following code assumes that ctx->b has at least one bucket in it,
namely e.
if (APR_BRIGADE_FIRST(ctx->b) != e)
{
/* move the sequence into the new brigade */
APR_RING_SPLICE_TAIL(&b->list,
APR_BRIGADE_FIRST(ctx->b),
APR_BUCKET_PREV(e),
apr_bucket, link);
/* fixup the dangling pointers in ctx->b */
APR_BRIGADE_FIRST(ctx->b) = e;
APR_BUCKET_PREV(e) = APR_BRIGADE_SENTINEL(ctx->b);
}
Lovely, eh? I didn't actually test this to make sure it's 100% right, but
conceptually I believe it should do the trick.
--Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
--On Wednesday, June 11, 2003 5:14 PM -0400 Cliff Woolley
<[EMAIL PROTECTED]> wrote:
On Tue, 10 Jun 2003, Justin Erenkrantz wrote:
What you are really wanting to do is a partial concatenation of the brigade.
Something like (not there, but it could be easily added):
APR_BRIGADE_CONCAT_UNTIL(b, ctx->b, e)
I'm not sure I see exactly where you're going, but you might be able to
get what you want by doing an APR_RING_SPLICE().
Perhaps. My thought is that all of ctx->b up to e should be inserted into b.
for (bucket = APR_BUCKET_FIRST(ctx->b);
bucket != e && bucket != APR_BRIGADE_LAST(ctx->b);
bucket = APR_BUCKET_NEXT(bucket)) {
apr_bucket_remove(bucket);
APR_BRIGADE_INSERT_TAIL(b, bucket);
}
Um, something like that. It's 1AM and that's about all the pseudocode I can
write.
If APR_RING_SPLICE can do it, yea. But, I've found apr_ring.h to be a
complete black hole. If you understand the type safety checks it is
attempting, you are a far more intelligent person than I. =) -- justin
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Wed, 11 Jun 2003, Juan Rivera wrote: > the bucket previous to e and then splicing it at the tail of b. Oh, I missed this part. Try APR_RING_SPLICE_TAIL(). But don't forget you have to fix up the brigade you spliced out of, which is why APR_RING_CONCAT() does an APR_RING_INIT() on h2 after moving its contents into h1 with APR_RING_SPLICE_BEFORE(). --Cliff
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Wed, 11 Jun 2003, Juan Rivera wrote:
> I don't understand well how the ring data structure works.
Have you read apr_ring.h? I tried to heavily document that file. If you
have and it's still unclear, I'd like to know how to make it more clear.
> How would the code look like? I tried unsplicing from the first bucket to
> the bucket previous to e and then splicing it at the tail of b. But
> didn't work.
A splice basically takes one chain of ring elements (buckets in this case)
that are already tied together [you pass in pointers to the first and last
elements in this chain] and inserts it either before or after some other
element (APR_RING_SPLICE_BEFORE() and APR_RING_SPLICE_AFTER()).
--Cliff
>From apr_ring.h:
/**
* Splice the sequence ep1..epN into the ring before element lep
* (..lep.. becomes ..ep1..epN..lep..)
* @warning This doesn't work for splicing before the first element or on
* empty rings... see APR_RING_SPLICE_HEAD for one that does
* @param lep Element in the ring to splice before
* @param ep1 First element in the sequence to splice in
* @param epN Last element in the sequence to splice in
* @param link The name of the APR_RING_ENTRY in the element struct
*/
#define APR_RING_SPLICE_BEFORE(lep, ep1, epN, link) do {\
APR_RING_NEXT((epN), link) = (lep); \
APR_RING_PREV((ep1), link) = APR_RING_PREV((lep), link);\
APR_RING_NEXT(APR_RING_PREV((lep), link), link) = (ep1);\
APR_RING_PREV((lep), link) = (epN); \
} while (0)
/**
* Splice the sequence ep1..epN into the ring after element lep
* (..lep.. becomes ..lep..ep1..epN..)
* @warning This doesn't work for splicing after the last element or on
* empty rings... see APR_RING_SPLICE_TAIL for one that does
* @param lep Element in the ring to splice after
* @param ep1 First element in the sequence to splice in
* @param epN Last element in the sequence to splice in
* @param link The name of the APR_RING_ENTRY in the element struct
*/
#define APR_RING_SPLICE_AFTER(lep, ep1, epN, link) do { \
APR_RING_PREV((ep1), link) = (lep); \
APR_RING_NEXT((epN), link) = APR_RING_NEXT((lep), link);\
APR_RING_PREV(APR_RING_NEXT((lep), link), link) = (epN);\
APR_RING_NEXT((lep), link) = (ep1); \
} while (0)
RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Title: RE: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets Cliff, How would the code look like? I tried unsplicing from the first bucket to the bucket previous to e and then splicing it at the tail of b. But didn't work. I don't understand well how the ring data structure works. Juan -Original Message- From: Cliff Woolley [mailto:[EMAIL PROTECTED]] Sent: Wednesday, June 11, 2003 5:14 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets On Tue, 10 Jun 2003, Justin Erenkrantz wrote: > What you are really wanting to do is a partial concatenation of the brigade. > Something like (not there, but it could be easily added): > > APR_BRIGADE_CONCAT_UNTIL(b, ctx->b, e) I'm not sure I see exactly where you're going, but you might be able to get what you want by doing an APR_RING_SPLICE(). --Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
On Tue, 10 Jun 2003, Justin Erenkrantz wrote: > What you are really wanting to do is a partial concatenation of the brigade. > Something like (not there, but it could be easily added): > > APR_BRIGADE_CONCAT_UNTIL(b, ctx->b, e) I'm not sure I see exactly where you're going, but you might be able to get what you want by doing an APR_RING_SPLICE(). --Cliff
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
[EMAIL PROTECTED] wrote: Juan Rivera wrote: I'm seen this problem with a SOCKS protocol module I wrote. I'm including a patch that fixes this problem. It does what I mentioned below. In the input filter, it moves the buckets rather than creating a new brigade and then concatenate. In the output filter it splits the brigade after a flush bucket only if there are buckets after the flush. +1 to the piece that affects core_output_filter. I'll test and commit that part of it. done. Thanks again, Juan. Greg
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Juan Rivera wrote: I'm seen this problem with a SOCKS protocol module I wrote. I'm including a patch that fixes this problem. It does what I mentioned below. In the input filter, it moves the buckets rather than creating a new brigade and then concatenate. In the output filter it splits the brigade after a flush bucket only if there are buckets after the flush. +1 to the piece that affects core_output_filter. A small patch is often worth a thousand words. I remember seeing that code do unneccessary splits when I was working on a fd leak on keepalive connections with multiple requests. The empty brigades caused my new code to blow up for a while, so I considered it a good test case at the time. But I agree, it shouldn't split off empty brigades when they are so easy to avoid. I'll test and commit that part of it. Keep up the good work! Greg
Re: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
--On Tuesday, June 10, 2003 4:59 PM -0400 Juan Rivera <[EMAIL PROTECTED]> wrote: I'm including a patch that fixes this problem. It does what I mentioned below. In the input filter, it moves the buckets rather than creating a new brigade and then concatenate. In the output filter it splits the brigade after a flush bucket only if there are buckets after the flush. What you are really wanting to do is a partial concatenation of the brigade. Something like (not there, but it could be easily added): APR_BRIGADE_CONCAT_UNTIL(b, ctx->b, e) The difference between APR_BRIGADE_CONCAT_UNTIL and APR_BRIGADE_CONCAT would be that ctx->b may still have content if e is encountered. APR_BRIGADE_CONCAT (and APR_RING_CONCAT) are O(1), but this'd be O(n), but that's okay as it'd save the space of the new brigade in comparison to apr_brigade_split/apr_brigade_concat, I guess. You should reconstruct the AP_MODE_SPECULATIVE case to do a partial for loop rather than apr_brigade_split, etc. An APR_BRIGADE_CONCAT_UNTIL isn't enough as you want to copy the buckets, but a simple for-loop should do. Skip the APR_BRIGADE_CONCAT at the end as newbb should be empty if you follow the above strategies. I think the output FLUSH part looks okay. I'd also suggest reading over our style guide. Please watch your indention, C++-style comments, and tabs. It makes your patch almost impossible to review. Since it's attached with application/octet-stream I can't review it inline either. Back to my mad-paper-writing cave... -- justin
[PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets
Title: [PATCH] Avoid unnecessary brigade splits on core input and output filters. WAS: EOS or FLUSH buckets I'm seen this problem with a SOCKS protocol module I wrote. I'm including a patch that fixes this problem. It does what I mentioned below. In the input filter, it moves the buckets rather than creating a new brigade and then concatenate. In the output filter it splits the brigade after a flush bucket only if there are buckets after the flush. Juan -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] Sent: Tuesday, June 10, 2003 3:41 PM To: [EMAIL PROTECTED] Subject: Re: EOS or FLUSH buckets Juan Rivera wrote: > Right, my module leaks memory because the core input and output filters > split the bucket brigades. So it keeps creating more and more bucket > brigades that are not released until the connection is gone. When you see this, are we talking about a lot of HTTP requests pipelined on a single connection, or a single HTTP request that lasts a long time? > First of all, I think the split in the core input filter (READBYTES) > should be optimized because all it is doing is splitting the brigade to > concatenate it into another brigade. Wouldn't be more efficient to do a > "move buckets from brigade ctx->b to b" and avoid creating a temporary > brigade? > > So for the output side, when I send a flush, it splits the brigade. If > the flush is the last bucket, this might not be necessary, what do you > think? I'll defer these two questions to our Bucketmeister and/or efficiency experts. (Cliff? Brian?) > On the topic of EOS, I think that if the last bucket is an EOS and is > not a keep alive connection it should not hold the data but it currently > does. Maybe. But if it's not a keepalive connection, we should be sending a FLUSH bucket within microseconds, no? OK, maybe that path could be optimized. But we'd have to be careful because keepalive connections are very common. We wouldn't want to penalize the hot path by optimizing for the less common case. Greg core.c.patch Description: Binary data
Re: EOS or FLUSH buckets
On Tue, 10 Jun 2003 [EMAIL PROTECTED] wrote: > > So for the output side, when I send a flush, it splits the brigade. If > > the flush is the last bucket, this might not be necessary, what do you > > think? > > I'll defer these two questions to our Bucketmeister and/or efficiency > experts. (Cliff? Brian?) I'm trying to remember... I know much more about buckets than I do about filters. What's being described here happens in the core output filter, as I recall, which is somebodye else's ball of wax. OtherBill, Brian, Justin, etc I'll see what I can dig up out of the dusty recesses of my ole' brain here though and get back to you. :)
Re: EOS or FLUSH buckets
Juan Rivera wrote: Right, my module leaks memory because the core input and output filters split the bucket brigades. So it keeps creating more and more bucket brigades that are not released until the connection is gone. When you see this, are we talking about a lot of HTTP requests pipelined on a single connection, or a single HTTP request that lasts a long time? First of all, I think the split in the core input filter (READBYTES) should be optimized because all it is doing is splitting the brigade to concatenate it into another brigade. Wouldn't be more efficient to do a "move buckets from brigade ctx->b to b" and avoid creating a temporary brigade? So for the output side, when I send a flush, it splits the brigade. If the flush is the last bucket, this might not be necessary, what do you think? I'll defer these two questions to our Bucketmeister and/or efficiency experts. (Cliff? Brian?) On the topic of EOS, I think that if the last bucket is an EOS and is not a keep alive connection it should not hold the data but it currently does. Maybe. But if it's not a keepalive connection, we should be sending a FLUSH bucket within microseconds, no? OK, maybe that path could be optimized. But we'd have to be careful because keepalive connections are very common. We wouldn't want to penalize the hot path by optimizing for the less common case. Greg
RE: EOS or FLUSH buckets
At 10:49 AM 6/10/2003, Juan Rivera wrote: >Right, my module leaks memory because the core input and output filters split the >bucket brigades. So it keeps creating more and more bucket brigades that are not >released until the connection is gone. > >On the topic of EOS, I think that if the last bucket is an EOS and is not a keep >alive connection it should not hold the data but it currently does. Bang. That's the problem - pipelining! Pipelined responses are designed to send, say, 8 1kb images in a single packet, so all of those small 'ornamentation' elements on web pages are grabbed in a single server response. If we are keep alive and you don't want this behavior, send both a flush and and EOS bucket, and it will all clear out. But flush and EOS do mean to different things - and I don't believe this is a bug. Bill
RE: EOS or FLUSH buckets
Title: RE: EOS or FLUSH buckets
Greg,
Right, my module leaks memory because the core input and output filters split the bucket brigades. So it keeps creating more and more bucket brigades that are not released until the connection is gone.
First of all, I think the split in the core input filter (READBYTES) should be optimized because all it is doing is splitting the brigade to concatenate it into another brigade. Wouldn't be more efficient to do a "move buckets from brigade ctx->b to b" and avoid creating a temporary brigade?
So for the output side, when I send a flush, it splits the brigade. If the flush is the last bucket, this might not be necessary, what do you think?
On the topic of EOS, I think that if the last bucket is an EOS and is not a keep alive connection it should not hold the data but it currently does.
Juan
-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, June 10, 2003 10:38 AM
To: [EMAIL PROTECTED]
Subject: Re: EOS or FLUSH buckets
Juan Rivera wrote:
> Here is why I'm asking. I wrote a SOCKS proxy module. It has two
> connection records, one for the client and one for the backend server.
>
> When I received data I pass it to the other side with a flush at the
> end. It works fine with one problem. The core output filter splits the
> brigade after the flush bucket creating a new bucket brigade. This
> bucket brigade is never destroyed, consuming 16 bytes of memory
> (apr_brigade.c: line 84). This may not be a problem for short lived
> connection but it the connection is long lived the pool keeps getting
> bigger and bigger. Is there any way around this?
>
> So I though, I should use an EOS bucket instead (maybe not a good idea)
> but I found that the core output filter was setting aside my buckets.
> This section in core.c looks bogus to me:
>
> Core.c line 3884:
>
> if (nbytes + flen < AP_MIN_BYTES_TO_WRITE
> && ((!fd && !more && !APR_BUCKET_IS_FLUSH(last_e))
> || (APR_BUCKET_IS_EOS(last_e)
> && c->keepalive == AP_CONN_KEEPALIVE))) {
> /* set aside the buckets */
> }
>
> What is weird about this code is that if the last bucket in the
> bridade is an EOS, this part: ((!fd && !more &&
> !APR_BUCKET_IS_FLUSH(last_e)) will return true as long as you are not
> serving a file.
>
> But it seems that you want that to happen only if the connection
> is a keep-alive connection. Right?
Juan,
Congratulations! You have homed right in on one of the trickiest new parts of
Apache 2.0. I believe the intention here is to hang on to the data across HTTP
connections, so we can take advantage of keepalives and pipelining and
potentially pack more data into fewer TCP segments. Usually what will happen is
that at the end of a request, we look for more inbound data (the next request),
don't find any, and send a FLUSH bucket which causes the network write.
> Any suggestions how to deal with this?
Deal with what? Are you saying that we leak memory with your SOCKS module?
Greg
Re: EOS or FLUSH buckets
Juan Rivera wrote:
Here is why I'm asking. I wrote a SOCKS proxy module. It has two
connection records, one for the client and one for the backend server.
When I received data I pass it to the other side with a flush at the
end. It works fine with one problem. The core output filter splits the
brigade after the flush bucket creating a new bucket brigade. This
bucket brigade is never destroyed, consuming 16 bytes of memory
(apr_brigade.c: line 84). This may not be a problem for short lived
connection but it the connection is long lived the pool keeps getting
bigger and bigger. Is there any way around this?
So I though, I should use an EOS bucket instead (maybe not a good idea)
but I found that the core output filter was setting aside my buckets.
This section in core.c looks bogus to me:
Core.c line 3884:
if (nbytes + flen < AP_MIN_BYTES_TO_WRITE
&& ((!fd && !more && !APR_BUCKET_IS_FLUSH(last_e))
|| (APR_BUCKET_IS_EOS(last_e)
&& c->keepalive == AP_CONN_KEEPALIVE))) {
/* set aside the buckets */
}
What is weird about this code is that if the last bucket in the
bridade is an EOS, this part: ((!fd && !more &&
!APR_BUCKET_IS_FLUSH(last_e)) will return true as long as you are not
serving a file.
But it seems that you want that to happen only if the connection
is a keep-alive connection. Right?
Juan,
Congratulations! You have homed right in on one of the trickiest new parts of
Apache 2.0. I believe the intention here is to hang on to the data across HTTP
connections, so we can take advantage of keepalives and pipelining and
potentially pack more data into fewer TCP segments. Usually what will happen is
that at the end of a request, we look for more inbound data (the next request),
don't find any, and send a FLUSH bucket which causes the network write.
Any suggestions how to deal with this?
Deal with what? Are you saying that we leak memory with your SOCKS module?
Greg
RE: EOS or FLUSH buckets
Title: RE: EOS or FLUSH buckets
Here is why I'm asking. I wrote a SOCKS proxy module. It has two connection records, one for the client and one for the backend server.
When I received data I pass it to the other side with a flush at the end. It works fine with one problem. The core output filter splits the brigade after the flush bucket creating a new bucket brigade. This bucket brigade is never destroyed, consuming 16 bytes of memory (apr_brigade.c: line 84). This may not be a problem for short lived connection but it the connection is long lived the pool keeps getting bigger and bigger. Is there any way around this?
So I though, I should use an EOS bucket instead (maybe not a good idea) but I found that the core output filter was setting aside my buckets. This section in core.c looks bogus to me:
Core.c line 3884:
if (nbytes + flen < AP_MIN_BYTES_TO_WRITE
&& ((!fd && !more && !APR_BUCKET_IS_FLUSH(last_e))
|| (APR_BUCKET_IS_EOS(last_e)
&& c->keepalive == AP_CONN_KEEPALIVE))) {
/* set aside the buckets */
}
What is weird about this code is that if the last bucket in the bridade is an EOS, this part: ((!fd && !more && !APR_BUCKET_IS_FLUSH(last_e)) will return true as long as you are not serving a file.
But it seems that you want that to happen only if the connection is a keep-alive connection. Right?
Any suggestions how to deal with this?
Juan
-Original Message-
From: William A. Rowe, Jr. [mailto:[EMAIL PROTECTED]]
Sent: Monday, June 09, 2003 5:42 PM
To: [EMAIL PROTECTED]
Cc: '[EMAIL PROTECTED]'
Subject: Re: EOS or FLUSH buckets
At 04:29 PM 6/9/2003, Juan Rivera wrote:
>When should one use an EOS bucket vs. a FLUSH bucket?
At EOS, that's all she wrote; body or connection finished, nothing
more, don't call me, and I won't call you.
So a flush bucket is a request; please get rid of any lingering data
and push it on to the client.
I meant it's a request, consider a filter that does codepage translation,
and we have half of a multibyte character sequence. There is no way
to flush that half a character because we don't know what it will become.
So flush should be respected as far as possible, but you cannot trust
that everything is flushed. At EOS, such an incomplete translation
would have to be performed (or discarded) because there is nothing
more to come.
Bill
Re: EOS or FLUSH buckets
At 04:29 PM 6/9/2003, Juan Rivera wrote: >When should one use an EOS bucket vs. a FLUSH bucket? At EOS, that's all she wrote; body or connection finished, nothing more, don't call me, and I won't call you. So a flush bucket is a request; please get rid of any lingering data and push it on to the client. I meant it's a request, consider a filter that does codepage translation, and we have half of a multibyte character sequence. There is no way to flush that half a character because we don't know what it will become. So flush should be respected as far as possible, but you cannot trust that everything is flushed. At EOS, such an incomplete translation would have to be performed (or discarded) because there is nothing more to come. Bill
EOS or FLUSH buckets
When should one use an EOS bucket vs. a FLUSH bucket? Juan
