cc: Set ui/gpu/cc/ipc/upload/raster thread priorities.

cc: Set ui/gpu/cc/ipc/upload/raster thread priorities.

This bumps up the ui/gpu/cc/ipc threads and bumps down the
upload/raster threads. The webkit thread is left alone, as
it needs to by dynamically adjusted (in another patch).

To reduce risk and for easier merging we do this as #ifdefs
and only for Android. This bug tracks abstracting the code
and doing it on all platforms: http://crbug.com/170549

BUG=164651
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178473

[epenner, 2013-01-14 01:49:05]

Ptal.

Two problems remain here:

1.) Content/common can't be #included from cc and ui/gl. Honestly I don't know
what package this should go in then. Any ideas where it could go?

2.) I've seen the best results by making it completely black and white who gets
priority. For example:
GPU/CC/browser = -6
Webkit/Upload/Raster = 10

Since GPU/browser/CC should be idle when not scrolling this might work all the
time. But likely there will be pushback against keeping webkit so low.  So it
looks like we need to dynamically adjust webkit's priority.

[epenner, 2013-01-14 04:02:30]

On 2013/01/14 01:49:05, epenner wrote:
> Ptal.
> 
> Two problems remain here:
> 
> 1.) Content/common can't be #included from cc and ui/gl. Honestly I don't know
> what package this should go in then. Any ideas where it could go?
> 
> 2.) I've seen the best results by making it completely black and white who
gets
> priority. For example:
> GPU/CC/browser = -6
> Webkit/Upload/Raster = 10
> 
> Since GPU/browser/CC should be idle when not scrolling this might work all the
> time. But likely there will be pushback against keeping webkit so low.  So it
> looks like we need to dynamically adjust webkit's priority.

Given the results I'm getting, I think it will be best to have 3-4 priorities
instead of unique ones for each thread... Given that, and the layering violation
issue, I'm now thinking I should just add generic priorities straight into
base::Thread (eg. LOW/MEDIUM/HIGH). This way it won't leak Chrome details in the
base.

[epenner, 2013-01-14 04:07:40]

On 2013/01/14 04:02:30, epenner wrote:
> On 2013/01/14 01:49:05, epenner wrote:
> > Ptal.
> > 
> > Two problems remain here:
> > 
> > 1.) Content/common can't be #included from cc and ui/gl. Honestly I don't
know
> > what package this should go in then. Any ideas where it could go?
> > 
> > 2.) I've seen the best results by making it completely black and white who
> gets
> > priority. For example:
> > GPU/CC/browser = -6
> > Webkit/Upload/Raster = 10
> > 
> > Since GPU/browser/CC should be idle when not scrolling this might work all
the
> > time. But likely there will be pushback against keeping webkit so low.  So
it
> > looks like we need to dynamically adjust webkit's priority.
> 
> Given the results I'm getting, I think it will be best to have 3-4 priorities
> instead of unique ones for each thread... Given that, and the layering
violation
> issue, I'm now thinking I should just add generic priorities straight into
> base::Thread (eg. LOW/MEDIUM/HIGH). This way it won't leak Chrome details in
the
> base.

Speak of the devil, there is already something in PlatformThread.

[nduca, 2013-01-14 22:48:40]

+reveman

[nduca, 2013-01-14 23:33:22]

Eric, what about making it bit less "thread priority setting" and a bit more
"prefer_smoothness"-ish?

E.g.

start by adding a didPreferSmoothnessChange(bool preferSmoothness) call to
LTHIClient. Plumb that back to main thread on beginFrame message. Propagate it
out to the embedder, which is hopefully render_widget (depending on how far
along jamesr gets with un-inverting the render_widget).

For GPU process, I'm curious: does the gpu thread ever need to change? Or is it
just the upload thread? I think if its just the upload thread, then we should
pass urgency down on the async upload call. If its the gpu thread needing dynmic
priority, then making a command buffer method would be appropriate.

[epenner, 2013-01-15 01:34:19]

To summarize offline discussion, it looks like we can get away with keeping
GPU/Browser/CC/IPC always be 'urgent' and hopefully they never need to change.

Then webkit/raster/upload threads can fight for the remaining CPU using the
smoothness preference to determine their relative priority.

During load is the most complex case. We want to optimize load time while
ideally still not causing jank. We might even want to allow jank for webkit
driven frames.

[epenner, 2013-01-17 20:43:07]

Ptal.

We get even better performance by lowering webkit to the be
same as raster/upload but I need more verification that it
doesn't impact load time and webkit benchmarks. If so dynamic priority
adjustments might be needed.

[brianderson, 2013-01-18 00:53:22]

In the second patch, how did you avoid having to reset the thread priority of
new threads to 0 in base/threading/platform_thread_posix.cc?

[nduca, 2013-01-18 00:58:08]

better changelist summary plz

better changelist description. say what happens exactly.

explaiin why we do it with #ifs instead of blbalah in the commit descripiton.

https://codereview.chromium.org/11880014/diff/8001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_thread.cc (right):

https://codereview.chromium.org/11880014/diff/8001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_thread.cc:107: base::Unretained(this)));
make a method like InitOnThread() and always post it.

do the setup inside.

[epenner, 2013-01-18 00:59:08]

On 2013/01/18 00:53:22, Brian Anderson wrote:
> In the second patch, how did you avoid having to reset the thread priority of
> new threads to 0 in base/threading/platform_thread_posix.cc?

Hi Brian, I read the description and from what I could tell priority inheritance
only occurs when you fork a process? I don' think clank originally reset it so I
figured it might not be needed.

[epenner, 2013-01-18 01:01:09]

On 2013/01/18 00:59:08, epenner wrote:
> On 2013/01/18 00:53:22, Brian Anderson wrote:
> > In the second patch, how did you avoid having to reset the thread priority
of
> > new threads to 0 in base/threading/platform_thread_posix.cc?
> 
> Hi Brian, I read the description and from what I could tell priority
inheritance
> only occurs when you fork a process? I don' think clank originally reset it so
I
> figured it might not be needed.

If you know for sure it needs to be done, let me know and I'll do it. Otherwise
we can add a follow up to fix it after investigating?

[epenner, 2013-01-18 04:12:20]

I've added a default priority in platform_thread_posix, and done the minor init
refactor. Ptal.

I'm going to test both priority patches together to see how it works.

[nduca, 2013-01-18 04:59:47]

mostly lgtm. You should run by piman maybe, since he reviewed the other part of
this.

https://codereview.chromium.org/11880014/diff/15001/content/common/child_proc...
File content/common/child_process.cc (right):

https://codereview.chromium.org/11880014/diff/15001/content/common/child_proc...
content/common/child_process.cc:2: // Use of this source code is governed by a
BSD-style license that can be
this one is intending to influence the renderer main therad, right? isn't there
a render_thread_impl or something that we can use instead?

[epenner, 2013-01-18 05:18:41]

https://codereview.chromium.org/11880014/diff/15001/content/common/child_proc...
File content/common/child_process.cc (right):

https://codereview.chromium.org/11880014/diff/15001/content/common/child_proc...
content/common/child_process.cc:2: // Use of this source code is governed by a
BSD-style license that can be
On 2013/01/18 04:59:47, nduca wrote:
> this one is intending to influence the renderer main therad, right? isn't
there
> a render_thread_impl or something that we can use instead?

This is for a child process IPC thread (io_thread_). The browser IPC thread is
in another file.

[commit-bot: I haz the power, 2013-01-19 01:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11880014/19003

[commit-bot: I haz the power, 2013-01-19 01:37:32]

Presubmit check for 11880014-19003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer/gpu
    ui/gl
    base/threading
    content/gpu
    content/browser
    content/common

Presubmit checks took 1.5s to calculate.

[epenner, 2013-01-19 01:38:53]

Landing. This was apparently already merged so any further changes should be in
a follow up CL.

I've tested it and verified all the right priorities are set "ps -t -p".

[epenner, 2013-01-19 01:59:39]

This patch needs all sorts of OWNER reviews. 

content/renderer/gpu -> apatrick/piman
ui/gl -> apatrick
base/threading -> jar
content -> piman
content/gpu -> apatrick
content/browser -> sky
content/common -> piman

[piman, 2013-01-19 02:25:00]

+darin who may have opinions about thread priorities. I know we've resisted them
in the past except for very specific cases, and I think it's worth a more
general discussion.

If we're convinced we need to fine-tune thread priorities, I think one of the
first things to do is to pass it as an option to Thread::StartWithOptions, and
define them as enums, like PRIORITY_DEFAULT, _HIGH, _IDLE, that can be
interpreted appropriately on each platform.

Meta-comment: if this has already landed on the branch, does it make sense to
wait until crbug.com/170549 is fixed so that it's done more right?

[epenner, 2013-01-19 02:42:38]

On 2013/01/19 02:25:00, piman wrote:
> +darin who may have opinions about thread priorities. I know we've resisted
them
> in the past except for very specific cases, and I think it's worth a more
> general discussion.
> 
> If we're convinced we need to fine-tune thread priorities, I think one of the
> first things to do is to pass it as an option to Thread::StartWithOptions, and
> define them as enums, like PRIORITY_DEFAULT, _HIGH, _IDLE, that can be
> interpreted appropriately on each platform.
> 

I totally agree on the enum for priorities, and there is actually already such
an enum for audio/normal. We just need to make it work for Linux/Android in
base. I started off doing this in base but there was no reviewer bandwidth so
reviewers were removed from the patch:
https://codereview.chromium.org/11880014/
There is also a discussion here on an alternative to my patch:
http://code.google.com/p/chromium/issues/detail?id=116172

I also agree priority setting should only be done as a last resort, and that it
can be counter intuitive to do it right. However, with impl-side painting we
have created 3 new threads, two of which are background worker threads that are
always busy. We want those threads to be busy, but not at the expense of
responsiveness on our critical rendering threads. Before this patch, our
rendering threads would get descheduled for 5-10ms at a time and result in
dropped frames.

The priorities used are based on the priorities Android uses (SurfaceFlinger is
-7, and java async task threads are 10).

> Meta-comment: if this has already landed on the branch, does it make sense to
> wait until crbug.com/170549 is fixed so that it's done more right?

I think this particular branch will last longer than usual, so I believe there
is a desire to keep it in sync.

[nduca, 2013-01-22 20:26:42]

We aren't landing onto branch directly. Ås near as I can tell, the right thing
to do is land this patch as proposed, then pursue 170549 as soon as we get to 26
activities.

Lets talk offline if you are not feeling comfortable with the general strategy
of thread priority setting.

[piman, 2013-01-22 21:14:58]

Talked to Nat, I'm going to LGTM this for now, counting on the cleanup
afterwards. You still need a ui/gl and a base/ owner.

[apatrick_chromium, 2013-01-22 21:49:17]

ui/gl LGTM.

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:27: #if defined(OS_ANDROID)
You don't need the ifdef here right? This is an Android specific file.

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:106: #if defined(OS_ANDROID)
or here?

[epenner, 2013-01-23 00:28:45]

Addressed feedback.

Just waiting on base OWNER review now.

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:27: #if defined(OS_ANDROID)
On 2013/01/22 21:49:18, apatrick_chromium wrote:
> You don't need the ifdef here right? This is an Android specific file.

Thanks. I just left the comment and removed #ifdef.

Done.

https://codereview.chromium.org/11880014/diff/19003/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:106: #if defined(OS_ANDROID)
On 2013/01/22 21:49:18, apatrick_chromium wrote:
> or here?

Done.

[jar (doing other things), 2013-01-23 02:37:37]

Patch set 5, base (impacting only Android), LGTM... but please consider the nit
below.

https://codereview.chromium.org/11880014/diff/30001/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/11880014/diff/30001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:64: setpriority(PRIO_PROCESS,
base::PlatformThread::CurrentId(), 0);
nit: how about checking the return value, at least as is done in line 137 of
this file?

[commit-bot: I haz the power, 2013-01-23 20:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11880014/32016

[darin (slow to review), 2013-01-23 21:52:43]

https://codereview.chromium.org/11880014/diff/32016/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_thread.cc (right):

https://codereview.chromium.org/11880014/diff/32016/content/renderer/gpu/comp...
content/renderer/gpu/compositor_thread.cc:99:
base::Bind(&CompositorThread::InitOnThread,
nit: bind a static function since you don't need a |this| pointer.  then, you
won't need to pass an unretained this.

[commit-bot: I haz the power, 2013-01-24 00:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11880014/31011

[commit-bot: I haz the power, 2013-01-24 01:55:08]

Change committed as 178473