Mojo: Abstract out the endpoints of MessagePipes.

Mojo: Abstract out the endpoints of MessagePipes.

This will allow us to plug in different kinds of endpoints, in particular,
non-local endpoints (so that MessagePipes can be cross-process, with, e.g., a
socket as transport).

R=darin

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228815

[viettrungluu, 2013-10-11 19:42:58]

Better send this to you while you're here. :)

I kind of regret the additional level of indirection/abstraction, but strangely
this actually increases performance (~3.3 million iterations/second to 3.5+).

Also, it'll hopefully allow me to implement MessagePipes over sockets cleanly
(and offers the possibility of additional implementations, e.g., over shared
memory).

[viettrungluu, 2013-10-11 23:36:42]

Don't review this yet. I don't think it's designed correctly, and I need to
think about it some more.

[viettrungluu, 2013-10-14 22:54:09]

On second/third/fourth thought, I think I'll go with this as-is. I can't think
of anything substantially better, and I'll adapt this approach as necessary.

[darin (slow to review), 2013-10-15 06:50:36]

This seems like a reasonable refactoring to me. I just have a question about how
you would use this in the non-local case.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/local_message...
File mojo/system/local_message_pipe_endpoint.h (right):

https://codereview.chromium.org/27060003/diff/10001/mojo/system/local_message...
mojo/system/local_message_pipe_endpoint.h:33: // Implementations need only
implement/override these if they are "connected"
nit: This comment seems out of place. Looks to have been copied from
MessagePipeEndpoint. Ditto for the comment above OnPeerClose.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe.cc
File mojo/system/message_pipe.cc (right):

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe....
mojo/system/message_pipe.cc:26: endpoints_[0].reset(new
LocalMessagePipeEndpoint());
I see. This is the connected pair case.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
File mojo/system/message_pipe_endpoint.h (right):

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
mojo/system/message_pipe_endpoint.h:20: // corresponding to the endpoint, and
|MessagePipeEndpoint| also implements the
nit: Is the comment "|MessagePipeEndpoint| also implements the requisite
functionality, e.g., to read messages and to wait" accurate? It doesn't seem to
provide interesting implementations of those functions. They are on
LocalMessagePipeEndpoint.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
mojo/system/message_pipe_endpoint.h:35: // local |MessagePipeDispatcher| (the
default implementations should never be
I'm having trouble following this design. Are you saying that when connected to
a non-local MessagePipeDispatcher that these methods may not be implemented /
overridden? What does that mean?

[viettrungluu, 2013-10-15 17:48:58]

The design of MessagePipeEndpoint is a little strange, because it really
represents two things (that are rather intertwined).

1) It represents a sink for messages. In the case of a local endpoint, that's
just a local queue. In the case of a remote endpoint, that'll be a frontend to
something that writes messages to a pipe (e.g., a RawChannel), which you could
consider to be an outgoing queue.

2) In the case of a local endpoint (i.e., one for which there's a
MessagePipeDispatcher), it provides the implementation of much of
MessagePipeDispatcher's functionality. It's separated from MessagePipeDispatcher
since a) it has to deal with both endpoints and thus b) it has to operate under
MessagePipe's lock.

The ASCII art diagram looks like:

handle - MessagePipeDispatcher - LocalMessagePipeEndpoint / MessagePipe /
RemoteMessagePipeEndpoint - ... - OS pipe - ... - RemoteMessagePipeEndpoint /
MessagePipe / LocalMessagePipeEndpoint - MessagePipeDispatcher - handle

I suppose things might be a little cleaner if MessagePipes were unidirectional
(but then you'd need four handles for a duplex connection). Hmmm.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/local_message...
File mojo/system/local_message_pipe_endpoint.h (right):

https://codereview.chromium.org/27060003/diff/10001/mojo/system/local_message...
mojo/system/local_message_pipe_endpoint.h:33: // Implementations need only
implement/override these if they are "connected"
On 2013/10/15 06:50:36, darin wrote:
> nit: This comment seems out of place. Looks to have been copied from
> MessagePipeEndpoint. Ditto for the comment above OnPeerClose.

Done.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
File mojo/system/message_pipe_endpoint.h (right):

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
mojo/system/message_pipe_endpoint.h:20: // corresponding to the endpoint, and
|MessagePipeEndpoint| also implements the
On 2013/10/15 06:50:36, darin wrote:
> nit: Is the comment "|MessagePipeEndpoint| also implements the requisite
> functionality, e.g., to read messages and to wait" accurate? It doesn't seem
to
> provide interesting implementations of those functions. They are on
> LocalMessagePipeEndpoint.

Errr, "the implementation of |MessagePipeEndpoint|". I tried to make the comment
a little more clear.

https://codereview.chromium.org/27060003/diff/10001/mojo/system/message_pipe_...
mojo/system/message_pipe_endpoint.h:35: // local |MessagePipeDispatcher| (the
default implementations should never be
On 2013/10/15 06:50:36, darin wrote:
> I'm having trouble following this design. Are you saying that when connected
to
> a non-local MessagePipeDispatcher that these methods may not be implemented /
> overridden? What does that mean?

A MessagePipeDispatcher corresponds to a handle, so in the remote (non-local)
case, there's no MessagePipeDispatcher (locally) and thus nothing to call these
methods.

I've tried to make the comments a little more clear.

[viettrungluu, 2013-10-15 20:05:20]

+davemoore, since darin's out for the rest of this week (and shouldn't be doing
code reviews)

[DaveMoore, 2013-10-15 20:35:33]

lgtm

[commit-bot: I haz the power, 2013-10-15 20:46:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/27060003/24001

[commit-bot: I haz the power, 2013-10-16 00:24:40]

Change committed as 228815