CrOS: Set max frames pending for UI from command line.

CrOS: Set max frames pending for UI from command line.

This CL is a no-op, but with a CrOS change to /sbin/session_manager_setup.sh,
we can reduce the number of UI frames in flight.

BUG=181317, 189142
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188038

[jonathan.backer, 2013-03-06 16:10:14]

With the recent changes to vsync scheduling (all CrOS platforms and for renderer
and UI), I think that it is safe to reduce the number of pending frames in the
UI. This frees up time in the GPU process for renderers and browser tex upload
(less blocking on SwapBuffers and MakeCurrent).

Setting --ui-max-frames-pending=1 is
- practically a no-op on IVB and SNB (hardly ever more than one frame pending)
- helpful on Mali: open a tab with chrome:hang, open another tab with simple web
content and fling, see less jank
- a wash on pineview

I tested with --composite-to-mailbox --enable-compositor-frame-message as well.
I'll need a tweak in this case (separate CL). I sometimes see some renderer work
sneek in between a UI swap and echo, causing us to throttle UI drawing more than
necessary.

[piman, 2013-03-06 17:46:41]

On 2013/03/06 16:10:14, jonathan.backer wrote:
> With the recent changes to vsync scheduling (all CrOS platforms and for
renderer
> and UI), I think that it is safe to reduce the number of pending frames in the
> UI.

I would really like to maintain the behavior that we enqueue 2 frames if we are
gpu-bound, so that we don't starve the GPU. Maybe we need a different logic to
detect the case (in particular, channel scheduling adds a bunch of noise here),
but I want to avoid cases where the CPU and the GPU both are heavily loaded and
we lose parallelism.

When we have a UI compositor, we can, however, reduce the renderer to 1 frame,
assuming the UI will take care of enqueuing 2 frames if necessary.

> This frees up time in the GPU process for renderers and browser tex upload
> (less blocking on SwapBuffers and MakeCurrent).
> 
> Setting --ui-max-frames-pending=1 is
> - practically a no-op on IVB and SNB (hardly ever more than one frame pending)
> - helpful on Mali: open a tab with chrome:hang, open another tab with simple
web
> content and fling, see less jank
> - a wash on pineview
> 
> I tested with --composite-to-mailbox --enable-compositor-frame-message as
well.
> I'll need a tweak in this case (separate CL). I sometimes see some renderer
work
> sneek in between a UI swap and echo, causing us to throttle UI drawing more
than
> necessary.

[piman, 2013-03-06 17:47:04]

Also, Nat should be involved in the discussion

[hshi1, 2013-03-11 23:21:24]

On 2013/03/06 17:47:04, piman wrote:
> Also, Nat should be involved in the discussion

backer@ - what's the recommended cmdline setting for max frames pending? I'd
like to try a few numbers and see how they improve full-screen flash playback on
Link.

[hshi1, 2013-03-11 23:32:20]

Never mind, I just did a few experiments.

It appears that setting ui-max-frames-pending=1 makes the jank completely go
away.

On the other hand, increasing the number to 2, 3 or higher seems to degrade
performance.

On 2013/03/11 23:21:24, hshi1 wrote:
> On 2013/03/06 17:47:04, piman wrote:
> > Also, Nat should be involved in the discussion
> 
> backer@ - what's the recommended cmdline setting for max frames pending? I'd
> like to try a few numbers and see how they improve full-screen flash playback
on
> Link.

[piman, 2013-03-13 15:55:22]

I thought more about this CL. The logic for the GPU-bound detection is pretty
broken, and the drivers we have degrade badly. So this would be a strict
improvement for now, at least until we switch to the SwapCompositorFrame flow
(composite to mailbox or ubercomp) which fixes a lot of the problems with GPU
process being busy.

https://codereview.chromium.org/12540003/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/12540003/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_output_surface.cc:69:
capabilities_.max_frames_pending = 1;
This logic should stay inside the compositor. It's necessary (at least
currently) for correctness of the DelegatingRenderer, so I'd rather not rely on
the embedder to do the right thing (which may change in the future).

[jonathan.backer, 2013-03-13 16:55:28]

https://codereview.chromium.org/12540003/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/12540003/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_output_surface.cc:69:
capabilities_.max_frames_pending = 1;
On 2013/03/13 15:55:22, piman wrote:
> This logic should stay inside the compositor. It's necessary (at least
> currently) for correctness of the DelegatingRenderer, so I'd rather not rely
on
> the embedder to do the right thing (which may change in the future).

Done.

[piman, 2013-03-13 17:21:17]

lgtm

[DaveMoore, 2013-03-13 17:49:06]

LGTM Modulo nits

https://codereview.chromium.org/12540003/diff/9001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/12540003/diff/9001/cc/thread_proxy.cc#newcode...
cc/thread_proxy.cc:1043: int maxFramesPending =
m_layerTreeHostImpl->outputSurface()->capabilities().max_frames_pending;
Nit: Line length

https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:321: LOG(ERROR) <<
"Trouble parsing --" <<  switches::kUIMaxFramesPending;
Nit: extra space

https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_s...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_s...
ui/compositor/compositor_switches.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Nit:2013 (check all other files too)

[jonathan.backer, 2013-03-13 18:00:25]

https://codereview.chromium.org/12540003/diff/9001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/12540003/diff/9001/cc/thread_proxy.cc#newcode...
cc/thread_proxy.cc:1043: int maxFramesPending =
m_layerTreeHostImpl->outputSurface()->capabilities().max_frames_pending;
On 2013/03/13 17:49:06, DaveMoore wrote:
> Nit: Line length

cc is a mix of WK and Cr style (currently being scripted to match Cr style).
This style (> 80 characters with 4 char indent) matches this block.

https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:321: LOG(ERROR) <<
"Trouble parsing --" <<  switches::kUIMaxFramesPending;
On 2013/03/13 17:49:06, DaveMoore wrote:
> Nit: extra space

Done.

https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_s...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_s...
ui/compositor/compositor_switches.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/03/13 17:49:06, DaveMoore wrote:
> Nit:2013 (check all other files too)

My understanding is that we don't do that anymore:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

[piman, 2013-03-13 18:16:14]

On Wed, Mar 13, 2013 at 11:00 AM, <backer@chromium.org> wrote:

>
>
https://codereview.chromium.**org/12540003/diff/9001/cc/**thread_proxy.cc<htt...
> File cc/thread_proxy.cc (right):
>
> https://codereview.chromium.**org/12540003/diff/9001/cc/**
>
thread_proxy.cc#newcode1043<https://codereview.chromium.org/12540003/diff/9001/cc/thread_proxy.cc#newcode1043>
> cc/thread_proxy.cc:1043: int maxFramesPending =
> m_layerTreeHostImpl->**outputSurface()->capabilities(**
> ).max_frames_pending;
> On 2013/03/13 17:49:06, DaveMoore wrote:
>
>> Nit: Line length
>>
>
> cc is a mix of WK and Cr style (currently being scripted to match Cr
> style). This style (> 80 characters with 4 char indent) matches this
> block.


background: this file was moved from WebKit, people are working on
Chromifying the style, but the consensus is that until that's done we
should keep the local style, and the chromification is being done in bulk
in an automated fashion.


>
> https://codereview.chromium.**org/12540003/diff/9001/**
>
content/browser/renderer_host/**image_transport_factory.cc<https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_host/image_transport_factory.cc>
> File content/browser/renderer_host/**image_transport_factory.cc (right):
>
> https://codereview.chromium.**org/12540003/diff/9001/**
>
content/browser/renderer_host/**image_transport_factory.cc#**newcode321<https://codereview.chromium.org/12540003/diff/9001/content/browser/renderer_host/image_transport_factory.cc#newcode321>
> content/browser/renderer_host/**image_transport_factory.cc:**321:
> LOG(ERROR)
> << "Trouble parsing --" <<  switches::kUIMaxFramesPending;
> On 2013/03/13 17:49:06, DaveMoore wrote:
>
>> Nit: extra space
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12540003/diff/9001/ui/**
>
compositor/compositor_**switches.cc<https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_switches.cc>
> File ui/compositor/compositor_**switches.cc (right):
>
> https://codereview.chromium.**org/12540003/diff/9001/ui/**
>
compositor/compositor_**switches.cc#newcode1<https://codereview.chromium.org/12540003/diff/9001/ui/compositor/compositor_switches.cc#newcode1>
> ui/compositor/compositor_**switches.cc:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> On 2013/03/13 17:49:06, DaveMoore wrote:
>
>> Nit:2013 (check all other files too)
>>
>
> My understanding is that we don't do that anymore:
> https://groups.google.com/a/**chromium.org/forum/?**
>
fromgroups=#!topic/chromium-**dev/8p4JKV76kig<https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/8p4JKV76kig>
>
>
https://codereview.chromium.**org/12540003/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-13 19:36:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/12540003/9001

[commit-bot: I haz the power, 2013-03-13 19:36:26]

Failed to apply patch for cc/thread_proxy.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/thread_proxy.cc
  Hunk #1 FAILED at 1040.
  1 out of 1 hunk FAILED -- saving rejects to file cc/thread_proxy.cc.rej

Patch:       cc/thread_proxy.cc
Index: cc/thread_proxy.cc
diff --git a/cc/thread_proxy.cc b/cc/thread_proxy.cc
index
faa66957d219bde5449c7654b2d6cfd41c2fc149..602f7f9f90ddb75a87dacd0e57332003e54d40b9
100644
--- a/cc/thread_proxy.cc
+++ b/cc/thread_proxy.cc
@@ -1040,7 +1040,9 @@ void
ThreadProxy::initializeRendererOnImplThread(CompletionEvent* completion, bo
         m_schedulerOnImplThread->setSwapBuffersCompleteSupported(
                 capabilities->using_swap_complete_callback);
 
-        int maxFramesPending = FrameRateController::kDefaultMaxFramesPending;
+        int maxFramesPending =
m_layerTreeHostImpl->outputSurface()->capabilities().max_frames_pending;
+        if (maxFramesPending <= 0)
+            maxFramesPending = FrameRateController::kDefaultMaxFramesPending;
         if
(m_layerTreeHostImpl->outputSurface()->capabilities().has_parent_compositor)
             maxFramesPending = 1;
         m_schedulerOnImplThread->setMaxFramesPending(maxFramesPending);

[commit-bot: I haz the power, 2013-03-13 19:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/12540003/33001

[zel, 2013-03-13 19:55:41]

lgtm

[commit-bot: I haz the power, 2013-03-13 22:04:23]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-13 23:20:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/12540003/33001

[commit-bot: I haz the power, 2013-03-14 09:13:45]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[jonathan.backer, 2013-03-14 10:55:52]

Committed patchset #4 manually as r188038 (presubmit successful).