[mojo-edk] Expose notification source to MojoWatch callbacks

[mojo-edk] Expose notification source to MojoWatch callbacks

This adds a flags argument to watch callbacks and exposes a flag
to indicate that the callback was invoked as a result of some
external process event (i.e. an incoming EDK IPC message). The
public C++ Watcher implementation is updated to take advantage
of this, effectively allowing us to dispatch to Mojo bindings
synchronously when they live on the IO thread and are servicing
messages from out-of-process.

BUG=590495
Committed: https://crrev.com/6aca885ca93bb6194e67be3c62ac409745595e3c
Cr-Commit-Position: refs/heads/master@{#381767}

[Ken Rockot(use gerrit already), 2016-03-16 03:07:15]

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

[Ken Rockot(use gerrit already), 2016-03-16 03:07:15]

Yuzhu could you look at bindings, John at //ipc?

Still running real benchmarks for this. Our microbenchmarks improve by 20-25%.
Still more expensive than plain IPC::Channel of course, but this removes one
more source of latency.

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

Also, to clarify since I didn't think it was appropriate to mention this in the
public API comments, any message coming in from out of process for an endpoint
with sync dispatch enabled will synchronously invoke the method/response call on
the IO thread. :)

[Ken Rockot(use gerrit already), 2016-03-16 03:10:44]

On 2016/03/16 at 03:10:05, Ken Rockot wrote:
> Also, to clarify since I didn't think it was appropriate to mention this in
the public API comments, any message coming in from out of process for an
endpoint with sync dispatch enabled will synchronously invoke the
method/response call on the IO thread. :)

Ahem... that is, IF the Binding/InterfacePtr lives on the IO thread.

[jam, 2016-03-16 15:26:26]

On 2016/03/16 03:07:15, Ken Rockot wrote:
> Yuzhu could you look at bindings, John at //ipc?

lgtm (not that you need it)

> 
> Still running real benchmarks for this. Our microbenchmarks improve by 20-25%.
> Still more expensive than plain IPC::Channel of course, but this removes one
> more source of latency.

[yzshen1, 2016-03-16 15:38:18]

https://codereview.chromium.org/1811433002/diff/1/mojo/public/cpp/bindings/bi...
File mojo/public/cpp/bindings/binding.h (right):

https://codereview.chromium.org/1811433002/diff/1/mojo/public/cpp/bindings/bi...
mojo/public/cpp/bindings/binding.h:211: // Allows synchronous dispatch for this
Binding. When sync dispatch is
This comment also needs to mention bindings living on the IO thread and the
client is out of process. And likely that is the only case that we would like to
use this feature. Is that correct?

I feel that this is something related to our implementation details and hard to
explain clearly as a public API. I wonder whether we can have a flag passed into
the watcher API's callback, indicating whether the signal state change is
directly resulted from a Mojo system API call (as opposed to receiving a message
from another process). If it is not, then we should be safe to do sync
dispatching, right?

If the approach above is not possible, maybe we should consider a different
method name. The current name sounds to me that it is related to sync method
call.

WDYT? I am happy to chat about this in person if that is more convenient.

[Ken Rockot(use gerrit already), 2016-03-16 15:54:58]

We could plumb the detail through from the system layer, but adds a lot of
complexity that's hard to get right and has a higher run-time cost. I
started down that path and decided that an opt-in API would be much simpler
and therefore better.

I think "sync dispatch" is an accurate name for the option, but I'm open to
suggestions. IMHO it should be sufficient to accurately document the
meaning.

On Wed, Mar 16, 2016 at 8:38 AM, <yzshen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1811433002/diff/1/mojo/public/cpp/bindings/bi...
> File mojo/public/cpp/bindings/binding.h (right):
>
>
>
https://codereview.chromium.org/1811433002/diff/1/mojo/public/cpp/bindings/bi...
> mojo/public/cpp/bindings/binding.h:211: // Allows synchronous dispatch
> for this Binding. When sync dispatch is
> This comment also needs to mention bindings living on the IO thread and
> the client is out of process. And likely that is the only case that we
> would like to use this feature. Is that correct?
>
> I feel that this is something related to our implementation details and
> hard to explain clearly as a public API. I wonder whether we can have a
> flag passed into the watcher API's callback, indicating whether the
> signal state change is directly resulted from a Mojo system API call (as
> opposed to receiving a message from another process). If it is not, then
> we should be safe to do sync dispatching, right?
>
> If the approach above is not possible, maybe we should consider a
> different method name. The current name sounds to me that it is related
> to sync method call.
>
> WDYT? I am happy to chat about this in person if that is more
> convenient.
>
> https://codereview.chromium.org/1811433002/
>

-- 
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.

[Ken Rockot(use gerrit already), 2016-03-16 18:12:37]

After some more careful consideration I've decided to stick with this approach.
I think it will prove a useful API for writing simpler unit tests in the future.

I've renamed SetAllowSyncDispatch to
EnableImmediateDispatchOfEventsFromSameThread and updated the documentation in
Binding and InterfacePtr. WDYT?

[yzshen1, 2016-03-16 18:31:33]

LGTM

https://codereview.chromium.org/1811433002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/multiplex_router.h (right):

https://codereview.chromium.org/1811433002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/multiplex_router.h:149: void
EnableImmediateDispatchOfEventsFromSameThread(bool enabled) {
nit: DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/1811433002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/associated_interface_unittest.cc (right):

https://codereview.chromium.org/1811433002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:588: explicit
SingleMethodImpl(const Closure& on_call,
nit: explicit is not needed here, but needed on line 608.

[Ken Rockot(use gerrit already), 2016-03-16 21:13:38]

Description was changed from

==========
Enable sync dispatch for Mojo Watcher

This allows bindings endpoints to explicitly opt in to synchronous
dispatch of incoming calls and responses. While users must be careful
to avoid subtle reentrancy issues when both endpoints live on the
same thread, in practice this situation should almost never happen.

This also flips on sync dispatch for ChannelMojo endpoints. Since
all received messages come from out-of-process, it's always safe
to dispatch them synchronously.

BUG=590495
==========

to

==========
[mojo-edk] Expose notification source to MojoWatch callbacks

This adds a flags argument to watch callbacks and exposes a flag
to indicate that the callback was invoked as a result of some
external process event (i.e. an incoming EDK IPC message). The
public C++ Watcher implementation is updated to take advantage
of this, effectively allowing us to dispatch to Mojo bindings
synchronously when they live on the IO thread and are servicing
messages from out-of-process.

BUG=590495
==========

[Ken Rockot(use gerrit already), 2016-03-16 21:13:51]

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

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

+amistry since this has turned into an EDK change

[yzshen1, 2016-03-16 21:53:52]

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:25: if (!tls_context_->Get())
nit: it seems the two tls_context_->Get() (line 25 and 28) can be folded into
one.

[Anand Mistry (off Chromium), 2016-03-16 23:04:14]

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:28: RequestContext* current_context =
tls_context_->Get();
There's something very fishy about this code. Why do you need to save the outer
source?

There are four cases to consider (and other possible cases inductively break
down into these four):
1. LOCAL nested in LOCAL. This is the original code, and the nested context
should be ignored.
2. EXTERNAL nested in LOCAL. This should be impossible.
3. EXTERNAL nested in EXTERNAL. Should also be impossible.
4. LOCAL nested in EXTERNAL. This looks like the case you're trying to handle.
But why? Wouldn't you want to keep the existing behaviour and have the outermost
context (the external one) handle the watchers. You don't need to track the
outer context source, since that's available via TLS context at any point.

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.h:39: explicit RequestContext(Source source);
If local is the default and "external process" is the exceptional case, have a
default constructor that uses local. It avoids all the dispatcher changes. If
you do, make sure you add a comment that local is the default.

And we can use delegated constructors now, so this should be trivial.

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.h:60: //
nit: blank comment line.

https://codereview.chromium.org/1811433002/diff/60001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:188: //  
|MOJO_WATCH_NOTIFICATION_EXTERNAL_PROCESS| - The callback is being invoked
Not a fan of the "EXTERNAL_PROCESS" name, since it:
1. Leaks implementation details
2. Doesn't really reflect the meaning

As discussed, other possibilities:
- Re-entrant safe
- System
- Something, something, empty stack something

[Ken Rockot(use gerrit already), 2016-03-17 00:15:34]

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:25: if (!tls_context_->Get())
On 2016/03/16 at 21:53:52, yzshen1 wrote:
> nit: it seems the two tls_context_->Get() (line 25 and 28) can be folded into
one.

Done

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:28: RequestContext* current_context =
tls_context_->Get();
On 2016/03/16 at 23:04:14, Anand Mistry wrote:
> There's something very fishy about this code. Why do you need to save the
outer source?
> 
> There are four cases to consider (and other possible cases inductively break
down into these four):
> 1. LOCAL nested in LOCAL. This is the original code, and the nested context
should be ignored.
> 2. EXTERNAL nested in LOCAL. This should be impossible.
> 3. EXTERNAL nested in EXTERNAL. Should also be impossible.
> 4. LOCAL nested in EXTERNAL. This looks like the case you're trying to handle.
But why? Wouldn't you want to keep the existing behaviour and have the outermost
context (the external one) handle the watchers. You don't need to track the
outer context source, since that's available via TLS context at any point.

I don't really know how I convinced myself that #4 was necessary to handle, but
I agree that it's not necessary. Removed this crap.

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.h:39: explicit RequestContext(Source source);
On 2016/03/16 at 23:04:14, Anand Mistry wrote:
> If local is the default and "external process" is the exceptional case, have a
default constructor that uses local. It avoids all the dispatcher changes. If
you do, make sure you add a comment that local is the default.
> 
> And we can use delegated constructors now, so this should be trivial.

Done

https://codereview.chromium.org/1811433002/diff/60001/mojo/edk/system/request...
mojo/edk/system/request_context.h:60: //
On 2016/03/16 at 23:04:14, Anand Mistry wrote:
> nit: blank comment line.

done

https://codereview.chromium.org/1811433002/diff/60001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/1811433002/diff/60001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:188: //  
|MOJO_WATCH_NOTIFICATION_EXTERNAL_PROCESS| - The callback is being invoked
On 2016/03/16 at 23:04:14, Anand Mistry wrote:
> Not a fan of the "EXTERNAL_PROCESS" name, since it:
> 1. Leaks implementation details
> 2. Doesn't really reflect the meaning
> 
> As discussed, other possibilities:
> - Re-entrant safe
> - System
> - Something, something, empty stack something

Went with FROM_SYSTEM for the public API flag. The internal Source name is just
SYSTEM now.

[Anand Mistry (off Chromium), 2016-03-17 01:53:09]

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:35: RequestContext::Source
notification_source = tls_context_->Get()->source();
I don't think this is what we want. Consider a stack with an external context
and a nested local one. When the nested local context is destroyed,
tls_context->source is external since the tls refers to the bottom most context
on the stack. So below, any notification is going to be run with from_system as
the source instead of none since this is a local context...

...but... at the same time, there shouldn't be any pending watch callbacks
because this context was nested and nested contexts should be ignored and not
have any watch callbacks.

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
mojo/edk/system/request_context.h:89: Source source_;
nit: const

[Ken Rockot(use gerrit already), 2016-03-17 01:56:36]

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:35: RequestContext::Source
notification_source = tls_context_->Get()->source();
On 2016/03/17 at 01:53:09, Anand Mistry wrote:
> I don't think this is what we want. Consider a stack with an external context
and a nested local one. When the nested local context is destroyed,
tls_context->source is external since the tls refers to the bottom most context
on the stack. So below, any notification is going to be run with from_system as
the source instead of none since this is a local context...
> 
> ...but... at the same time, there shouldn't be any pending watch callbacks
because this context was nested and nested contexts should be ignored and not
have any watch callbacks.

That was the intent. I thought that's what you were suggesting earlier and it
seems correct to me. What's wrong with these calls running in a system context
if they are in fact rooted there on the stack? Perhaps it's not semantically
accurate from some perspective, but that's an internal detail.

[chromium-reviews, 2016-03-17 02:06:01]

On Mar 16, 2016 6:56 PM, <rockot@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
> File mojo/edk/system/request_context.cc (right):
>
>
https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
> mojo/edk/system/request_context.cc:35: RequestContext::Source
> notification_source = tls_context_->Get()->source();
> On 2016/03/17 at 01:53:09, Anand Mistry wrote:
> > I don't think this is what we want. Consider a stack with an external
> context and a nested local one. When the nested local context is
> destroyed, tls_context->source is external since the tls refers to the
> bottom most context on the stack. So below, any notification is going to
> be run with from_system as the source instead of none since this is a
> local context...
> >
> > ...but... at the same time, there shouldn't be any pending watch
> callbacks because this context was nested and nested contexts should be
> ignored and not have any watch callbacks.
>
> That was the intent. I thought that's what you were suggesting earlier
> and it seems correct to me. What's wrong with these calls running in a
> system context if they are in fact rooted there on the stack? Perhaps
> it's not semantically accurate from some perspective, but that's an
> internal detail.
>
> https://codereview.chromium.org/1811433002/

Sorry I think I misunderstood your concern. In the case of a nested context
you're right that there will never be any callbacks to run, so this is
effectively correct behavior, but it's confusing and I will make the
meaning more explicit.

-- 
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.

[Ken Rockot(use gerrit already), 2016-03-17 15:44:30]

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
mojo/edk/system/request_context.cc:35: RequestContext::Source
notification_source = tls_context_->Get()->source();
On 2016/03/17 at 01:56:36, Ken Rockot wrote:
> On 2016/03/17 at 01:53:09, Anand Mistry wrote:
> > I don't think this is what we want. Consider a stack with an external
context and a nested local one. When the nested local context is destroyed,
tls_context->source is external since the tls refers to the bottom most context
on the stack. So below, any notification is going to be run with from_system as
the source instead of none since this is a local context...
> > 
> > ...but... at the same time, there shouldn't be any pending watch callbacks
because this context was nested and nested contexts should be ignored and not
have any watch callbacks.
> 
> That was the intent. I thought that's what you were suggesting earlier and it
seems correct to me. What's wrong with these calls running in a system context
if they are in fact rooted there on the stack? Perhaps it's not semantically
accurate from some perspective, but that's an internal detail.

I've updated the destructor.

Do you agree that all this logic is essentially correct for the case where the
dtor is for the outermost context, and that for all other cases we should just
CHECK that the finalizer list is empty?

We need a new outer context to run each of these callbacks and I can see no
reason why they shouldn't inherit Source from the one we just tore down.

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1811433002/diff/80001/mojo/edk/system/request...
mojo/edk/system/request_context.h:89: Source source_;
On 2016/03/17 at 01:53:09, Anand Mistry wrote:
> nit: const

done

[Ken Rockot(use gerrit already), 2016-03-17 17:09:51]

things are less dumb now

[Anand Mistry (off Chromium), 2016-03-17 17:26:50]

lgtm

https://codereview.chromium.org/1811433002/diff/120001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/120001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:57:
CHECK(watch_notify_finalizers_.container().empty());
optional nit: I'd argue these should be DCHECK instead of CHECK since it should
be impossible and is not a security issue or causes any sort of corruption.

[Ken Rockot(use gerrit already), 2016-03-17 17:29:56]

https://codereview.chromium.org/1811433002/diff/120001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1811433002/diff/120001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:57:
CHECK(watch_notify_finalizers_.container().empty());
On 2016/03/17 at 17:26:49, Anand Mistry wrote:
> optional nit: I'd argue these should be DCHECK instead of CHECK since it
should be impossible and is not a security issue or causes any sort of
corruption.

Done

[Ken Rockot(use gerrit already), 2016-03-17 17:29:57]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-03-17 17:29:58]

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

[commit-bot: I haz the power, 2016-03-17 17:30:15]

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

[commit-bot: I haz the power, 2016-03-17 19:10:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-17 19:12:09]

Description was changed from

==========
[mojo-edk] Expose notification source to MojoWatch callbacks

This adds a flags argument to watch callbacks and exposes a flag
to indicate that the callback was invoked as a result of some
external process event (i.e. an incoming EDK IPC message). The
public C++ Watcher implementation is updated to take advantage
of this, effectively allowing us to dispatch to Mojo bindings
synchronously when they live on the IO thread and are servicing
messages from out-of-process.

BUG=590495
==========

to

==========
[mojo-edk] Expose notification source to MojoWatch callbacks

This adds a flags argument to watch callbacks and exposes a flag
to indicate that the callback was invoked as a result of some
external process event (i.e. an incoming EDK IPC message). The
public C++ Watcher implementation is updated to take advantage
of this, effectively allowing us to dispatch to Mojo bindings
synchronously when they live on the IO thread and are servicing
messages from out-of-process.

BUG=590495

Committed: https://crrev.com/6aca885ca93bb6194e67be3c62ac409745595e3c
Cr-Commit-Position: refs/heads/master@{#381767}
==========

[commit-bot: I haz the power, 2016-03-17 19:12:10]

Patchset 8 (id:??) landed as
https://crrev.com/6aca885ca93bb6194e67be3c62ac409745595e3c
Cr-Commit-Position: refs/heads/master@{#381767}