android: Change OutputSurfaceWithoutParent max swaps to 1.

android: Change OutputSurfaceWithoutParent max swaps to 1.

BUG=493854, 537335
Committed: https://crrev.com/ebd2c9a67a1b3cf397055013db1a3e7cc186e19e
Cr-Commit-Position: refs/heads/master@{#351836}

[brianderson, 2015-09-30 00:08:08]

brianderson@chromium.org changed reviewers:
+ miletus@chromium.org, skyostil@chromium.org

[brianderson, 2015-09-30 00:08:09]

I sent this to the perf trybots:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-09...

It improves latency without hurting throughput for that particular pageset and
device. I'd like to push this more widely and see what the effects are.

What do ya'll think?

[Sami, 2015-09-30 13:07:28]

On 2015/09/30 00:08:09, brianderson wrote:
> I sent this to the perf trybots:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-09...
> 
> It improves latency without hurting throughput for that particular pageset and
> device. I'd like to push this more widely and see what the effects are.
> 
> What do ya'll think?

Which device was that on? I wonder if this might end up hurting throughput on
low end devices more than on high end. In the test run it looks like
first_gesture_scroll_update_latency went down but mean_input_event_latency
regressed a bit. 

Still, I don't see harm in experimenting more widely so lgtm.

[Sami, 2015-09-30 13:08:07]

By the way do we have any UMA or tracing for when we are swap throttled here?
Should we?

[brianderson, 2015-09-30 19:40:08]

On 2015/09/30 13:07:28, Sami wrote:
> On 2015/09/30 00:08:09, brianderson wrote:
> > I sent this to the perf trybots:
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-09...
> > 
> > It improves latency without hurting throughput for that particular pageset
and
> > device. I'd like to push this more widely and see what the effects are.
> > 
> > What do ya'll think?
> 
> Which device was that on? I wonder if this might end up hurting throughput on
> low end devices more than on high end. In the test run it looks like
> first_gesture_scroll_update_latency went down but mean_input_event_latency
> regressed a bit. 

The test run has the results in the wrong order, so it's a bit confusing, but
mean_input_event_latency is faster for "Patch" than "ToT".

> 
> Still, I don't see harm in experimenting more widely so lgtm.

Yeah - I think the only way to tell for sure is by experimenting more widely.

Some theoretical reasoning for using 1 frames pending though:
1) The DisplayScheduler attempts to "catch up" when it is no longer swap
throttled and should be immune to odd frames here and there that take more than
1 vsync.
2) With Surfaces, the ui updates have an extra level of indirection now and
cc::Display returns the swap ack to the ui before it swaps itself. This should
reduce the amount of pipelining we would expect to lose had we done this before
Surfaces was enabled on Android.
3) We are already getting the swap ack back from the GPU service before the
actual display vsync.

>
> By the way do we have any UMA or tracing for when we are swap throttled here?
Should we?

There is tracing in the DisplayScheduler for when it is swap throttled.

[brianderson, 2015-09-30 19:40:10]

On 2015/09/30 13:07:28, Sami wrote:
> On 2015/09/30 00:08:09, brianderson wrote:
> > I sent this to the perf trybots:
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-09...
> > 
> > It improves latency without hurting throughput for that particular pageset
and
> > device. I'd like to push this more widely and see what the effects are.
> > 
> > What do ya'll think?
> 
> Which device was that on? I wonder if this might end up hurting throughput on
> low end devices more than on high end. In the test run it looks like
> first_gesture_scroll_update_latency went down but mean_input_event_latency
> regressed a bit. 

The test run has the results in the wrong order, so it's a bit confusing, but
mean_input_event_latency is faster for "Patch" than "ToT".

> 
> Still, I don't see harm in experimenting more widely so lgtm.

Yeah - I think the only way to tell for sure is by experimenting more widely.

Some theoretical reasoning for using 1 frames pending though:
1) The DisplayScheduler attempts to "catch up" when it is no longer swap
throttled and should be immune to odd frames here and there that take more than
1 vsync.
2) With Surfaces, the ui updates have an extra level of indirection now and
cc::Display returns the swap ack to the ui before it swaps itself. This should
reduce the amount of pipelining we would expect to lose had we done this before
Surfaces was enabled on Android.
3) We are already getting the swap ack back from the GPU service before the
actual display vsync.

>
> By the way do we have any UMA or tracing for when we are swap throttled here?
Should we?

There is tracing in the DisplayScheduler for when it is swap throttled.

[brianderson, 2015-09-30 19:41:59]

brianderson@chromium.org changed reviewers:
+ sievers@chromium.org

[brianderson, 2015-09-30 19:42:00]

+sievers for owners review.

I will track results on the perf dashboard and revert if it looks like this is
an overall loss.

[no sievers, 2015-09-30 22:20:26]

lgtm

will the behavior be the same with cc scheduler for being swap limited, which
currently (as hardcoded in compositor_impl_android.cc) is that when we get the
ack we immediately post a composite?

https://codereview.chromium.org/1262163004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/1262163004/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:88:
capabilities_.max_frames_pending = 1;
nit: kMaxSwapBuffers?

[brianderson, 2015-10-01 00:20:12]

> will the behavior be the same with cc scheduler for being swap limited, which
currently (as hardcoded in compositor_impl_android.cc) is that when we get the
ack we immediately post a composite?

Yep.

https://chromiumcodereview.appspot.com/1262163004/diff/20001/content/browser/...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/1262163004/diff/20001/content/browser/...
content/browser/renderer_host/compositor_impl_android.cc:88:
capabilities_.max_frames_pending = 1;
On 2015/09/30 22:20:25, sievers wrote:
> nit: kMaxSwapBuffers?

This refers to a different setting that will be used by cc::Display. I've added
a new kConstant for that in the latest patch.

[tdresser, 2015-10-01 12:25:58]

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

[tdresser, 2015-10-01 12:25:58]

I think an UMA metric to measure how often we're swap throttled would be a good
plan.

The bug is here: crbug.com/525756.

[brianderson, 2015-10-01 17:25:56]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-10-01 17:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, sievers@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1262163004/#ps40001
(title: "Use kConstants")

[commit-bot: I haz the power, 2015-10-01 17:26:10]

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

[commit-bot: I haz the power, 2015-10-01 17:40:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-01 17:41:02]

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