Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9293029:
net: Introduce SeekableIOBuffer and clean up HttpStreamParser. (Closed)
|
Created:
8 years, 10 months ago by satorux1 Modified:
8 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionnet: Introduce SeekableIOBuffer and clean up HttpStreamParser.
Eliminate allocations for DrainableIOBuffer with SeekableIOBuffer.
BUG=72001
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120757
Patch Set 1 #Patch Set 2 : rebase and update comments #
Total comments: 19
Patch Set 3 : address comments #Patch Set 4 : update some comment #
Total comments: 14
Patch Set 5 : address comments #Patch Set 6 : fix comments #
Messages
Total messages: 15 (0 generated)
Here you go. A new IOBuffer class is introduced. After all, introducing a new class was a cleaner approach. Hope you'll like this version.
Patch Set 1 LGTM. Please wait for rvargas's review. rvargas: please review this CL. satorux1: I wonder if it's possible to merge SeekableIOBuffer and GrowableIOBuffer. If it's not possible, please improve the comments in io_buffer.h as I suggested below (look for my comment marked with "IMPORTANT"). Thanks for your effort on IOBuffers. https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h File net/base/io_buffer.h (right): https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:166: // int bytes_read = some_reader->Read(buf, buf.capacity()); The first argument should be buf.data(). In the general case, this call should look like: int bytes_read = some_reader->Read(buf.data() + buf.size(), buf.capacity() - buf.size()); https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:171: // int bytes_consumed = Read(buf, buf->BytesRemaining()); We should call Write() instead of Read() here. The first argument should be buf.data(). https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:184: explicit SeekableIOBuffer(int size); This parameter should be named 'capacity'. https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:196: int BytesConsumed() const; Do we need the BytesConsumed() method? https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:223: char* begin_; I suggest you name this member 'real_data_'. https://chromiumcodereview.appspot.com/9293029/diff/5002/net/base/io_buffer.h... net/base/io_buffer.h:230: class NET_EXPORT GrowableIOBuffer : public IOBuffer { IMPORTANT: DrainableIOBuffer, SeekableIOBuffer, and GrowableIOBuffer are all similar. It would be nice to improve the comments for DrainableIOBuffer and GrowableIOBuffer to explain when they should be used. DrainableIOBuffer is used for writing the data progressively. The data we need to write is given to us in an IOBuffer. GrowableIOBuffer is used for reading the data progressively, I think. https://chromiumcodereview.appspot.com/9293029/diff/5002/net/http/http_stream... File net/http/http_stream_parser.cc (right): https://chromiumcodereview.appspot.com/9293029/diff/5002/net/http/http_stream... net/http/http_stream_parser.cc:352: if (result > 0) { Nit: it is safe to call chunk_buf_->DidConsume(result) with a result of 0, so you can omit this result > 0 test. https://chromiumcodereview.appspot.com/9293029/diff/5002/net/http/http_stream... File net/http/http_stream_parser.h (right): https://chromiumcodereview.appspot.com/9293029/diff/5002/net/http/http_stream... net/http/http_stream_parser.h:23: class SeekableIOBuffer; Nit: list in alphabetical order.
wtc, thank you for the valuable feedback. Hope you'll like the updated comments. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h File net/base/io_buffer.h (right): http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode166 net/base/io_buffer.h:166: // int bytes_read = some_reader->Read(buf, buf.capacity()); On 2012/02/02 20:22:57, wtc wrote: > > The first argument should be buf.data(). > > In the general case, this call should look like: > int bytes_read = some_reader->Read(buf.data() + buf.size(), > buf.capacity() - buf.size()); > Done. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode171 net/base/io_buffer.h:171: // int bytes_consumed = Read(buf, buf->BytesRemaining()); On 2012/02/02 20:22:57, wtc wrote: > > We should call Write() instead of Read() here. Good catch. Fixed. > The first argument should be buf.data(). Here, we assume Write() takes IOBuffer*. If it takes const char*, we could just use a regular IOBuffer with buf->data() + offset. Added some comment about it. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode184 net/base/io_buffer.h:184: explicit SeekableIOBuffer(int size); On 2012/02/02 20:22:57, wtc wrote: > > This parameter should be named 'capacity'. Done. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode196 net/base/io_buffer.h:196: int BytesConsumed() const; On 2012/02/02 20:22:57, wtc wrote: > > Do we need the BytesConsumed() method? Removed. Right now, we don't need it. Removed it in the spirit of YAGNI. :) http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode223 net/base/io_buffer.h:223: char* begin_; On 2012/02/02 20:22:57, wtc wrote: > > I suggest you name this member 'real_data_'. Done. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode230 net/base/io_buffer.h:230: class NET_EXPORT GrowableIOBuffer : public IOBuffer { On 2012/02/02 20:22:57, wtc wrote: > > IMPORTANT: DrainableIOBuffer, SeekableIOBuffer, and > GrowableIOBuffer are all similar. > > It would be nice to improve the comments for DrainableIOBuffer > and GrowableIOBuffer to explain when they should be used. > > DrainableIOBuffer is used for writing the data progressively. > The data we need to write is given to us in an IOBuffer. > > GrowableIOBuffer is used for reading the data > progressively, I think. Agreed. Added some comments for DrainableIOBuffer and GroableIOBuffer. Updated the comment for SeekableIOBuffer as well. http://codereview.chromium.org/9293029/diff/5002/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/9293029/diff/5002/net/http/http_stream_parser.... net/http/http_stream_parser.cc:352: if (result > 0) { On 2012/02/02 20:22:57, wtc wrote: > > Nit: it is safe to call > chunk_buf_->DidConsume(result) > with a result of 0, so you can omit this result > 0 test. Good point. Done. http://codereview.chromium.org/9293029/diff/5002/net/http/http_stream_parser.h File net/http/http_stream_parser.h (right): http://codereview.chromium.org/9293029/diff/5002/net/http/http_stream_parser.... net/http/http_stream_parser.h:23: class SeekableIOBuffer; On 2012/02/02 20:22:57, wtc wrote: > > Nit: list in alphabetical order. Done.
http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h File net/base/io_buffer.h (right): http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode166 net/base/io_buffer.h:166: // int bytes_read = some_reader->Read(buf, buf.capacity()); On 2012/02/02 20:22:57, wtc wrote: > > The first argument should be buf.data(). > > In the general case, this call should look like: > int bytes_read = some_reader->Read(buf.data() + buf.size(), > buf.capacity() - buf.size()); > Please ignore this entire comment. I forgot that our Read() methods take IOBuffer* as the first argument. Sorry about the confusion. This does mean such a Read() method can only be called once. On the other hand, if we are using something like memcpy() to read data into 'buf', then we can do that multiple times, using the code I suggested above. http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode171 net/base/io_buffer.h:171: // int bytes_consumed = Read(buf, buf->BytesRemaining()); On 2012/02/02 22:02:27, satorux1 wrote: > On 2012/02/02 20:22:57, wtc wrote: > > The first argument should be buf.data(). > > Here, we assume Write() takes IOBuffer*. If it takes const char*, we could just > use a regular IOBuffer with buf->data() + offset. Added some comment about it. I was wrong again. Sorry!
http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h File net/base/io_buffer.h (right): http://codereview.chromium.org/9293029/diff/5002/net/base/io_buffer.h#newcode166 net/base/io_buffer.h:166: // int bytes_read = some_reader->Read(buf, buf.capacity()); On 2012/02/02 22:32:30, wtc wrote: > On 2012/02/02 20:22:57, wtc wrote: > > > > The first argument should be buf.data(). > > > > In the general case, this call should look like: > > int bytes_read = some_reader->Read(buf.data() + buf.size(), > > buf.capacity() - buf.size()); > > > > Please ignore this entire comment. I forgot that our Read() > methods take IOBuffer* as the first argument. Sorry about > the confusion. No worry, some Read() function takes const char* (ex. FileStream as of now). Changed it back from buf.data() -> buf, as it's more common. Removed "the general case" for simplicity. > This does mean such a Read() method can only be called once. It's possible to call such a Read() method multiple times, but it's a bit tricky: bytes_read = some_reader->Read(buf, buf.capacity()); buf->DidAppend(bytes_read); buf->DidConsume(bytes_read); // advance data() by bytes_read. bytes_read = some_reader->Read(buf, buf.capacity() -> buf.size()); buf->DidAppend(bytes_read); buf->DidConsume(bytes_read); // Before reading buf, move data() to the beginning... buf->SetOffset(0); > On the other hand, if we are using something like memcpy() > to read data into 'buf', then we can do that multiple times, > using the code I suggested above. You are right.
rvargas@, please take a look.
I'm having a hard time figuring out what's the best thing to do here. What we currently have is: IOBufferWithSize: Simple implementation class that provides minimal book keeping of what's going on with the buffer (doesn't track dynamic changes). GrowableIOBuffer: More elaborate implementation that keeps track of a current offset, and allows reallocation (malloc based). DrainableIOBuffer: Adapter class for a layer that receives a buffer and it has to provide a moving window to the lower layer. The last two classes are very similar, and they both can be used with reads or writes... the main difference is if the underlying buffer is provided by the upper layer (as an IOBuffer) or not. The new class (SeekableIOB) sits in the group of creating a buffer from scratch (rather than reusing something), but the main characteristic is not that it is seekable (both DrainableIOB and GrowableIOB are seekable), but that it is meant to support two uses at the same time: Read from one end and Write to another. So it supports the moving window with DidConsume, BytesRamaining, SetOffset and size, and a not moving window side with DidAppend and capacity (plus a global restart with Clear). So in that sense it is somewhat asymmetrical (one side can only grow, does not slide as the other side), and there is some potential confusion with which side gets "size" and which one gets "capacity", especially when just reading the code at the user side. Having another class means more "work" deciding what is the right class to use, but on the other hand adding more functionality to one of the existing classes also means "more work" trying to figure out how to use the class. In any case, this class is special purposed for what the user wants (and that's OK), but as it is it doesn't look general enough to live with the other "general purpose" versions. Options: 1. Add something specific for the HttpStreamParser 2. Upgrade this class to be more symmetrical 2.1 Maybe remove one or both of the other "complex" implementations (Drainable/Growable). http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.cc File net/base/io_buffer.cc (right): http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.cc#newco... net/base/io_buffer.cc:109: void SeekableIOBuffer::DidAppend(int bytes) { Add a dcheck for bytes > 0 (or >= ) http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h File net/base/io_buffer.h (right): http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:173: // storage, so you don't need to create a separate IOBuffer. DidAppend(), I would prefer not having references to bad patterns here (or at least not that much emphasis). This makes me feel like if allocating a separate buffer was OK in the first place, instead of forcing a square peg into a round hole, and the new class was just making things a little more convenient. I think it would be better to say that DrainableIOBuffer is meant for code that lives in the middle layer of a stack that uses IOBuffers (so it receives an IOBuffer and it has to provide an IOBuffer to the lower layer) (and you added a comment saying that), while this class is meant for code that lives at the top of a particular stack, so it does not receive an IOBuffer from anybody else, and it can be the ultimate owner of the buffer provided to a lower layer. This means I would remove the loop showing the bad pattern. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:238: // GUARANTEES: 0 <= BytesRemaining() <= size(). nit: we have always used comments that are in regular prose instead of a list of any kind (arguments, returns, preconditions etc). I suggest following that style here: if you think something is not obvious and should be spelled out, just mention it. (and that is to say that I would consider this line obvious, and just cognitive burden when I'm reading it). http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:246: // Marks that |bytes| have been appended. |bytes| is added to |size_|, but nit: "marks" is not that clear. Go either with "called after blah", or what it actually does: "Modifies the number of bytes..." http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:266: char* real_data_; buffer_start_ ? data_start_ ? http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:286: // some_stream->set_offset(buf->offset() + bytes_read); This should be buf->set_offset( http://codereview.chromium.org/9293029/diff/11003/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/9293029/diff/11003/net/http/http_stream_parser... net/http/http_stream_parser.cc:70: GrowableIOBuffer* read_buffer, This is of course not related to your change, but something to keep in mind is that the IOBuffer subclasses are not meant to be passed around directly... they are just convenience classes for creation of buffers, but the base class is the only interface that is supposed to be used when going from one class to another.
Ricardo, thank you for the feedback. I was also having hard time how to co-exist with similar classes. For now, I think it makes most sense to move this to http_stream_parser.cc. We can revisit this when the new class is needed elsewhere. I kept comments in io_buffer.h, assuming these are useful. I'm OK with removing them if you didn't find them useful. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.cc File net/base/io_buffer.cc (right): http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.cc#newco... net/base/io_buffer.cc:109: void SeekableIOBuffer::DidAppend(int bytes) { On 2012/02/03 23:13:34, rvargas wrote: > Add a dcheck for bytes > 0 (or >= ) Done. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h File net/base/io_buffer.h (right): http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:173: // storage, so you don't need to create a separate IOBuffer. DidAppend(), On 2012/02/03 23:13:34, rvargas wrote: > I would prefer not having references to bad patterns here (or at least not that > much emphasis). This makes me feel like if allocating a separate buffer was OK > in the first place, instead of forcing a square peg into a round hole, and the > new class was just making things a little more convenient. > > I think it would be better to say that DrainableIOBuffer is meant for code that > lives in the middle layer of a stack that uses IOBuffers (so it receives an > IOBuffer and it has to provide an IOBuffer to the lower layer) (and you added a > comment saying that), while this class is meant for code that lives at the top > of a particular stack, so it does not receive an IOBuffer from anybody else, and > it can be the ultimate owner of the buffer provided to a lower layer. > > This means I would remove the loop showing the bad pattern. Good point. Reworked and simplified the comment per your suggestion. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:238: // GUARANTEES: 0 <= BytesRemaining() <= size(). On 2012/02/03 23:13:34, rvargas wrote: > nit: we have always used comments that are in regular prose instead of a list of > any kind (arguments, returns, preconditions etc). I suggest following that style > here: if you think something is not obvious and should be spelled out, just > mention it. > > (and that is to say that I would consider this line obvious, and just cognitive > burden when I'm reading it). Removed. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:246: // Marks that |bytes| have been appended. |bytes| is added to |size_|, but On 2012/02/03 23:13:34, rvargas wrote: > nit: "marks" is not that clear. Go either with "called after blah", or what it > actually does: "Modifies the number of bytes..." Done. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:266: char* real_data_; On 2012/02/03 23:13:34, rvargas wrote: > buffer_start_ ? data_start_ ? it used to be begin_, but changed to real_data_ per wtc's suggestion. real_data_ makes sense as it always points to the beginning of the allocated memory chunk. http://codereview.chromium.org/9293029/diff/11003/net/base/io_buffer.h#newcod... net/base/io_buffer.h:286: // some_stream->set_offset(buf->offset() + bytes_read); On 2012/02/03 23:13:34, rvargas wrote: > This should be buf->set_offset( Good catch. Done. http://codereview.chromium.org/9293029/diff/11003/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/9293029/diff/11003/net/http/http_stream_parser... net/http/http_stream_parser.cc:70: GrowableIOBuffer* read_buffer, On 2012/02/03 23:13:34, rvargas wrote: > This is of course not related to your change, but something to keep in mind is > that the IOBuffer subclasses are not meant to be passed around directly... they > are just convenience classes for creation of buffers, but the base class is the > only interface that is supposed to be used when going from one class to another. Thank you for sharing the thought. I'll keep in mind that IOBuffer should be used for interface.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/9293029/9004
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is mac_rel, revision is 120596, job name was 9293029-9004 (previous was lost) (retry) (previous was lost) (previous was lost).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/9293029/9004
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is mac_rel, revision is 120695, job name was 9293029-9004 (previous was lost) (previous was lost) (previous was lost) (retry).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/9293029/9004
Change committed as 120757 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
