You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Greg Stein <gs...@lyra.org> on 2002/09/23 01:34:59 UTC
Re: cvs commit: apr-util CHANGES
On Sat, Sep 21, 2002 at 11:40:41PM -0000, brianp@apache.org wrote:
>...
> +{
> + apr_bucket *e = APR_BRIGADE_LAST(b);
> + apr_size_t remaining;
> + char *buf;
> + apr_size_t bytes_written = 0;
> + apr_size_t i;
> +
> + /* Step 1: check if there is a heap bucket at the end
> + * of the brigade already
> + */
> + if (!APR_BRIGADE_EMPTY(b) && APR_BUCKET_IS_HEAP(e)) {
> + apr_bucket_heap *h = e->data;
> + remaining = h->alloc_len - (e->length + (apr_size_t)e->start);
> + buf = h->base + e->start + e->length;
> + }
> + else {
> + remaining = 0;
> + buf = NULL;
> + }
> +
> + /* Step 2: copy the data into the heap bucket, appending
> + * a new heap bucket each time the old one becomes full
> + */
> + for (i = 0; i < nvec; i++) {
> + apr_size_t nbyte = vec[i].iov_len;
> + const char *str = (const char *)(vec[i].iov_base);
> +
> + bytes_written += nbyte;
> + if (nbyte <= remaining) {
> + memcpy(buf, str, nbyte);
> + e->length += nbyte;
> + buf += nbyte;
> + remaining -= nbyte;
> + }
> + else if (nbyte < APR_BUCKET_BUFF_SIZE) {
> + buf = apr_bucket_alloc(APR_BUCKET_BUFF_SIZE, b->bucket_alloc);
> + e = apr_bucket_heap_create(buf, APR_BUCKET_BUFF_SIZE,
> + apr_bucket_free, b->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(b, e);
> + memcpy(buf, str, nbyte);
> + e->length = nbyte;
> + buf += nbyte;
> + remaining = APR_BUCKET_BUFF_SIZE - nbyte;
> + }
> + else { /* String larger than APR_BUCKET_BUFF_SIZE */
> + e = apr_bucket_transient_create(str, nbyte, b->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(b, e);
> + remaining = 0; /* create a new heap bucket for the next write */
> + }
Ewww... icky implementation... If you have a dozen iovec's where each
iov_len is slightly less than APR_BUCKET_BUFF_SIZE, then you're going to
copy the entire thing onto the heap.
> + /* Step 3: if necessary, output the brigade contents now
> + */
> + if (bytes_written >= APR_BUCKET_BUFF_SIZE) {
> + return flush(b, ctx);
> + }
And *then* you're going to flush the thing.
If you total up the number of bytes represented by the iovec, and that total
is more than APR_BUCKET_BUFF_SIZE, then simply create <nvec> TRANSIENT
buckets and flush the thing. No copies at all.
If the total is less, then go ahead and shove the iovec's onto the existing
HEAP bucket, and/or one or two new HEAP buckets (you can show that you'll
never need more than two (existing/created) buckets altogether).
I also think that doing it this way will also greatly simplify the code.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr-util CHANGES
Posted by Brian Pane <br...@cnet.com>.
On Mon, 2002-09-23 at 03:30, Greg Stein wrote:
> On Sun, Sep 22, 2002 at 07:00:42PM -0700, Brian Pane wrote:
> > On Sun, 2002-09-22 at 16:34, Greg Stein wrote:
> >...
> > All the copying doesn't bother me as much, because it's the same
> > thing that would happen if you did a dozen calls to apr_brigade_write()
> > for slightly less than APR_BUCKET_BUFF_SIZE bytes each. So the copying
> > overhead in this case would be no worse than apr_brigade_write().
>
> That's not an argument :-) You're only comparing brigade_writev() against
> *prior* usage which did a bunch of _write() calls. So what?
So there's no performance penalty for using the current implementation
on large buffers, compared to the alternate way of writing large
buffers.
That's important because, if we were to change it to precompute the
total length, there likely *would* be a performance penalty for small
buffers (an extra O(n) loop through the iovec). And small buffers
are the common use case right now.
Brian
Re: cvs commit: apr-util CHANGES
Posted by Cliff Woolley <jw...@virginia.edu>.
On Mon, 23 Sep 2002, Greg Stein wrote:
> The setaside is not your problem. You cannot and should not guess what is
> going to or *might* happen. The brigades are supposed to be about *avoiding*
> copies. If/when a copy *must* be made, then fine. But don't make judgements
> in advance because you *don't have the info*.
>
> If the core_output filter is in use, then let it combine the small buckets.
> Don't do it up front. There are too many other things that might happen, and
> it isn't right to pre-judge.
I'm with Greg on this one. Just make a bunch of transient buckets and
pass the brigade.
--Cliff
Re: cvs commit: apr-util CHANGES
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Sep 22, 2002 at 07:00:42PM -0700, Brian Pane wrote:
> On Sun, 2002-09-22 at 16:34, Greg Stein wrote:
>...
> All the copying doesn't bother me as much, because it's the same
> thing that would happen if you did a dozen calls to apr_brigade_write()
> for slightly less than APR_BUCKET_BUFF_SIZE bytes each. So the copying
> overhead in this case would be no worse than apr_brigade_write().
That's not an argument :-) You're only comparing brigade_writev() against
*prior* usage which did a bunch of _write() calls. So what?
If we code some new stuff using the new _writev(), then you've got a bunch
of copying occurring. And if the total size of your _writev() is going to
cause a flush, then you shouldn't do *any* copying. Just pass it to the
flush function and be done with it.
> > If you total up the number of bytes represented by the iovec, and that total
> > is more than APR_BUCKET_BUFF_SIZE, then simply create <nvec> TRANSIENT
> > buckets and flush the thing. No copies at all.
>
> That approach would create some different problems, though. Consider
> a case where the vector consists of a few dozen small strings followed
> by a large buffer--for example, a lot of small bits of text comprising
> a response header, followed by a >8KB buffer containing the response
> body. If we create two dozen transient buckets, we could end up doing
> extra copying: a copy per bucket if any filter tries to setaside the
> brigade, plus a copy in the core_output filter as it tries to combine
> the many tiny buckets into one larger buffer.
The setaside is not your problem. You cannot and should not guess what is
going to or *might* happen. The brigades are supposed to be about *avoiding*
copies. If/when a copy *must* be made, then fine. But don't make judgements
in advance because you *don't have the info*.
If the core_output filter is in use, then let it combine the small buckets.
Don't do it up front. There are too many other things that might happen, and
it isn't right to pre-judge.
If those little buckets bother you, then write an aggregation filter. But
you shouldn't be monkeying with the core functions because of suppositions
about how the system is set up or what it is going to do.
apr_brigade_writev() knows nothing about the core_output filter. And it
shouldn't. Buckets may or may not be set aside; hell... the brigades might
not even be involved in a filter chain!
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr-util CHANGES
Posted by Brian Pane <br...@apache.org>.
On Sun, 2002-09-22 at 16:34, Greg Stein wrote:
> Ewww... icky implementation... If you have a dozen iovec's where each
> iov_len is slightly less than APR_BUCKET_BUFF_SIZE, then you're going to
> copy the entire thing onto the heap.
>
> > + /* Step 3: if necessary, output the brigade contents now
> > + */
> > + if (bytes_written >= APR_BUCKET_BUFF_SIZE) {
> > + return flush(b, ctx);
> > + }
>
> And *then* you're going to flush the thing.
That part definitely is a problem. The apr_brigade_writev() code
needs to flush the brigade each time it overflows the previous
heap buffer. I'll add a fix for that.
All the copying doesn't bother me as much, because it's the same
thing that would happen if you did a dozen calls to apr_brigade_write()
for slightly less than APR_BUCKET_BUFF_SIZE bytes each. So the copying
overhead in this case would be no worse than apr_brigade_write().
> If you total up the number of bytes represented by the iovec, and that total
> is more than APR_BUCKET_BUFF_SIZE, then simply create <nvec> TRANSIENT
> buckets and flush the thing. No copies at all.
That approach would create some different problems, though. Consider
a case where the vector consists of a few dozen small strings followed
by a large buffer--for example, a lot of small bits of text comprising
a response header, followed by a >8KB buffer containing the response
body. If we create two dozen transient buckets, we could end up doing
extra copying: a copy per bucket if any filter tries to setaside the
brigade, plus a copy in the core_output filter as it tries to combine
the many tiny buckets into one larger buffer.
Brian