Send input event IPCs directly from the UI thread

Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
TBR=jam@chromium.org

Committed: https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257
Cr-Commit-Position: refs/heads/master@{#395906}

[Ken Rockot(use gerrit already), 2016-05-19 21:14:15]

Description was changed from

==========
Send input events directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

BUG=612944
==========

to

==========
Send input events directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

[Ken Rockot(use gerrit already), 2016-05-19 21:14:48]

Description was changed from

==========
Send input events directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

to

==========
Send input events directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

[Ken Rockot(use gerrit already), 2016-05-19 21:29:45]

Patchset #2 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-19 21:29:49]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-05-19 21:39:44]

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

[Ken Rockot(use gerrit already), 2016-05-19 21:39:45]

Please take a look.

Other options I rejected after prototyping them:

  - Adding more surface to the IPC::Sender interface. This was
    a lot of churn and seemed less elegant than any alternative.

  - Adding immediate_sender() and ipc_thread_sender() accessors
    to ChannelProxy. This would be OK in the general case but
    RenderProcessHost reveals the desire for ChannelProxy owners
    to have more control over the Sender implementation they
    expose.

In the end I decided it would be best to leave it up to
ChannelProxy owners to expose whichever Sender interface(s)
they like, and started by exposing both on RenderProcessHost.

I'm mildly concerned that the pattern used by RPHI to support
this will be duplicated by other ChannelProxy owners, but it
might not be a big problem. Please let me know what you think.

Thanks!

[Ken Rockot(use gerrit already), 2016-05-19 21:40:09]

Description was changed from

==========
Send input events directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

to

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

[Charlie Reis, 2016-05-20 00:11:42]

creis@chromium.org changed reviewers:
+ kenrb@chromium.org, piman@chromium.org

[Charlie Reis, 2016-05-20 00:11:42]

[+piman,kenrb]

I haven't looked yet, but it looks like this is failing a lot of tests.

I'm also not familiar enough with the input event logic to know if it's safe to
switch them over, so I'm adding piman@ and kenrb@ for their thoughts.

Hopefully I can look at the rest tomorrow.

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

On 2016/05/20 at 00:11:42, creis wrote:
> [+piman,kenrb]
> 
> I haven't looked yet, but it looks like this is failing a lot of tests.
> 

Ah right, I made the mock behavior incorrect. Easy fix!

> I'm also not familiar enough with the input event logic to know if it's safe
to switch them over, so I'm adding piman@ and kenrb@ for their thoughts.
> 
> Hopefully I can look at the rest tomorrow.

[piman, 2016-05-20 00:23:57]

On 2016/05/20 00:11:42, Charlie Reis (slow to reply) wrote:
> [+piman,kenrb]
> 
> I haven't looked yet, but it looks like this is failing a lot of tests.
> 
> I'm also not familiar enough with the input event logic to know if it's safe
to
> switch them over, so I'm adding piman@ and kenrb@ for their thoughts.

Do we need to maintain ordering wrt things like resize event or BeginFrame?

> 
> Hopefully I can look at the rest tomorrow.

[piman, 2016-05-20 00:35:41]

https://codereview.chromium.org/1991323002/diff/40001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/1991323002/diff/40001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:136: bool SendNow(Message* message);
drive-by: it would be nice to take this occasion to fix this interface to take a
std::unique_ptr<Message>, since it takes ownership. It's hard to do at the
IPC::Sender level today because of the sheer amount of implementations, but if
we're going to progressively migrate callers, I think it would be a great
occasion to do this at the same time. thoughts?

[Ken Rockot(use gerrit already), 2016-05-20 02:10:04]

On 2016/05/20 at 00:35:41, piman wrote:
> https://codereview.chromium.org/1991323002/diff/40001/ipc/ipc_channel_proxy.h
> File ipc/ipc_channel_proxy.h (right):
> 
>
https://codereview.chromium.org/1991323002/diff/40001/ipc/ipc_channel_proxy.h...
> ipc/ipc_channel_proxy.h:136: bool SendNow(Message* message);
> drive-by: it would be nice to take this occasion to fix this interface to take
a std::unique_ptr<Message>, since it takes ownership. It's hard to do at the
IPC::Sender level today because of the sheer amount of implementations, but if
we're going to progressively migrate callers, I think it would be a great
occasion to do this at the same time. thoughts?

Seems reasonable to me. I've updated the new methods to use unique_ptr, and also
updated some bits of RPHI as well to do the same while here.

[Ken Rockot(use gerrit already), 2016-05-20 02:17:20]

On 2016/05/20 at 00:23:57, piman wrote:
> On 2016/05/20 00:11:42, Charlie Reis (slow to reply) wrote:
> > [+piman,kenrb]
> > 
> > I haven't looked yet, but it looks like this is failing a lot of tests.
> > 
> > I'm also not familiar enough with the input event logic to know if it's safe
to
> > switch them over, so I'm adding piman@ and kenrb@ for their thoughts.
> 
> Do we need to maintain ordering wrt things like resize event or BeginFrame?
> 

I tried to figure this out and it's still unclear to me. It will either take a
lot more investigation, or hopefully someone knows enough about this part of the
stack that they can comment on it.

Of course there's also always the possibility that nobody was ever assuming or
explicitly relying on a particular ordering, but that ordering will impact
behavior all the same...

> > 
> > Hopefully I can look at the rest tomorrow.

https://codereview.chromium.org/1991323002/diff/60001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (left):

https://codereview.chromium.org/1991323002/diff/60001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:324: return;
Heh, looks like this leaked before. Though it was probably very uncommon to hit
this, still a leak.

[piman, 2016-05-20 02:45:00]

piman@chromium.org changed reviewers:
+ rbyers@chromium.org

[piman, 2016-05-20 02:45:01]

+rbyers, do you have some thoughts about this? This means the input events are
sent directly to the renderer from the UI thread, without an IO thread hop on
the browser side (hopefully lower latency), but that they're now out-of-order
with respect to other messages, such as resize, BeginFrame, visibility, or even
render widget lifetime messages. Would that be an issue?

[Rick Byers, 2016-05-20 14:03:51]

Description was changed from

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

to

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

[Rick Byers, 2016-05-20 14:03:51]

rbyers@chromium.org changed reviewers:
+ skyostil@chromium.org

[Rick Byers, 2016-05-20 14:12:18]

On 2016/05/20 02:45:01, piman wrote:
> +rbyers, do you have some thoughts about this? This means the input events are
> sent directly to the renderer from the UI thread, without an IO thread hop on
> the browser side (hopefully lower latency), but that they're now out-of-order
> with respect to other messages, such as resize, BeginFrame, visibility, or
even
> render widget lifetime messages. Would that be an issue?

I think this is OK because we already do a bunch of queuing/flow-control of
input event messages in the browser process.  Our only invariant is that the
input events remaining in the same order with respect to other events from the
same input device.  But it's possible this might change the behavior in the
common case on Android with vsync aligned input due to the relative order of
input events and BeginMainFrame.  +skyostila / tdresser for their thoughts.


Note that our (somewhat anecdotal) data suggest we won't see much latency
improvement from this.  The cost of all the thread hops isn't so much in the
context switch but in the risk that the thread will be busy, and the browser's
I/O thread is actually one of the few fairly realiably clean threads we have. 
Still I'm happy to see this - every little bit helps.  Eg. on Android the
latency impact of the browser I/O thread is certainly still non-trivial (95th
percentile is 3ms -
https://uma.googleplex.com/p/chrome/histograms/?endDate=05-18-2016&dayCount=7...).
 We've got a lot of UMA metrics around scroll latency - it'll be interesting to
see if/how those move in response to this change.

[kenrb, 2016-05-20 14:21:12]

Don't all InputMsg_* messages already get thread hops in the renderer process to
give the compositor thread first crack at handling them? That would mean any
ordering assumptions between InputMsg_* and other types of messages would
already be broken.

[piman, 2016-05-20 15:18:08]

On Fri, May 20, 2016 at 7:21 AM, <kenrb@chromium.org> wrote:

> Don't all InputMsg_* messages already get thread hops in the renderer
> process to
> give the compositor thread first crack at handling them? That would mean
> any
> ordering assumptions between InputMsg_* and other types of messages would
> already be broken.
>

FYI some other messages, like BeginFrame also first hop to the compositor
thread, following the same flow as input events.


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

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

[Sami, 2016-05-23 17:24:47]

+enne for any unified BeginFrame overlap.

Is there a chance that with this patch input events could arrive after the
BeginFrame from the browser even if they were sent in the opposite order? If so,
my follow-up wish would be to make BeginFrame use this same IPC path :)

FWIW the renderer main thread tries hard to avoid reordering input events and
BeginMainFrame. I'm not sure what the specific correctness issue there is
however.

[piman, 2016-05-23 17:49:57]

On Mon, May 23, 2016 at 10:24 AM, skyostil@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> +enne for any unified BeginFrame overlap.
>
> Is there a chance that with this patch input events could arrive after the
> BeginFrame from the browser even if they were sent in the opposite order?
>

I'm not sure - Ken? What happens if the pipe is full when sending directly
from the caller thread, does the caller block, or is the message deferred
until the pipe is free? In the latter case, is there any guarantee relative
to other messages sent via the IO thread?


> If so,
> my follow-up wish would be to make BeginFrame use this same IPC path :)
>

Which in turn would need to pull ViewMsg_SwapCompositorFrameAck
and ViewMsg_ReclaimCompositorResources, I think.


> FWIW the renderer main thread tries hard to avoid reordering input events
> and
> BeginMainFrame. I'm not sure what the specific correctness issue there is
> however.
>
> https://codereview.chromium.org/1991323002/
>

-- 
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-05-23 17:58:21]

On Mon, May 23, 2016 at 10:49 AM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, May 23, 2016 at 10:24 AM, skyostil@chromium.org via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
> wrote:
>
>> +enne for any unified BeginFrame overlap.
>>
>> Is there a chance that with this patch input events could arrive after the
>> BeginFrame from the browser even if they were sent in the opposite order?
>>
>
> I'm not sure - Ken? What happens if the pipe is full when sending directly
> from the caller thread, does the caller block, or is the message deferred
> until the pipe is free? In the latter case, is there any guarantee relative
> to other messages sent via the IO thread?
>

The message is be deferred until the pipe becomes writable again.

This won't affect ordering: a message is always queued if it's sent while
the the outgoing queue is nonempty.


>
>> If so,
>> my follow-up wish would be to make BeginFrame use this same IPC path :)
>>
>
> Which in turn would need to pull ViewMsg_SwapCompositorFrameAck
> and ViewMsg_ReclaimCompositorResources, I think.
>
>
>> FWIW the renderer main thread tries hard to avoid reordering input events
>> and
>> BeginMainFrame. I'm not sure what the specific correctness issue there is
>> however.
>>
>> https://codereview.chromium.org/1991323002/
>>
>
>

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

[piman, 2016-05-23 18:06:40]

On Mon, May 23, 2016 at 10:58 AM, Ken Rockot <rockot@chromium.org> wrote:

> On Mon, May 23, 2016 at 10:49 AM, Antoine Labour <piman@chromium.org>
> wrote:
>
>>
>>
>> On Mon, May 23, 2016 at 10:24 AM, skyostil@chromium.org via
>> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
>> wrote:
>>
>>> +enne for any unified BeginFrame overlap.
>>>
>>> Is there a chance that with this patch input events could arrive after
>>> the
>>> BeginFrame from the browser even if they were sent in the opposite order?
>>>
>>
>> I'm not sure - Ken? What happens if the pipe is full when sending
>> directly from the caller thread, does the caller block, or is the message
>> deferred until the pipe is free? In the latter case, is there any guarantee
>> relative to other messages sent via the IO thread?
>>
>
> The message is be deferred until the pipe becomes writable again.
>
> This won't affect ordering: a message is always queued if it's sent while
> the the outgoing queue is nonempty.
>

Ok, then I think it ensures that the input event can only be reordered to
*before* a previous BeginFrame, never to *after* a subsequent BeginFrame.

Antoine


>
>>
>>> If so,
>>> my follow-up wish would be to make BeginFrame use this same IPC path :)
>>>
>>
>> Which in turn would need to pull ViewMsg_SwapCompositorFrameAck
>> and ViewMsg_ReclaimCompositorResources, I think.
>>
>>
>>> FWIW the renderer main thread tries hard to avoid reordering input
>>> events and
>>> BeginMainFrame. I'm not sure what the specific correctness issue there is
>>> however.
>>>
>>> https://codereview.chromium.org/1991323002/
>>>
>>
>>
>

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

[Charlie Reis, 2016-05-23 23:33:12]

A lot of the details here are beyond me, but I appreciate the approach of
exposing both options and leaving the old one as the default.  We can move
things over as we determine they're safe.

A few minor comments below, but hopefully piman@ or an IPC owner can verify that
the details are right.

https://codereview.chromium.org/1991323002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:481: return
safe_proxy_->SendImpl(base::WrapUnique(message), send_now_);
Is there a reason not to just store process_ on SenderProxy and put line 469
here?  Seems a bit indirect to get a SenderProxy from SafeSenderProxy and then
have to go through SafeSenderProxy to get to RPH on every send.  Then
SafeSenderProxy wouldn't need the pointer to RPH, and SenderProxy wouldn't need
a SafeSenderProxy pointer.

https://codereview.chromium.org/1991323002/diff/60001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1991323002/diff/60001/content/public/browser/...
content/public/browser/render_process_host.h:187: virtual IPC::Sender*
GetIOThreadSender() = 0;
These don't seem to have any callers outside content/ yet.  Is it possible to
add one for at least one of them?  (We generally don't add new public methods
until they're used by an embedder.)

https://www.chromium.org/developers/content-module/content-api
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

[Ken Rockot(use gerrit already), 2016-05-24 01:35:26]

On 2016/05/23 at 23:33:12, creis wrote:
> A lot of the details here are beyond me, but I appreciate the approach of
exposing both options and leaving the old one as the default.  We can move
things over as we determine they're safe.
> 
> A few minor comments below, but hopefully piman@ or an IPC owner can verify
that the details are right.
> 
>
https://codereview.chromium.org/1991323002/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1991323002/diff/60001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:481: return
safe_proxy_->SendImpl(base::WrapUnique(message), send_now_);
> Is there a reason not to just store process_ on SenderProxy and put line 469
here?  Seems a bit indirect to get a SenderProxy from SafeSenderProxy and then
have to go through SafeSenderProxy to get to RPH on every send.  Then
SafeSenderProxy wouldn't need the pointer to RPH, and SenderProxy wouldn't need
a SafeSenderProxy pointer.
> 

Done.

>
https://codereview.chromium.org/1991323002/diff/60001/content/public/browser/...
> File content/public/browser/render_process_host.h (right):
> 
>
https://codereview.chromium.org/1991323002/diff/60001/content/public/browser/...
> content/public/browser/render_process_host.h:187: virtual IPC::Sender*
GetIOThreadSender() = 0;
> These don't seem to have any callers outside content/ yet.  Is it possible to
add one for at least one of them?  (We generally don't add new public methods
until they're used by an embedder.)
> 
> https://www.chromium.org/developers/content-module/content-api
> "only expose methods in the public API that embedders need. If a method is
only used by other code in content, it belongs in foo_impl.h and not foo.h."

So I actually tried to avoid making them public in this CL initially, but tests
give the InputRouterImpl a mock RPH so there wasn't really a clean way to do
this.

I've added trivial usage of GetImmediateSender() in src/chrome; as far as I can
tell this won't affect ordering in any meaningful way.

I had a hard time finding a place where I could legitimately use
GetIOThreadSender() outside of content. Maybe I should just get rid of that
interface for this CL.

[Sami, 2016-05-24 10:11:49]

On 2016/05/23 18:06:40, piman wrote:
> On Mon, May 23, 2016 at 10:58 AM, Ken Rockot <mailto:rockot@chromium.org>
wrote:
> 
> > On Mon, May 23, 2016 at 10:49 AM, Antoine Labour <mailto:piman@chromium.org>
> > wrote:
> >
> >>
> >>
> >> On Mon, May 23, 2016 at 10:24 AM, mailto:skyostil@chromium.org via
> >> http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com>
> >> wrote:
> >>
> >>> +enne for any unified BeginFrame overlap.
> >>>
> >>> Is there a chance that with this patch input events could arrive after
> >>> the
> >>> BeginFrame from the browser even if they were sent in the opposite order?
> >>>
> >>
> >> I'm not sure - Ken? What happens if the pipe is full when sending
> >> directly from the caller thread, does the caller block, or is the message
> >> deferred until the pipe is free? In the latter case, is there any guarantee
> >> relative to other messages sent via the IO thread?
> >>
> >
> > The message is be deferred until the pipe becomes writable again.
> >
> > This won't affect ordering: a message is always queued if it's sent while
> > the the outgoing queue is nonempty.
> >
> 
> Ok, then I think it ensures that the input event can only be reordered to
> *before* a previous BeginFrame, never to *after* a subsequent BeginFrame.
> 

Okay, thanks for confirming. This seems fine then. The input latency perf bots
will tell us if we were wrong :)

[piman, 2016-05-24 17:24:51]

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:470: }
nit: this is not needed any more, I think.

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:491:
RenderProcessHostImpl* const process_;
nit: neither is this.

https://codereview.chromium.org/1991323002/diff/80001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/1991323002/diff/80001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:349: return true;
I also noted that SyncMessageFilter will also skip the IO thread hops if this is
true - causing equivalent issues in at least GPU code. Could it be fixed
similarly to ChannelProxy in this CL, before turning this on?

[Ken Rockot(use gerrit already), 2016-05-24 18:00:08]

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:470: }
On 2016/05/24 at 17:24:51, piman wrote:
> nit: this is not needed any more, I think.

Removed

https://codereview.chromium.org/1991323002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:491:
RenderProcessHostImpl* const process_;
On 2016/05/24 at 17:24:51, piman wrote:
> nit: neither is this.

Ditto

https://codereview.chromium.org/1991323002/diff/80001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/1991323002/diff/80001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:349: return true;
On 2016/05/24 at 17:24:51, piman wrote:
> I also noted that SyncMessageFilter will also skip the IO thread hops if this
is true - causing equivalent issues in at least GPU code. Could it be fixed
similarly to ChannelProxy in this CL, before turning this on?

Thanks, forgot about SyncMessageFilter. So for now I've also added SendNow to
SyncMessageFilter but haven't added any consumers. Send always posts.

[Charlie Reis, 2016-05-24 18:11:56]

Thanks.  content/ LGTM, with possibly one more bit of simplification.  Please
wait for piman's approval as well.


> >
>
https://codereview.chromium.org/1991323002/diff/60001/content/public/browser/...
> > content/public/browser/render_process_host.h:187: virtual IPC::Sender*
> GetIOThreadSender() = 0;
> > These don't seem to have any callers outside content/ yet.  Is it possible
to
> add one for at least one of them?  (We generally don't add new public methods
> until they're used by an embedder.)
> > 
> > https://www.chromium.org/developers/content-module/content-api
> > "only expose methods in the public API that embedders need. If a method is
> only used by other code in content, it belongs in foo_impl.h and not foo.h."
> 
> So I actually tried to avoid making them public in this CL initially, but
tests
> give the InputRouterImpl a mock RPH so there wasn't really a clean way to do
> this.
> 
> I've added trivial usage of GetImmediateSender() in src/chrome; as far as I
can
> tell this won't affect ordering in any meaningful way.
> 
> I had a hard time finding a place where I could legitimately use
> GetIOThreadSender() outside of content. Maybe I should just get rid of that
> interface for this CL.

That should be sufficient, thanks.  Let's keep both in the public interface,
especially given the mock RPH setup.

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:487: SenderProxy
io_thread_sender_;
We could just put these straight on RPHI and skip the SafeSenderProxy class now,
so that there's only one nested class needed for this.  I don't feel strongly if
you want to keep the encapsulation, though.

[piman, 2016-05-24 19:00:26]

lgtm

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:487: SenderProxy
io_thread_sender_;
On 2016/05/24 18:11:55, Charlie Reis wrote:
> We could just put these straight on RPHI and skip the SafeSenderProxy class
now,
> so that there's only one nested class needed for this.  I don't feel strongly
if
> you want to keep the encapsulation, though.

This sounds like a good idea.
We can keep encapsulation by having 2 unique_ptr<SenderProxy> fields in RPHI

[Ken Rockot(use gerrit already), 2016-05-24 19:15:11]

Description was changed from

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
==========

to

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
TBR=jam@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-05-24 19:15:38]

Thanks all for the reviews and helpful feedback

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1991323002/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:487: SenderProxy
io_thread_sender_;
On 2016/05/24 at 19:00:26, piman wrote:
> On 2016/05/24 18:11:55, Charlie Reis wrote:
> > We could just put these straight on RPHI and skip the SafeSenderProxy class
now,
> > so that there's only one nested class needed for this.  I don't feel
strongly if
> > you want to keep the encapsulation, though.
> 
> This sounds like a good idea.
> We can keep encapsulation by having 2 unique_ptr<SenderProxy> fields in RPHI

Agreed, and done

[Ken Rockot(use gerrit already), 2016-05-24 19:15:46]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-05-24 19:15:46]

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

[commit-bot: I haz the power, 2016-05-24 19:16:38]

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

[commit-bot: I haz the power, 2016-05-24 22:03:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 22:03:50]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-05-25 16:21:19]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-05-25 16:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1991323002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-05-25 16:21:43]

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

[commit-bot: I haz the power, 2016-05-25 16:26:00]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-05-25 16:28:20]

Description was changed from

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
TBR=jam@chromium.org
==========

to

==========
Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG=612944
TBR=jam@chromium.org

Committed: https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257
Cr-Commit-Position: refs/heads/master@{#395906}
==========

[commit-bot: I haz the power, 2016-05-25 16:28:22]

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