sync compositor: Memory policy async IPC

sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977
Committed: https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7
Cr-Commit-Position: refs/heads/master@{#393367}

[boliu, 2016-05-11 19:03:40]

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

[boliu, 2016-05-11 19:03:40]

PTAL

next easy one to pick off

dcheng for ipc
hush for the rest

[boliu, 2016-05-11 19:04:17]

Description was changed from

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an async IPC.

BUG=609977
==========

to

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an async IPC. Note need to remove
caching the bytes limit on the browser side due to correctly
handle OutputSurface getting lost.

BUG=609977
==========

[dcheng, 2016-05-11 20:15:49]

lgtm

[boliu, 2016-05-11 22:56:58]

sorry, hold off review for this a bit

In writing the next one, I realized this "just send an IPC every time foo is
called" isn't going to work. The renderer side might change due to output
surface getting lost and recreated. It's fine for this particular case because
SetMemoryPolicy is called every time before draw. But there are other cases
there is no frequent calls like that, and still need to handle output surface
recreate correctly.

Thinking of sending a renderer -> browser IPC that says "I'm a new
OutputSurface, I have no memory of previous browser state, please send
everything again"

[boliu, 2016-05-12 01:27:18]

Description was changed from

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an async IPC. Note need to remove
caching the bytes limit on the browser side due to correctly
handle OutputSurface getting lost.

BUG=609977
==========

to

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977
==========

[boliu, 2016-05-12 01:28:26]

ok, ptal at PS2

dcheng: there is a new IPC, so should be reviewed again

[dcheng, 2016-05-12 03:13:18]

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:428: uint32_t
output_surface_id) {
Doesn't appear to be used yet. How will it be used? What are the implications of
forging this ID?

[boliu, 2016-05-12 03:27:32]

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:428: uint32_t
output_surface_id) {
On 2016/05/12 03:13:18, dcheng wrote:
> Doesn't appear to be used yet. How will it be used? What are the implications
of
> forging this ID?

It's the same ID in the reply of SyncCompositorMsg_DemandDrawHw. The ID is
opaque on browser side, and sent back to the renderer in
SyncCompositorMsg_ReclaimResources so that resources don't get returned to the
wrong output surface.

In theory, it could be used as key to maps in the browser process. But that
generally shouldn't (and doesn't) happen, as there is only one "current" output
surface and any previous output surface is considered lost, and it's ok to just
drop resources for lost output surface. (There is no ordering issues like zombie
output surface coming back to life.)


Taking a step back, I don't foresee using this id in this method. Only thing I
can think of is saving ReclaimResources IPCs by dropping resources on the
browser side. Doubt that could be a perf issue especially during lost output
surface. So I can just remove this id if you prefer that?

[boliu, 2016-05-12 14:03:27]

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:428: uint32_t
output_surface_id) {
On 2016/05/12 03:27:32, boliu wrote:
> On 2016/05/12 03:13:18, dcheng wrote:
> > Doesn't appear to be used yet. How will it be used? What are the
implications
> of
> > forging this ID?
> 
> It's the same ID in the reply of SyncCompositorMsg_DemandDrawHw. The ID is
> opaque on browser side, and sent back to the renderer in
> SyncCompositorMsg_ReclaimResources so that resources don't get returned to the
> wrong output surface.
> 
> In theory, it could be used as key to maps in the browser process. But that
> generally shouldn't (and doesn't) happen, as there is only one "current"
output
> surface and any previous output surface is considered lost, and it's ok to
just
> drop resources for lost output surface. (There is no ordering issues like
zombie
> output surface coming back to life.)
> 
> 
> Taking a step back, I don't foresee using this id in this method. Only thing I
> can think of is saving ReclaimResources IPCs by dropping resources on the
> browser side. Doubt that could be a perf issue especially during lost output
> surface. So I can just remove this id if you prefer that?

Dropped in PS3

[dcheng, 2016-05-12 16:44:30]

lgtm

[hush (inactive), 2016-05-12 19:23:03]

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:318: void
SynchronousCompositorHost::SetMemoryPolicy(size_t bytes_limit) {
again, for multiprocess cross-bit (32bit renderer + 64bit browser) , we need
fixed bit types across IPC. So size_t needs to be uint32_t here.
I'm not sure clang plugins work, but this should have been caught by clang
plugins introduced by this CL?
https://codereview.chromium.org/1810243002/
Or maybe we're just not running these plugins?

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:431: new
SyncCompositorMsg_SetMemoryPolicy(routing_id_, bytes_limit_));
so... memory policy is *all* the browser side states? What about the states in
PopulateCommonParams?

https://codereview.chromium.org/1970863002/diff/40001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1970863002/diff/40001/content/common/android/...
content/common/android/sync_compositor_messages.h:73: unsigned int version;
now that we're here, let's change unsigned int to uint32_t. This should work for
cross-bit in multi process.

https://codereview.chromium.org/1970863002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1970863002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:293: bool
SynchronousCompositorOutputSurface::Send(IPC::Message* message) {
why do we need to create a function out of it?
why not just call sender_->Send(message)? There is only one place in this class
that calls it

[boliu, 2016-05-12 20:16:47]

uhh... ignoring all of your comments

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:318: void
SynchronousCompositorHost::SetMemoryPolicy(size_t bytes_limit) {
On 2016/05/12 19:23:03, hush wrote:
> again, for multiprocess cross-bit (32bit renderer + 64bit browser) , we need
> fixed bit types across IPC. So size_t needs to be uint32_t here.
> I'm not sure clang plugins work, but this should have been caught by clang
> plugins introduced by this CL?
> https://codereview.chromium.org/1810243002/
> Or maybe we're just not running these plugins?

As discussed over lunch. The wire format is uint32_t, so this is not an issue in
cross-bit. I agree on principle that implicit casting is still bad, but in
practice it's fine since the number won't exceed uint32, and fixing both sides
is way out of scope for this CL

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:431: new
SyncCompositorMsg_SetMemoryPolicy(routing_id_, bytes_limit_));
On 2016/05/12 19:23:03, hush wrote:
> so... memory policy is *all* the browser side states? What about the states in
> PopulateCommonParams?

It's not everything. I'm moving things to async IPCs, and out of
SynchronousCompositorProxy one at a time. If you go by state that's in
OutputSurface but *not* in Proxy, then yes, bytes_limit_ is the only one right
now.

https://codereview.chromium.org/1970863002/diff/40001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1970863002/diff/40001/content/common/android/...
content/common/android/sync_compositor_messages.h:73: unsigned int version;
On 2016/05/12 19:23:03, hush wrote:
> now that we're here, let's change unsigned int to uint32_t. This should work
for
> cross-bit in multi process.

again, int is fine for cross-bit on all hardware platforms we support

also out of scope for this CL

https://codereview.chromium.org/1970863002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1970863002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:293: bool
SynchronousCompositorOutputSurface::Send(IPC::Message* message) {
On 2016/05/12 19:23:03, hush wrote:
> why do we need to create a function out of it?
> why not just call sender_->Send(message)? There is only one place in this
class
> that calls it

Having a Send method is common pattern in classes that send and receive IPC. Eg
the IPC_MESSAGE_HANDLER assumes there's a Send method to call to send the reply.

[hush (inactive), 2016-05-12 20:47:23]

lgtm

[boliu, 2016-05-12 21:00:20]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-05-12 21:00:36]

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

[boliu, 2016-05-12 21:12:47]

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1970863002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:431: new
SyncCompositorMsg_SetMemoryPolicy(routing_id_, bytes_limit_));
On 2016/05/12 20:16:45, boliu wrote:
> On 2016/05/12 19:23:03, hush wrote:
> > so... memory policy is *all* the browser side states? What about the states
in
> > PopulateCommonParams?
> 
> It's not everything. I'm moving things to async IPCs, and out of
> SynchronousCompositorProxy one at a time. If you go by state that's in
> OutputSurface but *not* in Proxy, then yes, bytes_limit_ is the only one right
> now.

Actually... this *is* everything for OutputSurface.

If you look at the remaining state in SyncCompositorCommonBrowserParams, scroll
goes to input handler, and begin_frame_source_paused goes to BeginFrameSource.
And that's it.

[commit-bot: I haz the power, 2016-05-12 21:48:01]

Description was changed from

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977
==========

to

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977
==========

[commit-bot: I haz the power, 2016-05-12 21:48:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-12 21:49:40]

Description was changed from

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977
==========

to

==========
sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG=609977

Committed: https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7
Cr-Commit-Position: refs/heads/master@{#393367}
==========

[commit-bot: I haz the power, 2016-05-12 21:49:42]

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