Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169723004:
RawChannel refactoring (Closed)
|
Created:
6 years, 10 months ago by yzshen1 Modified:
6 years, 9 months ago Reviewers:
viettrungluu CC:
chromium-reviews, Aaron Boodman, darin (slow to review), viettrungluu+watch_chromium.org, ben+mojo_chromium.org, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRestructure RawChannelPosix and RawChannel, so that the platform-independent logic can be shared between POSIX and Windows.
BUG=None
TEST=mojo_system_unittests
R=viettrungluu@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253865
Patch Set 1 #
Total comments: 1
Patch Set 2 : raw_channel.cc #Patch Set 3 : raw_channel_posix #Patch Set 4 : some cleanup #Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : split to have Schedule[Read|WriteNoLock] #Patch Set 8 : introduce ReadBuffer / WriteBuffer #
Total comments: 34
Patch Set 9 : changes according to Trung's suggestions #Patch Set 10 : support multi-segment write #
Total comments: 11
Patch Set 11 : #Patch Set 12 : #
Total comments: 4
Patch Set 13 : #
Messages
Total messages: 16 (0 generated)
Hi, Trung. Would you please take a look at the new interface draft? I wish you could give me some early feedback before I make more changes. Thanks! https://codereview.chromium.org/169723004/diff/1/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/1/mojo/system/raw_channel.h#ne... mojo/system/raw_channel.h:109: class IOBufferPreserver { I created this opaque object so that subclasses can decided how long the buffer of the current pending read/write should live after |Shutdown()|. With its help, I don't need to |WaitForIOCompletion()| or make an extra copy of the buffer. What do you think?
Hi, Trung. I think this CL is ready for serious review now. Please take a look. Thanks!
I like things generally so far -- just some fairly minor organizational nits. (I still have to read the rest more carefully....) https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:110: class IOBufferPreserver { I think I'd move this to the Windows-specific class, and just do what's necessary to allow OnShutdownNoLock() to take ownership of the buffers. (You can then document the reason for |OnShutdownNoLock()'s existence, as well as any other methods.) https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:136: virtual IOResult Read(bool schedule_for_later, I think it may be better to split this apart into Read()/ScheduleRead(), rather than having a boolean. https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:149: virtual IOResult WriteNoLock(bool schedule_for_later, (ditto)
Thanks, Trung! https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:110: class IOBufferPreserver { On 2014/02/25 01:30:06, viettrungluu wrote: > I think I'd move this to the Windows-specific class, and just do what's > necessary to allow OnShutdownNoLock() to take ownership of the buffers. (You can > then document the reason for |OnShutdownNoLock()'s existence, as well as any > other methods.) (Maybe I haven't fully understood your idea.) IMO, it seems reasonable to have this class here: Currently, I don't provide the subclasses with access to the read/write buffer members (i.e., |read_buffer_|, |write_message_queue_|, etc.) Because they don't need to care about how buffer management is done. (Accordingly, IOBufferPreserver doesn't provide any accessor. It is an opaque object to the subclasses.) I personally like this encapsulation. If we move IOBufferPreserver into the Windows-specific class, we will have to expose buffer members to the subclasses. Besides, the subclasses need to understand the buffer management details in order to preserve buffers by themselves. For example, they need to know that the pending write buffer is within the first element of |write_message_queue_|; after |Shutdown()|, |write_message_queue_| won't be used by the base anymore, so it is safe to mutate it; etc. For subclasses (POSIX) that don't need to preserve the buffer beyond OnShutdownNoLock(), there isn't much burden for them -- they just ignore |buffer_preserver|, which will get destroyed when OnShutdownNoLock() exits. What do you think? https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:136: virtual IOResult Read(bool schedule_for_later, On 2014/02/25 01:30:06, viettrungluu wrote: > I think it may be better to split this apart into Read()/ScheduleRead(), rather > than having a boolean. After the split, Read() can still return IO_PENDING. Does that sound okay to you? (I don't have a strong opinion, just to double check.)
https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:110: class IOBufferPreserver { On 2014/02/25 05:16:34, yzshen1 wrote: > On 2014/02/25 01:30:06, viettrungluu wrote: > > I think I'd move this to the Windows-specific class, and just do what's > > necessary to allow OnShutdownNoLock() to take ownership of the buffers. (You > can > > then document the reason for |OnShutdownNoLock()'s existence, as well as any > > other methods.) > > (Maybe I haven't fully understood your idea.) IMO, it seems reasonable to have > this class here: > > Currently, I don't provide the subclasses with access to the read/write buffer > members (i.e., |read_buffer_|, |write_message_queue_|, etc.) Because they don't > need to care about how buffer management is done. (Accordingly, > IOBufferPreserver doesn't provide any accessor. It is an opaque object to the > subclasses.) I personally like this encapsulation. > > If we move IOBufferPreserver into the Windows-specific class, we will have to > expose buffer members to the subclasses. Besides, the subclasses need to > understand the buffer management details in order to preserve buffers by > themselves. For example, they need to know that the pending write buffer is > within the first element of |write_message_queue_|; after |Shutdown()|, > |write_message_queue_| won't be used by the base anymore, so it is safe to > mutate it; etc. > > For subclasses (POSIX) that don't need to preserve the buffer beyond > OnShutdownNoLock(), there isn't much burden for them -- they just ignore > |buffer_preserver|, which will get destroyed when OnShutdownNoLock() exits. > > What do you think? I like the encapsulation too, but I have to weight it against having a very specialized class hanging around for a single platform (namely Windows; or really just XP). I'd prefer that you expose the guts to subclasses (if you're feeling paranoid, you could even add accessors inside an ifdef, with an explanatory note). The reason is that it becomes much more obvious that a) this code is really only used on Windows (and can only be tested/debugged on Windows), b) this code isn't really a part of the core functionality (and perhaps can be removed at some point). Admittedly, either way, there's a cost. https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:136: virtual IOResult Read(bool schedule_for_later, On 2014/02/25 05:16:34, yzshen1 wrote: > On 2014/02/25 01:30:06, viettrungluu wrote: > > I think it may be better to split this apart into Read()/ScheduleRead(), > rather > > than having a boolean. > > After the split, Read() can still return IO_PENDING. Does that sound okay to > you? > (I don't have a strong opinion, just to double check.) Yes, that's fine. (The main reason for having "ScheduleRead(...)" instead of "Read(true, ...)" is that the former is clearer/more readable at the call site. (An alternative might be to use an enum and have "Read(SCHEDULE_ONLY, ...)", but I think that's probably overkill.)
On 2014/02/25 16:37:07, viettrungluu wrote: > https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel.h > File mojo/system/raw_channel.h (right): > > https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... > mojo/system/raw_channel.h:110: class IOBufferPreserver { > On 2014/02/25 05:16:34, yzshen1 wrote: > > On 2014/02/25 01:30:06, viettrungluu wrote: > > > I think I'd move this to the Windows-specific class, and just do what's > > > necessary to allow OnShutdownNoLock() to take ownership of the buffers. (You > > can > > > then document the reason for |OnShutdownNoLock()'s existence, as well as any > > > other methods.) > > > > (Maybe I haven't fully understood your idea.) IMO, it seems reasonable to have > > this class here: > > > > Currently, I don't provide the subclasses with access to the read/write buffer > > members (i.e., |read_buffer_|, |write_message_queue_|, etc.) Because they > don't > > need to care about how buffer management is done. (Accordingly, > > IOBufferPreserver doesn't provide any accessor. It is an opaque object to the > > subclasses.) I personally like this encapsulation. > > > > If we move IOBufferPreserver into the Windows-specific class, we will have to > > expose buffer members to the subclasses. Besides, the subclasses need to > > understand the buffer management details in order to preserve buffers by > > themselves. For example, they need to know that the pending write buffer is > > within the first element of |write_message_queue_|; after |Shutdown()|, > > |write_message_queue_| won't be used by the base anymore, so it is safe to > > mutate it; etc. > > > > For subclasses (POSIX) that don't need to preserve the buffer beyond > > OnShutdownNoLock(), there isn't much burden for them -- they just ignore > > |buffer_preserver|, which will get destroyed when OnShutdownNoLock() exits. > > > > What do you think? > > I like the encapsulation too, but I have to weight it against having a very > specialized class hanging around for a single platform (namely Windows; or > really just XP). > > I'd prefer that you expose the guts to subclasses (if you're feeling paranoid, > you could even add accessors inside an ifdef, with an explanatory note). The > reason is that it becomes much more obvious that a) this code is really only > used on Windows (and can only be tested/debugged on Windows), b) this code isn't > really a part of the core functionality (and perhaps can be removed at some > point). > > Admittedly, either way, there's a cost. An alternate is to come up with an abstraction which is more general/less specialized. E.g., there might be some use to coming up with an abstraction that encapsulates the notion of "current read/write buffer", actually pass those around as arguments to Read()/ScheduleRead()/WriteNoLock()/etc. and just give the subclass a chance to hang on to it/delay its destruction. E.g.: "CurrentReadBuffer" would actually have an owning pointer to an entire vector (plus offset/size values). "CurrentWriteBuffer" would own the MessageInTransit that's currently being written (plus offset/remaining size values). This might be helpful for future changes.
I apparently can't get all my thoughts down in a single, or even two, replies.
I should point out that what I said in my second reply came about after reading
raw_channel_posix.cc (and seeing the pending... variables).
I guess maybe you could have the "Current{Read,Write}Buffer" (substitute
"Pending" for "Current" if you want) owned by the superclass and exposed via
getters. (And then just pass ownership via a scoped_ptr to OnShutdownNoLock().)
(Just some more ideas....)
Trung, please take another look. Thanks! https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:110: class IOBufferPreserver { Done. https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:136: virtual IOResult Read(bool schedule_for_later, On 2014/02/25 16:37:07, viettrungluu wrote: > On 2014/02/25 05:16:34, yzshen1 wrote: > > On 2014/02/25 01:30:06, viettrungluu wrote: > > > I think it may be better to split this apart into Read()/ScheduleRead(), > > rather > > > than having a boolean. > > > > After the split, Read() can still return IO_PENDING. Does that sound okay to > > you? > > (I don't have a strong opinion, just to double check.) > > Yes, that's fine. (The main reason for having "ScheduleRead(...)" instead of > "Read(true, ...)" is that the former is clearer/more readable at the call site. > (An alternative might be to use an enum and have "Read(SCHEDULE_ONLY, ...)", but > I think that's probably overkill.) Done. https://codereview.chromium.org/169723004/diff/210001/mojo/system/raw_channel... mojo/system/raw_channel.h:149: virtual IOResult WriteNoLock(bool schedule_for_later, On 2014/02/25 01:30:06, viettrungluu wrote: > (ditto) Done.
https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel.cc File mojo/system/raw_channel.cc (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:21: namespace { Note that this anonymous namespace is redundant. (In C++, const globals have internal linkage by default.) <shrug> https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:98: return result == IO_PENDING; nit: I think you may as well write this as |return ScheduleRead() == IO_PENDING;| (unless you want to rename "result" to "io_result") https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:114: // Reminder: This must be thread-safe, and takes ownership of |message|. You can get rid of the ", and takes ownership ..." part. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:158: void RawChannel::OnReadCompleted(bool result, size_t bytes_read) { The implementation of this function could use a comment outlining its general structure. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:248: if (io_result == IO_PENDING) do { ... } while (io_result != IO_PENDING); https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:306: STLDeleteElements(&write_buffer_->message_queue_); Can't you just do |write_buffer_.reset()| here? https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:308: return false; More importantly, if it's the subclass that calls |OnWriteCompletedNoLock()|, don't you fail to call OnFatalError()? https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:152: WriteBuffer* write_buffer(); -> write_buffer_no_lock https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:154: // Reads into |read_buffer()|, the area indicated by |GetPosition()| and nit: comma-splice: , the -> . The https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:155: // |GetBytesToRead()| will stay valid until read completion. (But please also nit: "completion. (But [...] .)" -> "completion (but [...])." https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:159: // If the method returns IO_PENDING, |OnReadCompleted()| will be called on the nit: -> If IO_PENDING is returned, |OnReadCompleted()| must be called later on the I/O thread .... You should make it clear in the comments what contract this class offers (e.g., that |read_buffer()| stays valid until completion), versus what contract the implementing subclass has to satisfy (e.g., that it must call |OnReadCOmpleted()|). https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:160: // I/O thread to report the result, unless |Shutdown()| happens. happens -> is called https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:168: // Writes contents in |write_buffer()|, the area indicated by |GetPosition()| (similarly) I suggest that you merge the comments for reading/writing to the extent possible. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:217: base::MessageLoopForIO* const message_loop_for_io_; nit: I think I'd prefer if you grouped the const pointers at the top. The pointers themselves are de facto-thread-safe because they won't change. (Using the pointers is, as always, subject to the thread-safety of the thing they point to.) https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:198: scoped_ptr<ReadBuffer> /* read_buffer */, nit: In this directory, I've mostly left out spaces inside the /* ... */ (so /*read_buffer*/). (The absolute correctness of what I've done is unclear, but consistency is probably more important here.) https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:220: return; You should probably DCHECK that read_waiter_ is null (otherwise we could end up spinning). ... BUT: Of course you're assuming that |OnReadCompleted()| always schedules another read on (read) success/failure, which its contract doesn't appear to guarantee. (E.g., what if there's some condition that causes it to not call |ScheduleRead()|?) Sort of the minimum thing you can do here is to |DCHECK()| that |pending_read_| is true (or that read-watching has been cancelled) after |OnReadComplete()|. Quite possibly, you should get rid of |pending_read_| and make it so this class can be aware of/notified when the superclass gives up on reading. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:239: return; " (though the situation for pending_write_ isn't quite so bad, since write-watching is one-shot, not persistent.
Thanks Trung. Please take another look. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel.cc File mojo/system/raw_channel.cc (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:21: namespace { On 2014/02/26 23:03:39, viettrungluu wrote: > Note that this anonymous namespace is redundant. (In C++, const globals have > internal linkage by default.) <shrug> Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:98: return result == IO_PENDING; On 2014/02/26 23:03:39, viettrungluu wrote: > nit: I think you may as well write this as |return ScheduleRead() == > IO_PENDING;| (unless you want to rename "result" to "io_result") Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:114: // Reminder: This must be thread-safe, and takes ownership of |message|. On 2014/02/26 23:03:39, viettrungluu wrote: > You can get rid of the ", and takes ownership ..." part. Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:158: void RawChannel::OnReadCompleted(bool result, size_t bytes_read) { On 2014/02/26 23:03:39, viettrungluu wrote: > The implementation of this function could use a comment outlining its general > structure. Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:248: if (io_result == IO_PENDING) On 2014/02/26 23:03:39, viettrungluu wrote: > do { ... } while (io_result != IO_PENDING); Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:306: STLDeleteElements(&write_buffer_->message_queue_); On 2014/02/26 23:03:39, viettrungluu wrote: > Can't you just do |write_buffer_.reset()| here? I was thinking maybe it is good to always give the subclasses the same WriteBuffer object (via write_buffer() or the parameter of OnShutdownNoLock()). What do you think? https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.cc:308: return false; On 2014/02/26 23:03:39, viettrungluu wrote: > More importantly, if it's the subclass that calls |OnWriteCompletedNoLock()|, > don't you fail to call OnFatalError()? OnWriteCompletedNoLock() is a private helper method, which won't be called by subclasses. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:152: WriteBuffer* write_buffer(); On 2014/02/26 23:03:39, viettrungluu wrote: > -> write_buffer_no_lock Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:154: // Reads into |read_buffer()|, the area indicated by |GetPosition()| and On 2014/02/26 23:03:39, viettrungluu wrote: > nit: comma-splice: , the -> . The Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:155: // |GetBytesToRead()| will stay valid until read completion. (But please also On 2014/02/26 23:03:39, viettrungluu wrote: > nit: "completion. (But [...] .)" -> "completion (but [...])." Done. Thanks! :) https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:159: // If the method returns IO_PENDING, |OnReadCompleted()| will be called on the On 2014/02/26 23:03:39, viettrungluu wrote: > nit: -> If IO_PENDING is returned, |OnReadCompleted()| must be called later on > the I/O thread .... > > You should make it clear in the comments what contract this class offers (e.g., > that |read_buffer()| stays valid until completion), versus what contract the > implementing subclass has to satisfy (e.g., that it must call > |OnReadCOmpleted()|). Good point. Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:160: // I/O thread to report the result, unless |Shutdown()| happens. On 2014/02/26 23:03:39, viettrungluu wrote: > happens -> is called Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:168: // Writes contents in |write_buffer()|, the area indicated by |GetPosition()| On 2014/02/26 23:03:39, viettrungluu wrote: > (similarly) I made similar changes. > I suggest that you merge the comments for reading/writing to the extent > possible. I find that it is not very clear after merging. I ended up with things like |On{Read,Write}Completed()|, |bytes_{read,written}|. And I need to comment that this block of comments applies to all four methods, the other applies to .*Read(), etc. If you don't mind, I will leave them as separate blocks. (Sigh, English is hard. :)) https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel.h:217: base::MessageLoopForIO* const message_loop_for_io_; On 2014/02/26 23:03:39, viettrungluu wrote: > nit: I think I'd prefer if you grouped the const pointers at the top. The > pointers themselves are de facto-thread-safe because they won't change. (Using > the pointers is, as always, subject to the thread-safety of the thing they point > to.) Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:198: scoped_ptr<ReadBuffer> /* read_buffer */, On 2014/02/26 23:03:39, viettrungluu wrote: > nit: In this directory, I've mostly left out spaces inside the /* ... */ (so > /*read_buffer*/). > > (The absolute correctness of what I've done is unclear, but consistency is > probably more important here.) Done. https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:220: return; On 2014/02/26 23:03:39, viettrungluu wrote: > You should probably DCHECK that read_waiter_ is null (otherwise we could end up > spinning). > > ... > > BUT: Of course you're assuming that |OnReadCompleted()| always schedules another > read on (read) success/failure, which its contract doesn't appear to guarantee. > > (E.g., what if there's some condition that causes it to not call > |ScheduleRead()|?) > > Sort of the minimum thing you can do here is to |DCHECK()| that |pending_read_| > is true (or that read-watching has been cancelled) after |OnReadComplete()|. > > Quite possibly, you should get rid of |pending_read_| and make it so this class > can be aware of/notified when the superclass gives up on reading. Thanks! :) https://codereview.chromium.org/169723004/diff/300001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:239: return; On 2014/02/26 23:03:39, viettrungluu wrote: > " > > (though the situation for pending_write_ isn't quite so bad, since > write-watching is one-shot, not persistent. Right. I made it a DCHECK().
https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel.h:124: typedef std::vector<std::pair<const char*, size_t> > BufferVector; I'd generally prefer a struct Buffer { const void* buffer; size_t size; }; (or whatever), than an std::pair (because "first" and "second" aren't descriptive). https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:112: } else if (errno != EAGAIN && errno != EWOULDBLOCK) { nit: no |else| needed here https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:150: iovec* iov = new iovec[buffers.size()]; I think it's overkill to do a heap allocation for this. I'd just allocate N (where N is some reasonable size, say 10), and then iterate for i < std::min(buffers.size(), N). To avoid copying vectors around and such, it might be reasonable to change WriteBuffer's API to instead be: const WriteBuffer::Buffer* GetBuffer(size_t index); (returning null if index > number of segments). To make |GetBuffer()| work, you'd just store an array/vector of |WriteBuffer::Buffer|s (currently with exactly two entries) in the object. https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:164: } else if (errno != EAGAIN && errno != EWOULDBLOCK) { no else
Thanks Trung. Please take another look! https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel.h:124: typedef std::vector<std::pair<const char*, size_t> > BufferVector; On 2014/02/27 05:22:15, viettrungluu wrote: > I'd generally prefer a > > struct Buffer { > const void* buffer; > size_t size; > }; > > (or whatever), than an std::pair (because "first" and "second" aren't > descriptive). Done. https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:112: } else if (errno != EAGAIN && errno != EWOULDBLOCK) { On 2014/02/27 05:22:15, viettrungluu wrote: > nit: no |else| needed here Done. https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:150: iovec* iov = new iovec[buffers.size()]; On 2014/02/27 05:22:15, viettrungluu wrote: > I think it's overkill to do a heap allocation for this. I'd just allocate N > (where N is some reasonable size, say 10), and then iterate for i < > std::min(buffers.size(), N). Done. > > To avoid copying vectors around and such, it might be reasonable to change > WriteBuffer's API to instead be: > > const WriteBuffer::Buffer* GetBuffer(size_t index); > > (returning null if index > number of segments). > > To make |GetBuffer()| work, you'd just store an array/vector of > |WriteBuffer::Buffer|s (currently with exactly two entries) in the object. Storing a vector of |WriteBuffer::Buffer| requires an explicit action to "update" it and maintain its consistency with other data members. For now, however, WriteBuffer lets RawChannel directly access its members so the burden of maintaining consistency will be on RawChannel. Besides, IMO currently it is not very expensive to compute the vector for every call of GetBuffers() (at most two elements). If you don't mind, I would prefer to do this optimization later, when we want to encapsulate more logic in WriteBuffer, or when GetBuffers() becomes more expensive. What do you think? https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:164: } else if (errno != EAGAIN && errno != EWOULDBLOCK) { On 2014/02/27 05:22:15, viettrungluu wrote: > no else Done.
https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:150: iovec* iov = new iovec[buffers.size()]; On 2014/02/27 06:56:18, yzshen1 wrote: > On 2014/02/27 05:22:15, viettrungluu wrote: > > I think it's overkill to do a heap allocation for this. I'd just allocate N > > (where N is some reasonable size, say 10), and then iterate for i < > > std::min(buffers.size(), N). > > Done. > > > > > To avoid copying vectors around and such, it might be reasonable to change > > WriteBuffer's API to instead be: > > > > const WriteBuffer::Buffer* GetBuffer(size_t index); > > > > (returning null if index > number of segments). > > > > To make |GetBuffer()| work, you'd just store an array/vector of > > |WriteBuffer::Buffer|s (currently with exactly two entries) in the object. > Storing a vector of |WriteBuffer::Buffer| requires an explicit action to > "update" it and maintain its consistency with other data members. For now, > however, WriteBuffer lets RawChannel directly access its members so the burden > of maintaining consistency will be on RawChannel. Besides, IMO currently it is > not very expensive to compute the vector for every call of GetBuffers() (at most > two elements). If you don't mind, I would prefer to do this optimization later, > when we want to encapsulate more logic in WriteBuffer, or when GetBuffers() > becomes more expensive. > What do you think? Isn't consistency guaranteed by the fact that access is all done under write_lock()? (Consistency is already implied, since the WriteBuffer isn't allowed to be changed until the write completes.) https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel... mojo/system/raw_channel.h:101: struct ReadBuffer { nit: class https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel... mojo/system/raw_channel.h:121: struct WriteBuffer { "
Thanks Trung. https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:150: iovec* iov = new iovec[buffers.size()]; > Isn't consistency guaranteed by the fact that access is all done under > write_lock()? > > (Consistency is already implied, since the WriteBuffer isn't allowed to be > changed until the write completes.) I mean: write_buffer_->message_queue_->pop_front(); write_buffer_->offset_ = 0; [write_buffer_->]UpdateBuffersVector(); // I will need this explicit action to update the stored std::vector<Buffer>, so that it is consistent with message_queue_/offset_. (Because they contain duplicated information.) The current usage of GetBuffers() (called only once per WriteNoLock()) and the cost of generating the vector (at most two elements), makes me think that it doesn't worth the effort. But if later we want to encapsulate more logic in WriteBuffer so that it maintains consistency across its own members; or GetBuffers() becomes more expensive, I agree that stored std::vector<Buffer> is a way of optimization. https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel.h File mojo/system/raw_channel.h (right): https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel... mojo/system/raw_channel.h:101: struct ReadBuffer { On 2014/02/27 07:36:35, viettrungluu wrote: > nit: class Done. https://codereview.chromium.org/169723004/diff/370001/mojo/system/raw_channel... mojo/system/raw_channel.h:121: struct WriteBuffer { On 2014/02/27 07:36:35, viettrungluu wrote: > " Done.
https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... File mojo/system/raw_channel_posix.cc (right): https://codereview.chromium.org/169723004/diff/340001/mojo/system/raw_channel... mojo/system/raw_channel_posix.cc:150: iovec* iov = new iovec[buffers.size()]; On 2014/02/27 16:19:33, yzshen1 wrote: > > Isn't consistency guaranteed by the fact that access is all done under > > write_lock()? > > > > (Consistency is already implied, since the WriteBuffer isn't allowed to be > > changed until the write completes.) > > I mean: > > write_buffer_->message_queue_->pop_front(); > write_buffer_->offset_ = 0; > [write_buffer_->]UpdateBuffersVector(); // I will need this explicit action to > update the stored std::vector<Buffer>, so that it is consistent with > message_queue_/offset_. (Because they contain duplicated information.) > > The current usage of GetBuffers() (called only once per WriteNoLock()) and the > cost of generating the vector (at most two elements), makes me think that it > doesn't worth the effort. But if later we want to encapsulate more logic in > WriteBuffer so that it maintains consistency across its own members; or > GetBuffers() becomes more expensive, I agree that stored std::vector<Buffer> is > a way of optimization. Okay, LGTM then.
Message was sent while issue was closed.
Committed patchset #13 manually as r253865 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
