You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2000/11/08 15:34:13 UTC
Re: cvs commit: apache-2.0/src/main http_protocol.c
> typedef struct byterange_ctx {
> ap_bucket_brigade *bb;
> + const char *bound_head;
> + int num_ranges;
> } byterange_ctx;
>
> - curr_length = range_end - range_start + 1;
> - loc = range = apr_pcalloc(r->pool, curr_length + 1);
> + /* ### this is so bogus, but not dealing with it right now */
> + range = loc = apr_pcalloc(r->pool, range_length + 1);
Why is this bogus? We have no choice but to copy the buffer.
> + /* ### we should split() buckets rather than read() them. this
> + ### will allow us to avoid reading files or custom buckets
> + ### into memory. for example: we REALLY don't want to cycle
> + ### a 10gig file into memory just to send out 100 bytes from
> + ### the end of the file.
> + ###
> + ### content generators that used to call ap_each_byterange()
> + ### manually (thus, optimizing the output) can replace their
> + ### behavior with a new bucket type that understands split().
> + ### they can then defer reading actual content until a read()
> + ### occurs, using the split() as an internal "seek".
> + */
> +
> ap_bucket_read(e, &str, &n, AP_NONBLOCK_READ);
> + /* ### using e->length doesn't account for pipes */
Yes it does, because the read that is just above it converted the pipes
into blocks of actual data. The thing is that our file reading is bogus,
but I can't figure out how to fix it, so it has to stay as it is for now.
> while (range_start > (curr_offset + e->length)) {
> curr_offset += e->length;
> e = AP_BUCKET_NEXT(e);
> +
> if (e == AP_BRIGADE_SENTINEL(bb)) {
> break;
> }
> +
> + /* ### eventually we can avoid this */
> + ap_bucket_read(e, &str, &n, AP_NONBLOCK_READ);
How? We have just found the start of the data, so we need to have it in
memory so that we can copy it.
> - apr_cpystrn(loc, str + (range_start - curr_offset),
> - MIN_LENGTH(curr_length + 1, e->length));
> - loc += MIN_LENGTH(curr_length + 1, e->length);
> - curr_length -= MIN_LENGTH(curr_length + 1, e->length);
> +
> + /* ### we should use 'n', not e->length */
> + segment_length = MIN_LENGTH(curr_length + 1, e->length);
It doesn't matter if we use n or e->length, because the reads that you
want to get rid of make them equivalent.
Can I assume that the only difference in the output is that we now add the
content length correctly? If so, I had an idea last night that I think is
slightly cleaner than this. If we put back the original byterange filter,
and add it to the filter stack before the content-length filter for all
requests, it can check very quickly to determine if it should be
active. If not, it just removes itself. If so, it does the byterange
magic. Then, we can rely on the content-length filter to compute the C-L
correctly everytime.
I figured this out late last night, and I kind of like it because it
re-uses code, and allows us to get aways with just having a single place
to calculate the C-L, which is a VERY good thing IMHO.
If there were more differences, (other than general cleanup), please let
me know. I would love to know what the originl problem was.
Overall, very nice patch. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by rb...@covalent.net.
> Um. I didn't explain well, then. Defining copy() and split() for PIPEs and
> SOCKETs actually simplifies the APIs. Callers don't have to worry about
> support or needing to read first or whatever.
No, you explained just fine. You have two different semantics for
copy. On most buckets, it just creates a second bucket that is an exact
copy of the first. On pipe/socket buckets it reads data then copies the
bucket. While this does make the API more simple in this one case, it
means that there are two meanings for copy, and that is not a good thing.
> > Again, no. If the split bucket doesn't make sense for a given bucket
> > type, then the split doesn't make sense. The function will just have to
> > deal with a return code of APR_ENOTIMPL.
>
> If I have a PIPE, then it makes sense to say "split at 10 bytse". The result
> of the split is two buckets: the first 10 bytes of the PIPE, and the rest of
> the PIPE. How does this occur? Well, 10 bytes are read from the PIPE and
> form a HEAP bucket; the rest of the PIPE stays the same.
>
> There is a semantic, so lets implement it rather than making callers deal
> with the edge cases (buckets that can't split).
Again, now you have two meanings for split. One is just split a bucket,
the other is read then split. What happens with a pipe that has 5 bytes
if you ask for the split at 10? In the heap case this is an error, and
the bucket is left untouched. With a new menaing for split for
pipe/socket this is an error but the bucket is changed subtly. This is
invalid.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 08, 2000 at 08:52:05PM -0800, rbb@covalent.net wrote:
> On Wed, 8 Nov 2000, Greg Stein wrote:
> > On Wed, Nov 08, 2000 at 07:16:43PM -0800, rbb@covalent.net wrote:
> > > On Wed, 8 Nov 2000, Greg Stein wrote:
> > >...
> > > > PIPE and SOCKET need refcounting, but they cannot be copied. The data must
> > > > first be acquired, then it can be copied.
> > >
> > > We aren't copying data, we are copying the bucket, so the dup function
> > > should just create a second bucket that points to the same
> > > pipe/socket.
> >
> > That won't work. You can't have two buckets pointing to the same pipe/socket
> > because they cannot each read() the pipe/socket.
> >
> > If a bucket represents a pipe which is known to have 10 bytes in it before
> > the end-of-pipe (due to whatever protocol), and you copy the bucket, now you
> > have two buckets: one with 10 bytes in it, and one with none. And it all
> > just depends on which order you happen to read them from.
> >
> > To make it work, the pipe must read [into a heap bucket], then the copy is
> > made. Effectively, if you call copy(), then the pipe will read in a chunk,
> > create a heap bucket (adjust the brigade), then copy the heap bucket. Note
> > that you may still have a pipe bucket remaining (after the newly-created
> > heap bucket), and that bucket may need to be copied further.
>
> No. Now you have changed the definition of copy depending on the type of
> the bucket. That won't work, it makes the API too complex. If the
> pipe/socket buckets can't be copied until after they have been read, then
> the copy function returns APR_ENOTIMPL. We don't want the same function
> to mean different things in different situations.
Um. I didn't explain well, then. Defining copy() and split() for PIPEs and
SOCKETs actually simplifies the APIs. Callers don't have to worry about
support or needing to read first or whatever.
Let's say you have a brigade with just PIPE bucket:
brigade: PIPE
Now you make a copy:
brigade: HEAP -> PIPE
copy() result: HEAP
Now, let's say that we say "copy is not implemented on PIPE; read first."
Do a read:
brigade: HEAP -> PIPE
Do a copy:
brigade: HEAP -> PIPE
copy() result: HEAP
I'm suggesting that we define the semantics for copy() on a PIPE to be
exactly that of a read+copy. That will prevent all of the callers from doing
a copy(), seeing the failure, doing a read(), then doing a copy(). No sense
in complicating the life of all copy() callers.
IOW, by saying that PIPE *does* support a copy(), the API stays simple and
the callers don't have to deal with a bunch of variant cases.
Note that it could be said "but copy doesn't copy the whole bucket" is a bad
thing. Well, note that read() also mucks with the brigade. I believe there
is ample reason for saying "you get a copy of the whole bucket if that makes
sense, but some buckets may return a portion" similar to "you can read the
whole bucket if that makes sense, but some buckets may return a portion."
> > To really get this stuff up to top perf, we would want functional copy() and
> > split() operations on all bucket types.
>
> Again, no. If the split bucket doesn't make sense for a given bucket
> type, then the split doesn't make sense. The function will just have to
> deal with a return code of APR_ENOTIMPL.
If I have a PIPE, then it makes sense to say "split at 10 bytse". The result
of the split is two buckets: the first 10 bytes of the PIPE, and the rest of
the PIPE. How does this occur? Well, 10 bytes are read from the PIPE and
form a HEAP bucket; the rest of the PIPE stays the same.
There is a semantic, so lets implement it rather than making callers deal
with the edge cases (buckets that can't split).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by rb...@covalent.net.
On Wed, 8 Nov 2000, Greg Stein wrote:
> On Wed, Nov 08, 2000 at 07:16:43PM -0800, rbb@covalent.net wrote:
> > On Wed, 8 Nov 2000, Greg Stein wrote:
> >...
> > > PIPE and SOCKET need refcounting, but they cannot be copied. The data must
> > > first be acquired, then it can be copied.
> >
> > We aren't copying data, we are copying the bucket, so the dup function
> > should just create a second bucket that points to the same
> > pipe/socket.
>
> That won't work. You can't have two buckets pointing to the same pipe/socket
> because they cannot each read() the pipe/socket.
>
> If a bucket represents a pipe which is known to have 10 bytes in it before
> the end-of-pipe (due to whatever protocol), and you copy the bucket, now you
> have two buckets: one with 10 bytes in it, and one with none. And it all
> just depends on which order you happen to read them from.
>
> To make it work, the pipe must read [into a heap bucket], then the copy is
> made. Effectively, if you call copy(), then the pipe will read in a chunk,
> create a heap bucket (adjust the brigade), then copy the heap bucket. Note
> that you may still have a pipe bucket remaining (after the newly-created
> heap bucket), and that bucket may need to be copied further.
No. Now you have changed the definition of copy depending on the type of
the bucket. That won't work, it makes the API too complex. If the
pipe/socket buckets can't be copied until after they have been read, then
the copy function returns APR_ENOTIMPL. We don't want the same function
to mean different things in different situations.
> To really get this stuff up to top perf, we would want functional copy() and
> split() operations on all bucket types.
Again, no. If the split bucket doesn't make sense for a given bucket
type, then the split doesn't make sense. The function will just have to
deal with a return code of APR_ENOTIMPL.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 08, 2000 at 07:16:43PM -0800, rbb@covalent.net wrote:
> On Wed, 8 Nov 2000, Greg Stein wrote:
>...
> > PIPE and SOCKET need refcounting, but they cannot be copied. The data must
> > first be acquired, then it can be copied.
>
> We aren't copying data, we are copying the bucket, so the dup function
> should just create a second bucket that points to the same
> pipe/socket.
That won't work. You can't have two buckets pointing to the same pipe/socket
because they cannot each read() the pipe/socket.
If a bucket represents a pipe which is known to have 10 bytes in it before
the end-of-pipe (due to whatever protocol), and you copy the bucket, now you
have two buckets: one with 10 bytes in it, and one with none. And it all
just depends on which order you happen to read them from.
To make it work, the pipe must read [into a heap bucket], then the copy is
made. Effectively, if you call copy(), then the pipe will read in a chunk,
create a heap bucket (adjust the brigade), then copy the heap bucket. Note
that you may still have a pipe bucket remaining (after the newly-created
heap bucket), and that bucket may need to be copied further.
> The data should not be copied first. Now, having said that,
> it is incredibly unlikely that anybody will ever dup a pipe/socket bucket,
> because you really need either the data or length before you know if you
> want to dup or not.
I would tend to agree on this one.
Actually, I think what is going to happen:
split(brigade, range_start, &start_bucket)
split(brigade, range_end, &end_bucket)
while (start_bucket != end_bucket):
append(bsend, copy(start_bucket))
start_bucket = next(start_bucket)
Note that the split-brigade function would zoom through the brigade's
buckets looking for the split-position. If pipes/sockets are encountered,
then the contents will need to be read into memory. So yes... by the time
that we reach the copy(), there won't be any pipes/sockets in that area.
However, the pipe/socket *could* still exist at "end_bucket" or later.
To really get this stuff up to top perf, we would want functional copy() and
split() operations on all bucket types.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 08, 2000 at 07:23:23PM -0800, rbb@covalent.net wrote:
> > > Note that TRANSIENT and IMMORTAL do not need refcounting. The copied bucket
> > > can just refer to the same data.
> >
> > Transients most definately need to be ref-counted. If they aren't, then
> > you won't know the ref-count when setting it aside. If we ref-count, then
> > we can insert the same transient string into the brigade multiple times as
> > multiple instances of the same string, and we will only copy the data
> > once.
>
> That probably isn't clear at all. You are correct that currently
> transients don't need to be ref-counted. However, that limits transient
> data's usability, because a given transient string can only appear in a
> bucket brigade once optimally. If it occurs twice, then we have two
> buckets (one for each instance), and we copy it twice whenever we set it
> aside. If we ref-count the transient buckets, then we can insert a
> transient string multiple times in a single brigade, and we will only copy
> it once.
Right :-) ... I was gonna say... :-)
Hrm. This would get a bit tricky. Say you have two TRANSIENT buckets. A and
B. If you setaside A, then it becomes a HEAP bucket. Now... how does B refer
to the HEAP bucket so that it can optimize its setaside operation?
[ note: copy of a transient always produces a transient, no matter what ]
I think the answer is that a transient refers to a (refcounted) substructure
that looks something like:
{
const char *data;
apr_size_t length;
int on_heap;
}
When you first create the transient, on_heap is FALSE and data/length refer
to the transient memory.
copy() just refers to the same substructure.
setaside() of a transient will look in the substructure. If on_heap is FALSE,
then the data is copied to the heap and the "data" pointer is updated and
on_heap is set to TRUE. The bucket is then transformed into a HEAP bucket.
During a setaside(), if on_heap is TRUE, then we just build another HEAP
bucket around it. [ and there is some refcount magic to do ]
So... true: TRANSIENT should be refcounted for optimizing setaside().
IMMORTAL does not need to be refcounted.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by rb...@covalent.net.
> > Note that TRANSIENT and IMMORTAL do not need refcounting. The copied bucket
> > can just refer to the same data.
>
> Transients most definately need to be ref-counted. If they aren't, then
> you won't know the ref-count when setting it aside. If we ref-count, then
> we can insert the same transient string into the brigade multiple times as
> multiple instances of the same string, and we will only copy the data
> once.
That probably isn't clear at all. You are correct that currently
transients don't need to be ref-counted. However, that limits transient
data's usability, because a given transient string can only appear in a
bucket brigade once optimally. If it occurs twice, then we have two
buckets (one for each instance), and we copy it twice whenever we set it
aside. If we ref-count the transient buckets, then we can insert a
transient string multiple times in a single brigade, and we will only copy
it once.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by rb...@covalent.net.
On Wed, 8 Nov 2000, Greg Stein wrote:
> But you still need a copy() mechanism to create a second bucket that refers
> to the same data.
>
> Note that TRANSIENT and IMMORTAL do not need refcounting. The copied bucket
> can just refer to the same data.
Transients most definately need to be ref-counted. If they aren't, then
you won't know the ref-count when setting it aside. If we ref-count, then
we can insert the same transient string into the brigade multiple times as
multiple instances of the same string, and we will only copy the data
once.
> PIPE and SOCKET need refcounting, but they cannot be copied. The data must
> first be acquired, then it can be copied.
We aren't copying data, we are copying the bucket, so the dup function
should just create a second bucket that points to the same
pipe/socket. The data should not be copied first. Now, having said that,
it is incredibly unlikely that anybody will ever dup a pipe/socket bucket,
because you really need either the data or length before you know if you
want to dup or not.
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 09, 2000 at 02:24:51AM +0000, Tony Finch wrote:
> Greg Stein <gs...@lyra.org> wrote:
> >
> >If we must copy the buffer, then we've done something wrong :-)
> >
> >This certainly isn't your fault/design, but a limitation that we still have
> >in the brigade/bucket system. But! I have an idea... :-)
>
> The correct fix is to reference-count all buckets rather than having
> the half-hearted refcounting that we have at the moment.
But you still need a copy() mechanism to create a second bucket that refers
to the same data.
Note that TRANSIENT and IMMORTAL do not need refcounting. The copied bucket
can just refer to the same data.
PIPE and SOCKET need refcounting, but they cannot be copied. The data must
first be acquired, then it can be copied.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Tony Finch <do...@dotat.at>.
Greg Stein <gs...@lyra.org> wrote:
>
>If we must copy the buffer, then we've done something wrong :-)
>
>This certainly isn't your fault/design, but a limitation that we still have
>in the brigade/bucket system. But! I have an idea... :-)
The correct fix is to reference-count all buckets rather than having
the half-hearted refcounting that we have at the moment.
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: cvs commit: apache-2.0/src/main http_protocol.c
Posted by rb...@covalent.net.
> Have you happened to try the new code with the PDF plugin? I don't have the
> testing setup for PDF here. I was eyeballing the output, and using a
> fetching script to ensure that my Content-Length was computed properly.
I just tested the code that was committed last night. This code does not
work. I don't really know why, nor do I care. The patch that I have that
uses ap_content_length_filter to compute the content length does work with
the PDF plug-in, so I am perfectly happy not investigating the problems
with the current implementation. I believe the problems with the current
implementation are more in the interaction between modules than in the
code itself, because I didn't modify the byterange logic that much.
Anyway, I am committing my patch, but it should be scrutinized and tested
more thoroughly before we say that byteranges definately work.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 08, 2000 at 06:34:13AM -0800, rbb@covalent.net wrote:
> My CVS change:
>...
> > - curr_length = range_end - range_start + 1;
> > - loc = range = apr_pcalloc(r->pool, curr_length + 1);
> > + /* ### this is so bogus, but not dealing with it right now */
> > + range = loc = apr_pcalloc(r->pool, range_length + 1);
>
> Why is this bogus? We have no choice but to copy the buffer.
If we must copy the buffer, then we've done something wrong :-)
This certainly isn't your fault/design, but a limitation that we still have
in the brigade/bucket system. But! I have an idea... :-)
[ separate email for that ]
> > + /* ### we should split() buckets rather than read() them. this
> > + ### will allow us to avoid reading files or custom buckets
> > + ### into memory. for example: we REALLY don't want to cycle
> > + ### a 10gig file into memory just to send out 100 bytes from
> > + ### the end of the file.
> > + ###
> > + ### content generators that used to call ap_each_byterange()
> > + ### manually (thus, optimizing the output) can replace their
> > + ### behavior with a new bucket type that understands split().
> > + ### they can then defer reading actual content until a read()
> > + ### occurs, using the split() as an internal "seek".
> > + */
> > +
> > ap_bucket_read(e, &str, &n, AP_NONBLOCK_READ);
> > + /* ### using e->length doesn't account for pipes */
>
> Yes it does, because the read that is just above it converted the pipes
> into blocks of actual data. The thing is that our file reading is bogus,
> but I can't figure out how to fix it, so it has to stay as it is for now.
What should happen is that we split() the bucket. That will prevent a FILE
bucket from reading contents into memory. A PIPE or SOCKET bucket will need
to read up to the split point (into memory), but there may not be much we
can do about that. [ certainly, a first pass will do this; we could
potentially get a bit smarter and allow splitting pipes in some way ]
Thus, using a split() as we skip up to range_start, we can prevent reading
stuff into memory. This is important for FILE buckets, and possibly for
third part buckets (I have one planned for Subversion's content, and mod_dav
will make it easier for custom back-ends to use a bucket to represent a
resource).
> > while (range_start > (curr_offset + e->length)) {
> > curr_offset += e->length;
> > e = AP_BUCKET_NEXT(e);
> > +
> > if (e == AP_BRIGADE_SENTINEL(bb)) {
> > break;
> > }
> > +
> > + /* ### eventually we can avoid this */
> > + ap_bucket_read(e, &str, &n, AP_NONBLOCK_READ);
>
> How? We have just found the start of the data, so we need to have it in
> memory so that we can copy it.
Nope. This is still part of the code that skipping up to range_start.
You are correct, though, that we will need to read() at some point to copy
the bytes into the output buffer. But that is a different loop :-)
> > - apr_cpystrn(loc, str + (range_start - curr_offset),
> > - MIN_LENGTH(curr_length + 1, e->length));
> > - loc += MIN_LENGTH(curr_length + 1, e->length);
> > - curr_length -= MIN_LENGTH(curr_length + 1, e->length);
> > +
> > + /* ### we should use 'n', not e->length */
> > + segment_length = MIN_LENGTH(curr_length + 1, e->length);
>
> It doesn't matter if we use n or e->length, because the reads that you
> want to get rid of make them equivalent.
At this point in the code, you are correct. I just thought it would be
semantically cleaner to use what read() returns for the length. But I didn't
want to go and change that just yet :-)
> Can I assume that the only difference in the output is that we now add the
> content length correctly?
In terms of output: yes. But I also made a number of logical changes and
improved the memory footprint quite a bit. There were items getting
allocated on every entry to the filter (and then not used). I tossed a lot
of those and also simplified some other logic. etc.
> If so, I had an idea last night that I think is
> slightly cleaner than this. If we put back the original byterange filter,
> and add it to the filter stack before the content-length filter for all
> requests, it can check very quickly to determine if it should be
> active. If not, it just removes itself. If so, it does the byterange
> magic. Then, we can rely on the content-length filter to compute the C-L
> correctly everytime.
Using the C-L filter to compute the length is a definitely possibility.
However, the C-L works now, and I have some other changes to make to the
byterange filter to optimize it drastically. Please keep this idea in mind,
but it is working, so I'd like to defer this idea for a moment.
> I figured this out late last night, and I kind of like it because it
> re-uses code, and allows us to get aways with just having a single place
> to calculate the C-L, which is a VERY good thing IMHO.
Yes, it does make good sense, and I'm inclined to agree it would be good. It
would simplify ap_set_byterange(). But I want to take a closer look at the
C-L filter to see how it does it work first. I'm hoping it avoids a read().
But in any case, let's hold the thought for a bit (and we can since the
byterange output appears to be working)
> If there were more differences, (other than general cleanup), please let
> me know. I would love to know what the originl problem was.
Look at the loop in ap_set_byterange(). It needs to add in the length of the
boundaries and part-headers to the overall content-length. It wasn't doing
that at all. (if you look at the "realreq" logic in the original code, you
can see where it was doing that)
Have you happened to try the new code with the PDF plugin? I don't have the
testing setup for PDF here. I was eyeballing the output, and using a
fetching script to ensure that my Content-Length was computed properly.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/