sync compositor: Reduce begin frame sync IPC overhead

sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235
Committed: https://crrev.com/3dd3ca72f1a2c3211177c72aae2d5027d8217769
Cr-Commit-Position: refs/heads/master@{#408528}

[boliu, 2016-07-19 14:40:14]

Description was changed from

==========
[WIP] make update state pc per process

compiles

synchronous_compositor_renderer_process_host_observer GetOrCreateFor
observe vsync
keep list of sync comp host
destroys itself
send control message

BUG=
==========

to

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.



compiles

synchronous_compositor_renderer_process_host_observer GetOrCreateFor
observe vsync
keep list of sync comp host
destroys itself
send control message

BUG=
==========

[boliu, 2016-07-19 14:41:43]

Description was changed from

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.



compiles

synchronous_compositor_renderer_process_host_observer GetOrCreateFor
observe vsync
keep list of sync comp host
destroys itself
send control message

BUG=
==========

to

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.



compiles

synchronous_compositor_renderer_process_host_observer GetOrCreateFor
observe vsync
keep list of sync comp host
destroys itself
send control message

BUG=596235
==========

[boliu, 2016-07-19 14:47:39]

Description was changed from

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.



compiles

synchronous_compositor_renderer_process_host_observer GetOrCreateFor
observe vsync
keep list of sync comp host
destroys itself
send control message

BUG=596235
==========

to

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235
==========

[boliu, 2016-07-20 21:21:43]

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

[boliu, 2016-07-20 21:21:43]

ptal

dcheng for ipc
sievers for the rest

The good part, 1ms less spent waiting on UI thread, tested on nexus 5, in inbox
with 3 webviews drawing at the same time (each message in inbox is a separate
webview)

Potentially controversial parts:
* this kinda semi abuses the observer api in order to get a callback after "all
vsync observers have run", alternative is add an explicit callback
* this isn't actually one per renderer process, it's one per renderer process
and per WindowAndroid, and WindowAndroid isn't necessarily global in webview,
but close enough

And totally looking for a better name than SynchronousCompositorRPHObserver.
Ideals?

[dcheng, 2016-07-21 02:27:37]

https://codereview.chromium.org/2160743002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_rph_observer.cc (right):

https://codereview.chromium.org/2160743002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_rph_observer.cc:107:
DCHECK_EQ(compositor_host_pending_renderer_state_.size(), params.size());
This shouldn't be a DCHECK, since a malicious renderer can opt to not send back
the right amount of params.

[boliu, 2016-07-21 14:35:04]

https://codereview.chromium.org/2160743002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_rph_observer.cc (right):

https://codereview.chromium.org/2160743002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_rph_observer.cc:107:
DCHECK_EQ(compositor_host_pending_renderer_state_.size(), params.size());
On 2016/07/21 02:27:36, dcheng wrote:
> This shouldn't be a DCHECK, since a malicious renderer can opt to not send
back
> the right amount of params.

Turned this into a bad_message.

[dcheng, 2016-07-22 06:53:44]

ipc lgtm

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
File content/browser/android/synchronous_compositor_rph_observer.cc (right):

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_rph_observer.cc:115:
bad_message::SCA_INVALID_ARGUMENT);
I guess a //content reviewer can chime in but wow, there's a lot of
abbreviations on BadMessage...

[boliu, 2016-07-25 15:50:43]

sievers: ping

[no sievers, 2016-07-28 22:03:31]

lgtm w/nits

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
File content/browser/android/synchronous_compositor_rph_observer.cc (right):

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_rph_observer.cc:115:
bad_message::SCA_INVALID_ARGUMENT);
On 2016/07/22 06:53:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> I guess a //content reviewer can chime in but wow, there's a lot of
> abbreviations on BadMessage...

// The naming convention is abbreviated class
/ name (e.g. RenderFrameHost becomes RFH) plus a unique description of the	 
// reason.

So it should be SCRPHO.

The class name here should probably not have an abbreviation in itself either.
Maybe just SynchronousCompositorObserver? And SCO?

https://codereview.chromium.org/2160743002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/2160743002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:89:
SynchronizeRendererState)
nit: OnSynchronizeRendererState

[boliu, 2016-07-28 23:22:40]

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
File content/browser/android/synchronous_compositor_rph_observer.cc (right):

https://codereview.chromium.org/2160743002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_rph_observer.cc:115:
bad_message::SCA_INVALID_ARGUMENT);
On 2016/07/28 22:03:30, sievers wrote:
> On 2016/07/22 06:53:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> > I guess a //content reviewer can chime in but wow, there's a lot of
> > abbreviations on BadMessage...
> 
> // The naming convention is abbreviated class
> / name (e.g. RenderFrameHost becomes RFH) plus a unique description of the	 
> // reason.
> 
> So it should be SCRPHO.
> 
> The class name here should probably not have an abbreviation in itself either.
> Maybe just SynchronousCompositorObserver? And SCO?

both done. at this point, any name is better than the current one.

https://codereview.chromium.org/2160743002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/2160743002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:89:
SynchronizeRendererState)
On 2016/07/28 22:03:30, sievers wrote:
> nit: OnSynchronizeRendererState

Done.

[boliu, 2016-07-28 23:24:22]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-07-28 23:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2160743002/#ps140001
(title: "clang format")

[commit-bot: I haz the power, 2016-07-28 23:24:43]

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, 2016-07-29 00:18:55]

Description was changed from

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235
==========

to

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235
==========

[commit-bot: I haz the power, 2016-07-29 00:18:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-29 00:21:10]

Description was changed from

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235
==========

to

==========
sync compositor: Reduce begin frame sync IPC overhead

By making the begin frame sync IPC per renderer process rather than
per compositor.

Convert SyncCompositorMsg_SynchronizeRendererState
to a control message, with a list of routing_ids as input, and a list of
SyncCompositorCommonRendererParams as output.

Introduce SynchronousCompositorRPHObserver which is tied to the
lifetime of the RenderProcessHost, which is the browser side handler of
control messages. SynchronousCompositorFilter serves as the renderer
side control message handler.

SynchronousCompositorRPHObserver adds itself as a vsync observer
for the duration of a single vsync. Then its OnVSync callback is called
after SynchronousCompositorHosts, where the single synchronous
IPC is sent.

BUG=596235

Committed: https://crrev.com/3dd3ca72f1a2c3211177c72aae2d5027d8217769
Cr-Commit-Position: refs/heads/master@{#408528}
==========

[commit-bot: I haz the power, 2016-07-29 00:21:11]

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

[pfeldman, 2016-07-30 00:21:46]

you should have generated tools/metrics/histograms/histograms.xml

[boliu, 2016-07-30 00:23:18]

On 2016/07/30 00:21:46, pfeldman wrote:
> you should have generated tools/metrics/histograms/histograms.xml

Oops. Will do that now

[boliu, 2016-07-30 00:26:39]

On 2016/07/30 00:23:18, boliu wrote:
> On 2016/07/30 00:21:46, pfeldman wrote:
> > you should have generated tools/metrics/histograms/histograms.xml
> 
> Oops. Will do that now

https://codereview.chromium.org/2195983002/