sync compositor: Send ipc reply in swap

sync compositor: Send ipc reply in swap

Refactor SynchronousCompositorOutputSurface to expose the SwapBuffers
call to client. For IPC implementation, send the reply IPC inside
SwapBuffers instead of waiting for OnDraw to return.

This allows more code to run in parallel, since OnDraw generally calls
PrepareTiles after DrawAndSwap; now PrepareTiles can run in parallel
with IO/UI code. Depending on scheduling and available cores, this can
save up to ~0.7ms per frame of waiting time on UI thread.

BUG=545633
Committed: https://crrev.com/6c0ed631e1b36d36fec9a8e4cd83f6824ef58262
Cr-Commit-Position: refs/heads/master@{#367111}

[boliu, 2015-12-28 20:37:22]

boliu@chromium.org changed reviewers:
+ hush@chromium.org

[boliu, 2015-12-28 20:37:23]

around these days? ptal?

[hush (inactive), 2015-12-29 19:42:17]

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_impl.cc:174: if
(frame_holder_)
just checking, because the DemandDrawHw is synchronous here, doing so with the
in process compositor has no performance benefit right? This is done for the
sake of consistency with CompositorProxy implementation.

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_impl.cc:177: return
std::move(frame_holder_);
How is this is different from frame_holder_.pass()? No difference?

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_proxy.cc:200: inside_receive_ =
true;
Even though SwapBuffersHw will set inside_receive_ to false, you can still use
AutoReset right?

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_proxy.cc:211:
Send(reply_message);
I have a question about the case where the swap didn't happen:
in the previous code, the frame_ptr returned from output surface DemandDrawHw
will be null. We will at least populate the common renderer params for the
reply.

But the new code does not do so: it just send a completely empty reply. Is this
wrong?

[boliu, 2015-12-29 20:22:52]

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_impl.cc:174: if
(frame_holder_)
On 2015/12/29 19:42:17, hush wrote:
> just checking, because the DemandDrawHw is synchronous here, doing so with the
> in process compositor has no performance benefit right? This is done for the
> sake of consistency with CompositorProxy implementation.

Yep. SynchronousCompositorOutputSurface implementation is shared between
in-process and ipc paths.

https://codereview.chromium.org/1546383002/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_impl.cc:177: return
std::move(frame_holder_);
On 2015/12/29 19:42:17, hush wrote:
> How is this is different from frame_holder_.pass()? No difference?

No difference afaik. Presumit check says std::move is the way to go now.

Note that for scoped_ptr (and std::unique_ptr), the moved-from value after move
is defined to be null (just like Pass), so no need to null it out afterwards.
This is not true for types in general; apparently specs says the moved-from
value becomes undefined, and only safe operations on it after move are
assignment and destruction.

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_proxy.cc:200: inside_receive_ =
true;
On 2015/12/29 19:42:17, hush wrote:
> Even though SwapBuffersHw will set inside_receive_ to false, you can still use
> AutoReset right?

Yes technically. But I don't want to be changing a value being held by
AutoReset. Seems not very idiomatic..

Guess what I really want is DCHECK the value is false by the end of the
function, but that seems a bit overkill

https://codereview.chromium.org/1546383002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_proxy.cc:211:
Send(reply_message);
On 2015/12/29 19:42:17, hush wrote:
> I have a question about the case where the swap didn't happen:
> in the previous code, the frame_ptr returned from output surface DemandDrawHw
> will be null. We will at least populate the common renderer params for the
> reply.
> 
> But the new code does not do so: it just send a completely empty reply. Is
this
> wrong?

Good question. Yes this is wrong. If you look at
SynchronousCompositorHost::DemandDrawHw, ProcessCommonParams is called
regardless of the state of the compositor frame.

Turns out if you don't write the all of the params and send the reply, the Send
on the browser side returns false, which is also wrong. So need to write the
common renderer params, and also an empty frame back.

[hush (inactive), 2015-12-29 21:10:28]

https://codereview.chromium.org/1546383002/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (left):

https://codereview.chromium.org/1546383002/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:202: DeliverMessages();
When swap happened and output surface DemandDrawHw returns a real frame, Sync
compositor proxy will immediately send a bunch of IPC messages, drain the
messages from output surface's frame_swap_message_queue_. (don't know what it is
though)

The new code should do that too, when swap_hw happens.

[boliu, 2015-12-29 21:36:39]

https://codereview.chromium.org/1546383002/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (left):

https://codereview.chromium.org/1546383002/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:202: DeliverMessages();
On 2015/12/29 21:10:28, hush wrote:
> When swap happened and output surface DemandDrawHw returns a real frame, Sync
> compositor proxy will immediately send a bunch of IPC messages, drain the
> messages from output surface's frame_swap_message_queue_. (don't know what it
is
> though)
> 
> The new code should do that too, when swap_hw happens.

Barr, yep. Put it at end of DemandDrawHw though.

[hush (inactive), 2015-12-29 21:52:37]

lgtm

[boliu, 2015-12-29 22:11:38]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-12-29 22:11:54]

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

[commit-bot: I haz the power, 2015-12-29 22:46:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-29 22:47:02]

Description was changed from

==========
sync compositor: Send ipc reply in swap

Refactor SynchronousCompositorOutputSurface to expose the SwapBuffers
call to client. For IPC implementation, send the reply IPC inside
SwapBuffers instead of waiting for OnDraw to return.

This allows more code to run in parallel, since OnDraw generally calls
PrepareTiles after DrawAndSwap; now PrepareTiles can run in parallel
with IO/UI code. Depending on scheduling and available cores, this can
save up to ~0.7ms per frame of waiting time on UI thread.

BUG=545633
==========

to

==========
sync compositor: Send ipc reply in swap

Refactor SynchronousCompositorOutputSurface to expose the SwapBuffers
call to client. For IPC implementation, send the reply IPC inside
SwapBuffers instead of waiting for OnDraw to return.

This allows more code to run in parallel, since OnDraw generally calls
PrepareTiles after DrawAndSwap; now PrepareTiles can run in parallel
with IO/UI code. Depending on scheduling and available cores, this can
save up to ~0.7ms per frame of waiting time on UI thread.

BUG=545633

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

[commit-bot: I haz the power, 2015-12-29 22:47:03]

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