httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] buckets take 2
Date Thu, 18 Jan 2001 21:28:00 GMT
On Thu, Jan 18, 2001 at 12:41:42PM -0800, rbb@covalent.net wrote:
>
>... [ mixing APIs ] ...
> 
> Please show us how to do this then.  So far, I haven't seen a patch that
> will do this.

I believe that I described this earlier. When an ap_r* function is first
called, a coalesce filter gets added. Each ap_r* function shoves transient
buckets into the filter stack, and that coalesce filter copies it into an
internal buffer. When the buffer is full, the data is delivered to the next
filter.

Thus, all content from ap_r* and bucket-style output is properly
interleaved.

[ unfortunately, all of our strategies so far appear to copy-once content
  that is delivered via ap_r*; personally, I'm okay with that; we can refine
  and fine-tune further in the future. ]

>...
> > Somebody who doesn't mix APIs may still need to call it because they don't
> > know what somebody else has done. In fact, your latest patch puts a call
> > right into ap_pass_brigade() to standardize every single brigade that passes
> > through.
> 
> Actually that call to standardize has been there since the very first
> patch.

I was off in Chicago earlier over the weekend and early this week. I'm still
catching up and provide proper and accurate review to the patches. You've
been cranking them out :-) based on other feedback, so I've only had a
chance to carefully look at the latest. At this point, I've only hit on the
showstopper elements of the patch; there are other nigglies that aren't
important.

[ and as a result of looking into this, I've noticed that
  ap_brigade_vprintf() is broken right now(!) ]

>...
> 1)  Going to the direct bucket API requires a change in thinking,
> requiring a specific call to ap_brigade_standardize doesn't bother me,
> because it emphasizes that change in thinking.

Yah, but I don't think we need this step. The buffer that you've put at the
brigade level can just as easily hang out at the tail of the brigade.
Semantically, that is what you're reaching for, and it can be implemented
that way. It will also reduce confusion and potential error if somebody
forgets to call standardize()

Consider that this latest patch has *two* ways to lose out on the
synchronization:

1) r->bb might not be flushed at the right time
2) b->start/end might not be inserted into the brigade at the right time

[ the insert-into-tail-bucket solves (2); the coalesce filter solves (1) ]

> 2)  Think C Run-Time API's.  If you want to mix fwrite with write, you
> have to call fflush, or the data will be out of order.  This is exactly
> what we are doing here.  The ap_r* functions (and ap_brigade_*
> functions) are a buffering API.  If you want to go to the non-bufferred
> API, there is a flush call that you must use.

Agreed, but I don't see a reason to even let this happen.

> This is the exact same thing that happened in 1.3, if you wanted to use a
> buffered API, you could use ap_r* or ap_b* and they just worked, but if
> you didn't want to buffer, it was possible to write directly to the
> network with write or send.  If you wanted to switch between the two, you
> had call either ap_rflush or ap_bflush.  There is no difference with what
> I have written.

Handlers always used ap_r* to write to the network. We didn't skip around
because that would corrupt the output. So I'm not sure how this applies.

>...
> > I think we're probably quite fine with the existing implementations of
> > ap_send_fd() and ap_send_mmap(). The problem we're fixing deals with the
> > "little bits" of ap_r*().
> 
> No, we aren't just fine with those functions.  Because those functions
> need to work with ap_r*, there is a bit of work that needs to be done to
> ensure that the data is inserted into the array in the correct
> order.  If we apply the current patch, we will need to add calls to
> ap_brigade_standardize.

Ah. You're right... these would fail along sync-error (1) mentioned above.
So yes, they would need patching, too.

> > > But it doesn't work in general, because it easily allows for brigades like
> > > the following:
> > > 
> > > 5 byte heap -> 10 byte transient -> 5 byte heap -> 10 byte transient
> > > 
> > > Also, to really make the idea work, the HEAP and POOL buckets were always
> > > 4K buckets so we wasted a lot of memory.
> > 
> > The buckets would only need to be 4K if ap_r* were used. The above brigade
> > wouldn't really happen if somebody was using ap_r*, as the content would be
> > getting placed into HEAP bucket at the end of the brigade.
> > 
> > Again: using your original thought of appending-to-last-bucket will solve
> > the "variant brigade" issue, and remove any need for the standardize()
> > calls.
> 
> You lost me here.  The bucket has to be 4K regardless of the API, because
> otherwise it won't have any extra space to grow into.  If you ever put a
> heap bucket on the brigade that is the exact size of the data you are
> adding, you remove the ability to append data to that bucket (unless you
> want to call realloc, which you don't want to do).

If the bucket is inserted (by ap_r*) for buffering purposes, then it would
be 4K. Normal bucket users wouldn't have the memory overhead.

If you interleaved bucket operations and ap_r* calls on that brigade, then
you'd end up with a mix of buckets, but as long as the module keeps calling
ap_r*, we can append the data into the last bucket of the brigade.

I think the best way to look at it, is as an analogue to what you're doing
now: the buffer in the brigade structure itself simply moves down to the
tail of the brigade. If somebody adds a bucket to the brigade, then:

*) in the brigade-buffer approach, you'd have an ordering error
*) in the bucket-buffer approach, you'd lose out on the opportunity to
   continue filling the buffer, but the ordering would be fine

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message