[mojo] Implement pipe fusion API

[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accommodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

BUG=591742
Committed: https://crrev.com/b1e74df4227184deaaee4d192a5fb3c48116f081
Cr-Commit-Position: refs/heads/master@{#380964}

[Ken Rockot(use gerrit already), 2016-03-10 23:06:05]

rockot@chromium.org changed reviewers:
+ amistry@chromium.org, yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-03-10 23:06:06]

Hey, surprise API!

I've discussed this idea independently with at least Yuzhu and Darin at some
point but until now have put off serious consideration. It turns out to provide
an (IMHO) elegant solution to giving shell clients a Connector pipe without
having to block on Initialize from the shell, and it uses the port merge logic
we're already exploiting for convenient cross-process pipes.

We could in theory change shell interfaces so that the client pipe goes in the
other direction initially; i.e. rather a child getting a ShellClient pipe you'd
get a Shell pipe, and rather than waiting for Initialize(Connector, Identity,
uint32), you'd send an Initialize(ShellClient&, Connector&) => (Identity,
uint32)... but I think shell interfaces have already churned quite a bit and
this seems like a useful tool to have in our toolkit for cases when there's a
mismatch of expectations between a client and the interface it's consuming.

All feedback welcome, but Anand could you please review the EDK changes, and
Yuzhu the bindings?

Thanks

[Ken Rockot(use gerrit already), 2016-03-10 23:08:51]

Description was changed from

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accomodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

A concrete example of this is the ShellClient interface, where
Initialize is eventually called by the shell to provide the client with
a functioning Connector interface. Shell client code may wish to begin
issuing requests on the Connector pipe immediately on startup without
waiting for the Initialize request to be received.

The motivation for doing this now is to reduce content child startup
latency: we want MojoShellConnection to be available for immediate
use in child processes, but we currently have to block until Initialize()
for this to be this possible.

BUG=None
==========

to

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accomodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

A concrete example of this is the ShellClient interface, where
Initialize is eventually called by the shell to provide the client with
a functioning Connector interface. Shell client code may wish to begin
issuing requests on the Connector pipe immediately on startup without
waiting for the Initialize request to be received.

The motivation for doing this now is to reduce content child startup
latency: we want MojoShellConnection to be available for immediate
use in child processes, but we currently have to block until Initialize()
for this to be this possible.

BUG=591742
==========

[yzshen1, 2016-03-10 23:49:04]

Awesome!

https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
mojo/public/c/system/message_pipe.h:205: //       handle. In this case both
handles remain unaffected.
Is it easier if we always close the handles? That is one less thing to consider
for the users, and invalid argument is pretty rare. WDYT?

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
File mojo/public/cpp/bindings/interface_request.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
mojo/public/cpp/bindings/interface_request.h:124: bool
FuseProxy(InterfaceRequest<Interface> *request,
How about the following signature:

bool FuseInterface(InterfaceRequset<Interface> request,
                   InterfacePtrInfo<Interface> ptr_info);

- The request and ptr are "symmetric" in this case, so it seems unnecessary to
have "proxy" in the name.
- Pass them by value because they are invalidated anyway.
- Maybe PtrInfo is better than Ptr here?

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
File mojo/public/cpp/system/message_pipe.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
mojo/public/cpp/system/message_pipe.h:95: inline MojoResult
FuseMessagePipes(ScopedMessagePipeHandle *message_pipe0,
nit: * should be adjacent to the type, instead of the name. (here and the next
line)

[Ken Rockot(use gerrit already), 2016-03-11 00:32:46]

https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
mojo/public/c/system/message_pipe.h:205: //       handle. In this case both
handles remain unaffected.
On 2016/03/10 at 23:49:03, yzshen1 wrote:
> Is it easier if we always close the handles? That is one less thing to
consider for the users, and invalid argument is pretty rare. WDYT?

SGTM - I considered both and didn't feel strongly about it, but it makes the C++
APIs much cleaner IMHO. So done.

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
File mojo/public/cpp/bindings/interface_request.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
mojo/public/cpp/bindings/interface_request.h:124: bool
FuseProxy(InterfaceRequest<Interface> *request,
On 2016/03/10 at 23:49:03, yzshen1 wrote:
> How about the following signature:
> 
> bool FuseInterface(InterfaceRequset<Interface> request,
>                    InterfacePtrInfo<Interface> ptr_info);
> 
> - The request and ptr are "symmetric" in this case, so it seems unnecessary to
have "proxy" in the name.
> - Pass them by value because they are invalidated anyway.
> - Maybe PtrInfo is better than Ptr here?

Done

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
File mojo/public/cpp/system/message_pipe.h (right):

https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
mojo/public/cpp/system/message_pipe.h:95: inline MojoResult
FuseMessagePipes(ScopedMessagePipeHandle *message_pipe0,
On 2016/03/10 at 23:49:03, yzshen1 wrote:
> nit: * should be adjacent to the type, instead of the name. (here and the next
line)

WontFix - obsolete :)

[darin (slow to review), 2016-03-11 03:51:07]

Exciting to see this API happening. It is also useful if you want to
implement a stash :)

-Darin

On Thu, Mar 10, 2016 at 7:32 PM, <rockot@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
> File mojo/public/c/system/message_pipe.h (right):
>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/c/system/messag...
> mojo/public/c/system/message_pipe.h:205: // handle. In this case
> both handles remain unaffected.
> On 2016/03/10 at 23:49:03, yzshen1 wrote:
> > Is it easier if we always close the handles? That is one less thing to
> consider for the users, and invalid argument is pretty rare. WDYT?
>
> SGTM - I considered both and didn't feel strongly about it, but it makes
> the C++ APIs much cleaner IMHO. So done.
>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
> File mojo/public/cpp/bindings/interface_request.h (right):
>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/bindings/in...
> mojo/public/cpp/bindings/interface_request.h:124: bool
> FuseProxy(InterfaceRequest<Interface> *request,
> On 2016/03/10 at 23:49:03, yzshen1 wrote:
> > How about the following signature:
> >
> > bool FuseInterface(InterfaceRequset<Interface> request,
> > InterfacePtrInfo<Interface> ptr_info);
> >
> > - The request and ptr are "symmetric" in this case, so it seems
> unnecessary to have "proxy" in the name.
> > - Pass them by value because they are invalidated anyway.
> > - Maybe PtrInfo is better than Ptr here?
>
> Done
>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
> File mojo/public/cpp/system/message_pipe.h (right):
>
>
>
https://codereview.chromium.org/1785843002/diff/1/mojo/public/cpp/system/mess...
> mojo/public/cpp/system/message_pipe.h:95: inline MojoResult
> FuseMessagePipes(ScopedMessagePipeHandle *message_pipe0,
> On 2016/03/10 at 23:49:03, yzshen1 wrote:
> > nit: * should be adjacent to the type, instead of the name. (here and
> the next line)
>
> WontFix - obsolete :)
>
> https://codereview.chromium.org/1785843002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ben Goodger (Google), 2016-03-11 05:21:56]

Can you show a code snippet of what this would look like in the Connector
example?

[Ken Rockot(use gerrit already), 2016-03-11 05:23:12]

On 2016/03/11 at 05:21:56, ben wrote:
> Can you show a code snippet of what this would look like in the Connector
example?

Here's a patch which deletes WaitForInitialize:
https://codereview.chromium.org/1780973004/

In particular look at shell_connection.cc

[Anand Mistry (off Chromium), 2016-03-11 08:32:37]

> We could in theory change shell interfaces so that the client pipe goes in the
> other direction initially; i.e. rather a child getting a ShellClient pipe
you'd
> get a Shell pipe, and rather than waiting for Initialize(Connector, Identity,
> uint32), you'd send an Initialize(ShellClient&, Connector&) => (Identity,
> uint32)... but I think shell interfaces have already churned quite a bit and
> this seems like a useful tool to have in our toolkit for cases when there's a
> mismatch of expectations between a client and the interface it's consuming.

Admittedly, I agree this is a pretty elegant API. However I have two concerns:
1. Increasing EDK complexity due to startup processes.
2. I'm not familiar with the shell, but you've already presented a reasonably
alternative to this. It's not clear to me that this is worth it.

[Ken Rockot(use gerrit already), 2016-03-11 16:09:52]

On Mar 11, 2016 12:32 AM, <amistry@chromium.org> wrote:
>
>
> > We could in theory change shell interfaces so that the client pipe goes
in the
> > other direction initially; i.e. rather a child getting a ShellClient
pipe
> you'd
> > get a Shell pipe, and rather than waiting for Initialize(Connector,
Identity,
> > uint32), you'd send an Initialize(ShellClient&, Connector&) =>
(Identity,
> > uint32)... but I think shell interfaces have already churned quite a
bit and
> > this seems like a useful tool to have in our toolkit for cases when
there's a
> > mismatch of expectations between a client and the interface it's
consuming.
>
> Admittedly, I agree this is a pretty elegant API. However I have two
concerns:
> 1. Increasing EDK complexity due to startup processes.

This doesn't really add much complexity, it just exposes something that was
already there. And actually, given that the fuse call now always closes the
handles I can simplify its changes to MPD. I don't think the use cases are
limited to startup.

> 2. I'm not familiar with the shell, but you've already presented a
reasonably
> alternative to this. It's not clear to me that this is worth it.

Yes, it's a reasonable alternative. I'll let Ben weigh in on that option.
In general though I expect this problem to come up again.

The alternative is that people will inevitably start building their own
queuing impls to buffer requests and forward them later to avoid changing
service interfaces,  and that's a sad.

>
> https://codereview.chromium.org/1785843002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-03-11 17:17:21]

https://codereview.chromium.org/1785843002/diff/80001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.h (right):

https://codereview.chromium.org/1785843002/diff/80001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.h:53: bool
CompleteFuse(MessagePipeDispatcher* other);
It seems possible to only expose a single Fuse() method, and make
BeginFuse/CancelFuse private methods. WDYT?

https://codereview.chromium.org/1785843002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_request.h (right):

https://codereview.chromium.org/1785843002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_request.h:120: // Fuses an
InterfaceRequest<T> endpoint with an InterfacePtr<T> endpoint.
Nit: InterfacePtrInfo

[Ken Rockot(use gerrit already), 2016-03-11 17:19:19]

On 2016/03/11 at 17:17:21, yzshen wrote:
>
https://codereview.chromium.org/1785843002/diff/80001/mojo/edk/system/message...
> File mojo/edk/system/message_pipe_dispatcher.h (right):
> 
>
https://codereview.chromium.org/1785843002/diff/80001/mojo/edk/system/message...
> mojo/edk/system/message_pipe_dispatcher.h:53: bool
CompleteFuse(MessagePipeDispatcher* other);
> It seems possible to only expose a single Fuse() method, and make
BeginFuse/CancelFuse private methods. WDYT?

Yeah, that's what I was going to do. Begin/Cancel were only necessary to support
failures that don't close the handles.

> 
>
https://codereview.chromium.org/1785843002/diff/80001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/interface_request.h (right):
> 
>
https://codereview.chromium.org/1785843002/diff/80001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/interface_request.h:120: // Fuses an
InterfaceRequest<T> endpoint with an InterfacePtr<T> endpoint.
> Nit: InterfacePtrInfo

[yzshen1, 2016-03-11 18:33:59]

bindings LGTM

[Ken Rockot(use gerrit already), 2016-03-11 20:51:57]

BTW I'm going to put together a CL which changes shell interfaces to make
startup more friendly to clients. If that ends up suiting our needs for Chrome
we can punt on the decision to land this API or not.

[Anand Mistry (off Chromium), 2016-03-14 06:18:24]

EDK LGTM.

Ok, I've been convinced of the usefulness of this API.

[Ken Rockot(use gerrit already), 2016-03-14 08:13:13]

Description was changed from

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accomodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

A concrete example of this is the ShellClient interface, where
Initialize is eventually called by the shell to provide the client with
a functioning Connector interface. Shell client code may wish to begin
issuing requests on the Connector pipe immediately on startup without
waiting for the Initialize request to be received.

The motivation for doing this now is to reduce content child startup
latency: we want MojoShellConnection to be available for immediate
use in child processes, but we currently have to block until Initialize()
for this to be this possible.

BUG=591742
==========

to

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accommodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

BUG=591742
==========

[Ken Rockot(use gerrit already), 2016-03-14 13:13:41]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-03-14 13:13:41]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1785843002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-14 13:13:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785843002/120001

[commit-bot: I haz the power, 2016-03-14 13:18:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-14 13:19:52]

Description was changed from

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accommodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

BUG=591742
==========

to

==========
[mojo] Implement pipe fusion API

Sometimes it's desirable to fuse two pipes together. This is
particularly useful when a service provides some kind of interface
pipe later than its consumers would like to have such a pipe
available. Rather than require the service to be rewritten to
accommodate such cases, a consumer could create its own
disconnected pipe and fuse it lazily with a service endpoint.

BUG=591742

Committed: https://crrev.com/b1e74df4227184deaaee4d192a5fb3c48116f081
Cr-Commit-Position: refs/heads/master@{#380964}
==========

[commit-bot: I haz the power, 2016-03-14 13:19:53]

Patchset 7 (id:??) landed as
https://crrev.com/b1e74df4227184deaaee4d192a5fb3c48116f081
Cr-Commit-Position: refs/heads/master@{#380964}