WebVR: process animations from posted task to yield for other events

WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from OnVSync, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
Review-Url: https://codereview.chromium.org/2748293002
Cr-Commit-Position: refs/heads/master@{#457189}
Committed: https://chromium.googlesource.com/chromium/src/+/ff386cfefd3fe0f9cef72ff28f7c7f09d972608d

[klausw, 2017-03-15 01:16:27]

Description was changed from

==========
Call GetVSync from a posted task to let other events be processed

If we call GetVSync inline from a rAF context, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

to

==========
Call GetVSync from a posted task to let other events be processed

If we call GetVSync inline from a rAF context, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

[klausw, 2017-03-15 01:16:27]

klausw@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2017-03-15 15:17:35]

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:176:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
Two comments:
1. This shouldn't be necessary, this sounds like a mojo bug - it shouldn't be
starving out other IPC.

2. You should move the postTask to VRDisplay::OnVSync, because I think we want
to request the VSync early, since the process hops will add a bunch of delay and
we would prefer to do useful things during that delay.

[klausw, 2017-03-15 15:42:40]

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:176:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
On 2017/03/15 15:17:35, mthiesse wrote:
> Two comments:
> 1. This shouldn't be necessary, this sounds like a mojo bug - it shouldn't be
> starving out other IPC.

Yes, this was unexpected. It's not just starving IPC, it seems as if it
processes all received calls on its incoming mojo queue in a loop before
yielding so nothing else gets to execute that's not a direct result of this mojo
processing. I suspect this behavior gets triggered by the
waitForIncomingMethodCall which ends up moving a received VSync to the "ready to
execute now" queue. (This is a hypothesis, not sure if it's really how it works
internally.)

> 2. You should move the postTask to VRDisplay::OnVSync, because I think we want
> to request the VSync early, since the process hops will add a bunch of delay
and
> we would prefer to do useful things during that delay.

You mean moving the serviceScriptedAnimations call there to a posted task? I'll
try if that works, if yes it would indeed be cleaner. If not, I can try posting
the GetVSync request a bit earlier from submitFrame if I switch to the alternate
timing where it waits for transfer at the start of the next frame.

Another alternative might be to have VrShellGl actively send a VSync without
waiting for the client to request it, with throttling to let at most one be
outstanding. That could help get timing more consistent, but it's a larger
change since it doesn't fit with the current callback-based logic.

[klausw, 2017-03-15 15:50:59]

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:176:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
On 2017/03/15 15:42:39, klausw wrote:
> On 2017/03/15 15:17:35, mthiesse wrote:
> > 2. You should move the postTask to VRDisplay::OnVSync, because I think we
want
> > to request the VSync early, since the process hops will add a bunch of delay
> and
> > we would prefer to do useful things during that delay.
> 
> You mean moving the serviceScriptedAnimations call there to a posted task?
I'll
> try if that works, if yes it would indeed be cleaner.

To clarify, I think that executing the next GetVSync early from OnVSync would
still run into the same problem we have now, the issue seems to be that we don't
want the VSync callback being already available for processing when it finishes
current work. Specifically (if my hypothesis is right) it mustn't arrive before
or while we're doing a waitForIncomingMethodCall. I think that posting a
deferred GetVSync from OnVSync would be equivalent to doing so in rAF since in
both cases it would be executed after the current serviceScriptedAnimations call
executes, I think there's no yielding happening in between.

[klausw, 2017-03-15 17:39:25]

PTAL - the timing looks great now when combined with your crrev.com/2754493002
at ps3.

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:176:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
On 2017/03/15 15:42:39, klausw wrote:
> You mean moving the serviceScriptedAnimations call there to a posted task?
I'll
> try if that works, if yes it would indeed be cleaner.

The deferred serviceScriptedAnimations works, going with that.

[mthiesse, 2017-03-15 17:43:51]

lgtm with updated comment

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
Comment needs to be updated.

[mthiesse, 2017-03-15 17:44:37]

I should note that in the longer term, we should really try to remove our
"WaitForIncomingMethodCall"s.

[klausw, 2017-03-15 17:51:47]

Description was changed from

==========
Call GetVSync from a posted task to let other events be processed

If we call GetVSync inline from a rAF context, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

to

==========
WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from a rAF context, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

[klausw, 2017-03-15 17:55:40]

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
On 2017/03/15 17:43:51, mthiesse wrote:
> Comment needs to be updated.

The comment is accurate, but I'll rephrase it to be less confusing.

I think the sequence was:

while (mojo stuff to do)
  OnVSync
    serviceScriptedAnimations
      JS rAF callback
        rAF
          GetVSync
        ...
        submitFrame
          (VSync arrives, add to mojo queue)
  (keep going since queue isn't empty)

[klausw, 2017-03-15 18:03:22]

On 2017/03/15 17:44:37, mthiesse wrote:
> I should note that in the longer term, we should really try to remove our
> "WaitForIncomingMethodCall"s.

I agree, or at least make it conditional on the render path. We do need it for
the planned "direct draw to surface" path(*), but for the mailbox path it should
be possible to do the necessary sequencing on the VrShellGl side. I'll look into
it.

(*) We want to allow JS drawing to proceed while the previous frame is still
rendering, but need to ensure we don't swapBuffers on the result before that
render completes. This sequencing needs to happen within an active JS execution
context, so we need some kind of side channel to sync it.

[mthiesse, 2017-03-15 18:05:04]

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
On 2017/03/15 17:55:40, klausw wrote:
> On 2017/03/15 17:43:51, mthiesse wrote:
> > Comment needs to be updated.
> 
> The comment is accurate, but I'll rephrase it to be less confusing.
> 
> I think the sequence was:
> 
> while (mojo stuff to do)
>   OnVSync
>     serviceScriptedAnimations
>       JS rAF callback
>         rAF
>           GetVSync
>         ...
>         submitFrame
>           (VSync arrives, add to mojo queue)
>   (keep going since queue isn't empty)

The problem is the comment says "If we call GetVSync inline here via...", but we
do call GetVSync inline here. We should instead say that we need to inject a
posted task with every frame to prevent starvation of non-mojo IPC, as our
render loop blocks on mojo IPC and ends up taking priority over other posted
tasks as a result.

[klausw, 2017-03-15 18:14:26]

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
On 2017/03/15 18:05:03, mthiesse wrote:
> On 2017/03/15 17:55:40, klausw wrote:
> > On 2017/03/15 17:43:51, mthiesse wrote:
> > > Comment needs to be updated.
> > 
> > The comment is accurate, but I'll rephrase it to be less confusing.
> > 
> > I think the sequence was:
> > 
> > while (mojo stuff to do)
> >   OnVSync
> >     serviceScriptedAnimations
> >       JS rAF callback
> >         rAF
> >           GetVSync
> >         ...
> >         submitFrame
> >           (VSync arrives, add to mojo queue)
> >   (keep going since queue isn't empty)
> 
> The problem is the comment says "If we call GetVSync inline here via...", but
we
> do call GetVSync inline here. We should instead say that we need to inject a
> posted task with every frame to prevent starvation of non-mojo IPC, as our
> render loop blocks on mojo IPC and ends up taking priority over other posted
> tasks as a result.

No, we don't call GetVSync inline here. It's now called from the posted task,
the scheduled animation will call rAF from the JS code it's executing.

Anyway, comment rephrased to be less detailed, the bug pointer should be enough
for context.

[klausw, 2017-03-15 18:16:37]

klausw@chromium.org changed reviewers:
+ bajones@chromium.org

[klausw, 2017-03-15 18:17:59]

Description was changed from

==========
WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from a rAF context, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

to

==========
WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from OnVSync, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

[mthiesse, 2017-03-15 18:19:01]

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
On 2017/03/15 18:14:26, klausw wrote:
> On 2017/03/15 18:05:03, mthiesse wrote:
> > On 2017/03/15 17:55:40, klausw wrote:
> > > On 2017/03/15 17:43:51, mthiesse wrote:
> > > > Comment needs to be updated.
> > > 
> > > The comment is accurate, but I'll rephrase it to be less confusing.
> > > 
> > > I think the sequence was:
> > > 
> > > while (mojo stuff to do)
> > >   OnVSync
> > >     serviceScriptedAnimations
> > >       JS rAF callback
> > >         rAF
> > >           GetVSync
> > >         ...
> > >         submitFrame
> > >           (VSync arrives, add to mojo queue)
> > >   (keep going since queue isn't empty)
> > 
> > The problem is the comment says "If we call GetVSync inline here via...",
but
> we
> > do call GetVSync inline here. We should instead say that we need to inject a
> > posted task with every frame to prevent starvation of non-mojo IPC, as our
> > render loop blocks on mojo IPC and ends up taking priority over other posted
> > tasks as a result.
> 
> No, we don't call GetVSync inline here. It's now called from the posted task,
> the scheduled animation will call rAF from the JS code it's executing.
> 
> Anyway, comment rephrased to be less detailed, the bug pointer should be
enough
> for context.

We should at least mention in this comment that the starvation is due to
WaitForIncomingMethodCall

[klausw, 2017-03-15 18:21:01]

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:726: // execution context
finishes. If we call GetVSync inline here via
On 2017/03/15 18:19:01, mthiesse wrote:
> On 2017/03/15 18:14:26, klausw wrote:
> > On 2017/03/15 18:05:03, mthiesse wrote:
> > > On 2017/03/15 17:55:40, klausw wrote:
> > > > On 2017/03/15 17:43:51, mthiesse wrote:
> > > > > Comment needs to be updated.
> > > > 
> > > > The comment is accurate, but I'll rephrase it to be less confusing.
> > > > 
> > > > I think the sequence was:
> > > > 
> > > > while (mojo stuff to do)
> > > >   OnVSync
> > > >     serviceScriptedAnimations
> > > >       JS rAF callback
> > > >         rAF
> > > >           GetVSync
> > > >         ...
> > > >         submitFrame
> > > >           (VSync arrives, add to mojo queue)
> > > >   (keep going since queue isn't empty)
> > > 
> > > The problem is the comment says "If we call GetVSync inline here via...",
> but
> > we
> > > do call GetVSync inline here. We should instead say that we need to inject
a
> > > posted task with every frame to prevent starvation of non-mojo IPC, as our
> > > render loop blocks on mojo IPC and ends up taking priority over other
posted
> > > tasks as a result.
> > 
> > No, we don't call GetVSync inline here. It's now called from the posted
task,
> > the scheduled animation will call rAF from the JS code it's executing.
> > 
> > Anyway, comment rephrased to be less detailed, the bug pointer should be
> enough
> > for context.
> 
> We should at least mention in this comment that the starvation is due to
> WaitForIncomingMethodCall

Done.

[billorr, 2017-03-15 18:35:45]

billorr@chromium.org changed reviewers:
+ billorr@chromium.org

[billorr, 2017-03-15 18:35:45]

minor comment/question, but lgtm

https://codereview.chromium.org/2748293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:733:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
Does this measurably have a negative effect on latency?

Should we set m_pendingVsync = false in the posted task rather than here so we
can't queue many posted tasks and get behind?

[bajones, 2017-03-15 18:50:14]

LGTM with Bill's question answered.

[klausw, 2017-03-15 19:00:17]

https://codereview.chromium.org/2748293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2748293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:733:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(
On 2017/03/15 18:35:45, billorr wrote:
> Does this measurably have a negative effect on latency?

I don't think so, if I understand it right it's just saving a closure and
running it in the same thread from the work queue immediately after the current
context finishes. There's no task switching, scheduling, or IPC involved.
 
> Should we set m_pendingVsync = false in the posted task rather than here so we
> can't queue many posted tasks and get behind?

No, we're not going to be queueing multiple posted tasks. We're changing state
to indicate that we're not going to be getting another vsync because none is
scheduled. The VSync events don't happen automatically, we only get one per
GetVSync.

It gets flipped back to "pending" when the scheduled animation and its rAF
callback calls rAF which calls GetVSync.

[klausw, 2017-03-15 19:01:35]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-15 19:01:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2748293002/#ps60001
(title: "Throw shade at WaitForIncomingMethodCall in comment.")

[commit-bot: I haz the power, 2017-03-15 19:02:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 20:29:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489604495781230,
"parent_rev": "bd6fdcf63999268426e5e80802b5ab261fb2e1a2", "commit_rev":
"ff386cfefd3fe0f9cef72ff28f7c7f09d972608d"}

[commit-bot: I haz the power, 2017-03-15 20:30:30]

Description was changed from

==========
WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from OnVSync, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444
==========

to

==========
WebVR: process animations from posted task to yield for other events

If we call GetVSync inline from OnVSync, we can end up with the next
VSync already being ready and scheduled when we exit, and then we'll process
several frames in a row without ever yielding back to the main event loop,
causing input processing delay.

BUG=701444

Review-Url: https://codereview.chromium.org/2748293002
Cr-Commit-Position: refs/heads/master@{#457189}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff386cfefd3fe0f9cef72ff28f7c...
==========

[commit-bot: I haz the power, 2017-03-15 20:30:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ff386cfefd3fe0f9cef72ff28f7c...