content: Improve thread priority for raster threads.

content: Improve thread priority for raster threads.

This reduces the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.

Threads used for foreground work are now given normal priority
on all platforms.

BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/56d6a40f9f84140e8a806325beec52bea3f3bd2a
Cr-Commit-Position: refs/heads/master@{#377490}

[reveman, 2016-01-25 00:39:00]

reveman@chromium.org changed reviewers:
+ ericrk@chromium.org, vmiura@chromium.org

[reveman, 2016-01-25 00:39:01]

I'd like to give this a try now that Eric's task category change has landed and
gpu raster is always running on the same thread and prepaint tasks can't block
tasks for visible contents. Wdyt?

[reveman, 2016-01-25 00:39:05]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 00:39:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1616953003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1616953003/1

[commit-bot: I haz the power, 2016-01-25 01:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-25 01:49:19]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-01-25 20:53:42]

On 2016/01/25 01:49:19, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Makes sense to give this a try - how will we know if this worked? telemetry?

[reveman, 2016-01-25 21:10:24]

On 2016/01/25 at 20:53:42, ericrk wrote:
> On 2016/01/25 01:49:19, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Makes sense to give this a try - how will we know if this worked? telemetry?

Yes, reducing this caused a number of sw raster performance regressions and they
should recover as a result of this change without causing a gpu raster
regression.

[ericrk, 2016-01-26 00:07:28]

lgtm

[vmiura, 2016-01-26 00:11:19]

lgtm, let's try.

[reveman, 2016-01-26 00:12:30]

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

[reveman, 2016-01-26 00:12:31]

+sievers for content/

[no sievers, 2016-01-26 01:51:53]

Description was changed from

==========
content: Allow multiple raster threads on Android.

This improves performance on devices with 4 cores.

BUG=504515
==========

to

==========
content: Allow multiple raster threads on Android.

This improves performance on devices with 4 cores.

BUG=504515
==========

[no sievers, 2016-01-26 01:51:53]

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

[no sievers, 2016-01-26 01:54:46]

+aelias who has some concerns that just using more threads will mainly increase
our power footprint.

How do people feel about two dedicated threads (foreground + background work
respectively) with setting thread niceness accordingly so that we are less
likely to spin up more big cores as a side effect of this patch>

[aelias_OOO_until_Jul13, 2016-01-26 02:31:06]

Yeah, in my mind "prepaint tasks can't block tasks for visible contents" wasn't
the only blocker to turning this on on Android.  As I've mentioned, I think 1)
any additional raster threads should have niceness 10 to guarantee little core
placement, and 2) we should only run eventually-bin tiles and image decodes on
those secondary raster thread(s).  I'd like us to block enabling the feature on
those changes; not lgtm for now.


Reasoning: because of thermal throttling, battery life, and Linux "fair"
scheduling that sometimes prioritizes raster over draws, we shouldn't think of
unused big cores as a spare resource to be utilized to the max to improve raster
performance, as we do on desktop.  On Android we should have the exact opposite
goal -- to use the big cores as sparingly as we can, ideally for
user-waiting-to-see-the-result work only.  So the main appeal of multiple raster
threads on Android is to enable us to run *less* raster/decode work on a big
core, and I perceive any change where we use more of the big cores as a
regression.

[reveman, 2016-01-26 02:38:33]

On 2016/01/26 at 01:54:46, sievers wrote:
> +aelias who has some concerns that just using more threads will mainly
increase our power footprint.
> 
> How do people feel about two dedicated threads (foreground + background work
respectively) with setting thread niceness accordingly so that we are less
likely to spin up more big cores as a side effect of this patch>

Note that the current patch will not increase the number of threads used for pre
paint work. Pre paint will run on one thread on all platforms. Only visible high
priority tasks can use multiple threads.

I'm planning to upload another patch that adjusts the thread priority based on
the priority of the currently running task but I'd prefer to do that as a
separate patch is possible.

[aelias_OOO_until_Jul13, 2016-01-26 03:12:03]

On 2016/01/26 at 02:38:33, reveman wrote:
> On 2016/01/26 at 01:54:46, sievers wrote:
> > +aelias who has some concerns that just using more threads will mainly
increase our power footprint.
> > 
> > How do people feel about two dedicated threads (foreground + background work
respectively) with setting thread niceness accordingly so that we are less
likely to spin up more big cores as a side effect of this patch>
> 
> Note that the current patch will not increase the number of threads used for
pre paint work. Pre paint will run on one thread on all platforms. Only visible
high priority tasks can use multiple threads.

If I understand correctly, that still leaves prepaint using one big core and
high priority tasks running on another big core, so in practice it sounds like
this will allow prepaint to harm scroll framerate still.

More fundamentally I see "high priority tasks can use multiple threads" as
basically a nongoal and not worth the threat to other performance/efficiency
measures.  Our raster pipeline has always liked to expand to hog as many
resources as possible, and I see the single raster thread as a natural barrier
that's been useful to maintain sanity.  Now, you might argue that there are some
scenarios (say, a raster-bound <canvas> game) where the user does benefit from
max throughput, and that it's possible in theory to restrict the multi-threading
to those particular scenarios, and not harm scrolling framerate and power usage
otherwise.  But our story for those scenarios is Ganesh, and furthermore our
focus on Ganesh means we won't necessarily devote enough attention to the
software path to avoid changes that might make it hog multiple big cores in the
future.

[reveman, 2016-01-26 16:50:42]

On 2016/01/26 at 03:12:03, aelias wrote:
> On 2016/01/26 at 02:38:33, reveman wrote:
> > On 2016/01/26 at 01:54:46, sievers wrote:
> > > +aelias who has some concerns that just using more threads will mainly
increase our power footprint.
> > > 
> > > How do people feel about two dedicated threads (foreground + background
work respectively) with setting thread niceness accordingly so that we are less
likely to spin up more big cores as a side effect of this patch>
> > 
> > Note that the current patch will not increase the number of threads used for
pre paint work. Pre paint will run on one thread on all platforms. Only visible
high priority tasks can use multiple threads.
> 
> If I understand correctly, that still leaves prepaint using one big core and
high priority tasks running on another big core, so in practice it sounds like
this will allow prepaint to harm scroll framerate still.
> 
> More fundamentally I see "high priority tasks can use multiple threads" as
basically a nongoal and not worth the threat to other performance/efficiency
measures.  Our raster pipeline has always liked to expand to hog as many
resources as possible, and I see the single raster thread as a natural barrier
that's been useful to maintain sanity.  Now, you might argue that there are some
scenarios (say, a raster-bound <canvas> game) where the user does benefit from
max throughput, and that it's possible in theory to restrict the multi-threading
to those particular scenarios, and not harm scrolling framerate and power usage
otherwise.  But our story for those scenarios is Ganesh, and furthermore our
focus on Ganesh means we won't necessarily devote enough attention to the
software path to avoid changes that might make it hog multiple big cores in the
future.

The problem with one raster thread is that low-priority pre-paint (e.g. a large
image decode) can block high priority visible content from being generated. I
was hoping we could improve that.

Btw, I don't think raster will ever use a big core today. We use background
priority for worker threads on Android today and if understand correctly how
Android schedules work on big.LITTLE devices, this use of background priority
results in raster work (including Ganesh) never running on big cores.

I changed this patch to instead introduce a dedicated background thread on all
platforms with background priority. --num-raster-threads is now instead
controlling the number of threads that can run foreground work and it's set to 1
by default on Android. I don't know if this is what we want. Take it as an
example of how we might want to adjust the worker pool to better take advantage
of big.LITTLE architecture and avoid having low priority work block high
priority visible content from being generated.

[vmiura, 2016-01-26 17:23:37]

Ideally, we restrict to 1 raster thread running at a time, except in a case
where a low-priority task is running and we schedule a new high-priority task. 
In that case it may be desirable to allow 2 threads to run until the
low-priority task is done, and then suspend that thread.

1. High-priority tasks get affinity to one thread thread.
2. Low-priority tasks get affinity to one (lower) priority thread.
3. The high-priority thread sets a condition while running, so the low-priority
thread can't start new tasks.

This could be generalized to some N thread limit, rather than fixing to 1.

[aelias_OOO_until_Jul13, 2016-01-26 19:37:16]

On 2016/01/26 at 16:50:42, reveman wrote:
> On 2016/01/26 at 03:12:03, aelias wrote:
> > On 2016/01/26 at 02:38:33, reveman wrote:
> > > On 2016/01/26 at 01:54:46, sievers wrote:
> > > > +aelias who has some concerns that just using more threads will mainly
increase our power footprint.
> > > > 
> > > > How do people feel about two dedicated threads (foreground + background
work respectively) with setting thread niceness accordingly so that we are less
likely to spin up more big cores as a side effect of this patch>
> > > 
> > > Note that the current patch will not increase the number of threads used
for pre paint work. Pre paint will run on one thread on all platforms. Only
visible high priority tasks can use multiple threads.
> > 
> > If I understand correctly, that still leaves prepaint using one big core and
high priority tasks running on another big core, so in practice it sounds like
this will allow prepaint to harm scroll framerate still.
> > 
> > More fundamentally I see "high priority tasks can use multiple threads" as
basically a nongoal and not worth the threat to other performance/efficiency
measures.  Our raster pipeline has always liked to expand to hog as many
resources as possible, and I see the single raster thread as a natural barrier
that's been useful to maintain sanity.  Now, you might argue that there are some
scenarios (say, a raster-bound <canvas> game) where the user does benefit from
max throughput, and that it's possible in theory to restrict the multi-threading
to those particular scenarios, and not harm scrolling framerate and power usage
otherwise.  But our story for those scenarios is Ganesh, and furthermore our
focus on Ganesh means we won't necessarily devote enough attention to the
software path to avoid changes that might make it hog multiple big cores in the
future.
> 
> The problem with one raster thread is that low-priority pre-paint (e.g. a
large image decode) can block high priority visible content from being
generated. I was hoping we could improve that.

Agreed that is definitely worth improving.

> 
> Btw, I don't think raster will ever use a big core today. We use background
priority for worker threads on Android today and if understand correctly how
Android schedules work on big.LITTLE devices, this use of background priority
results in raster work (including Ganesh) never running on big cores.

No, as of https://codereview.chromium.org/1214503002 we never use priority 10 on
Android.  The little cores are too slow for high-priority raster on
very-high-dpi devices so always putting raster on them caused severe
checkerboarding on the Nexus 6P.  Confusingly, the name we use in Chrome is
still BACKGROUND since Dana didn't want to add another value to the enum in
base/ that's currently unused.  Look at patch set #3 of that patch for how to
add a new priority distinguishing big-core from little-core raster; now would be
a good time to add it, since we're planning to use it.

[reveman, 2016-01-26 20:00:38]

On 2016/01/26 at 19:37:16, aelias wrote:
> On 2016/01/26 at 16:50:42, reveman wrote:
> > On 2016/01/26 at 03:12:03, aelias wrote:
> > > On 2016/01/26 at 02:38:33, reveman wrote:
> > > > On 2016/01/26 at 01:54:46, sievers wrote:
> > > > > +aelias who has some concerns that just using more threads will mainly
increase our power footprint.
> > > > > 
> > > > > How do people feel about two dedicated threads (foreground +
background work respectively) with setting thread niceness accordingly so that
we are less likely to spin up more big cores as a side effect of this patch>
> > > > 
> > > > Note that the current patch will not increase the number of threads used
for pre paint work. Pre paint will run on one thread on all platforms. Only
visible high priority tasks can use multiple threads.
> > > 
> > > If I understand correctly, that still leaves prepaint using one big core
and high priority tasks running on another big core, so in practice it sounds
like this will allow prepaint to harm scroll framerate still.
> > > 
> > > More fundamentally I see "high priority tasks can use multiple threads" as
basically a nongoal and not worth the threat to other performance/efficiency
measures.  Our raster pipeline has always liked to expand to hog as many
resources as possible, and I see the single raster thread as a natural barrier
that's been useful to maintain sanity.  Now, you might argue that there are some
scenarios (say, a raster-bound <canvas> game) where the user does benefit from
max throughput, and that it's possible in theory to restrict the multi-threading
to those particular scenarios, and not harm scrolling framerate and power usage
otherwise.  But our story for those scenarios is Ganesh, and furthermore our
focus on Ganesh means we won't necessarily devote enough attention to the
software path to avoid changes that might make it hog multiple big cores in the
future.
> > 
> > The problem with one raster thread is that low-priority pre-paint (e.g. a
large image decode) can block high priority visible content from being
generated. I was hoping we could improve that.
> 
> Agreed that is definitely worth improving.
> 
> > 
> > Btw, I don't think raster will ever use a big core today. We use background
priority for worker threads on Android today and if understand correctly how
Android schedules work on big.LITTLE devices, this use of background priority
results in raster work (including Ganesh) never running on big cores.
> 
> No, as of https://codereview.chromium.org/1214503002 we never use priority 10
on Android.  The little cores are too slow for high-priority raster on
very-high-dpi devices so always putting raster on them caused severe
checkerboarding on the Nexus 6P.  Confusingly, the name we use in Chrome is
still BACKGROUND since Dana didn't want to add another value to the enum in
base/ that's currently unused.  Look at patch set #3 of that patch for how to
add a new priority distinguishing big-core from little-core raster; now would be
a good time to add it, since we're planning to use it.

Oh wow, that's confusing. Do we need visible raster to run on a thread with
lower than default priority on Android? This fine-tuning based on the specific
priority implementation on Android seems fragile. What if we just used default
priority for visible raster work and background priority (that matches
android.os.Process.THREAD_PRIORITY_BACKGROUND) for pre-paint?

[aelias_OOO_until_Jul13, 2016-01-26 20:16:02]

On 2016/01/26 at 20:00:38, reveman wrote:
> Oh wow, that's confusing. Do we need visible raster to run on a thread with
lower than default priority on Android? This fine-tuning based on the specific
priority implementation on Android seems fragile. What if we just used default
priority for visible raster work and background priority (that matches
android.os.Process.THREAD_PRIORITY_BACKGROUND) for pre-paint?

That sounds fine.  The reason raster thread is at lower priority is because it
contains a lot of prepaint work today.  Once we get the separate prepaint
thread, it sounds sane to promote the visible raster thread to default priority.

[ericrk, 2016-01-26 22:39:56]

https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
File content/renderer/raster_worker_pool.cc (right):

https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
content/renderer/raster_worker_pool.cc:147: // Background thread can only run
background tasks.
I like this idea in general, but a few questions:
 - What happens when a low priority task running on a background thread is
suddenly promoted to high priority (due to a view change / rescheduling). Are we
OK with it continuing to run to completion on the lower priority thread? Maybe
this is ok as it's only ever one task at most?
 - This change allows background work to run at all times, even when FG work is
still pending. We have a lower priority here, so maybe it's fine?

[aelias_OOO_until_Jul13, 2016-01-26 22:59:02]

On 2016/01/26 at 22:39:56, ericrk wrote:
>
https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
> File content/renderer/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
> content/renderer/raster_worker_pool.cc:147: // Background thread can only run
background tasks.
> I like this idea in general, but a few questions:
>  - What happens when a low priority task running on a background thread is
suddenly promoted to high priority (due to a view change / rescheduling). Are we
OK with it continuing to run to completion on the lower priority thread? Maybe
this is ok as it's only ever one task at most?

I had the same thought.  I don't have a strong opinion what the best solution
is.  There are three options, A) wait for the low priority thread to complete
it, B) temporarily increase the priority of the low-priority thread, C) create a
duplicative task on the high-priority thread.  I think we just go with the
simplest option A) for now and revisit if/when it comes up as a problem in
practice.

>  - This change allows background work to run at all times, even when FG work
is still pending. We have a lower priority here, so maybe it's fine?

Victor proposed in #20 that we stop this from happening most of the time, and
I'm inclined to agree that's the safest route.  I've definitely seen priority-10
work preempt the critical path in traces now and then; the Linux kernel allows
that to happen for "fairness".  (BTW, that same scheduling behavior is also what
allows option A above to be reasonable, as Linux won't allow low-priority tasks
to starve indefinitely under contention.)

[reveman, 2016-01-29 22:07:43]

On 2016/01/26 at 22:59:02, aelias wrote:
> On 2016/01/26 at 22:39:56, ericrk wrote:
> >
https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
> > File content/renderer/raster_worker_pool.cc (right):
> > 
> >
https://codereview.chromium.org/1616953003/diff/20001/content/renderer/raster...
> > content/renderer/raster_worker_pool.cc:147: // Background thread can only
run background tasks.
> > I like this idea in general, but a few questions:
> >  - What happens when a low priority task running on a background thread is
suddenly promoted to high priority (due to a view change / rescheduling). Are we
OK with it continuing to run to completion on the lower priority thread? Maybe
this is ok as it's only ever one task at most?
> 
> I had the same thought.  I don't have a strong opinion what the best solution
is.  There are three options, A) wait for the low priority thread to complete
it, B) temporarily increase the priority of the low-priority thread, C) create a
duplicative task on the high-priority thread.  I think we just go with the
simplest option A) for now and revisit if/when it comes up as a problem in
practice.

It would be nice to not rely on the ability to dynamically adjust thread
priority as the sandbox on non-Android platforms doesn't allow that. We can only
decrease priority of a thread.

We can always add some Android specific code that will temporarily bump the
priority at ScheduleTasks() time if a background thread happens to run a high
priority task.

> 
> >  - This change allows background work to run at all times, even when FG work
is still pending. We have a lower priority here, so maybe it's fine?
> 
> Victor proposed in #20 that we stop this from happening most of the time, and
I'm inclined to agree that's the safest route.  I've definitely seen priority-10
work preempt the critical path in traces now and then; the Linux kernel allows
that to happen for "fairness".  (BTW, that same scheduling behavior is also what
allows option A above to be reasonable, as Linux won't allow low-priority tasks
to starve indefinitely under contention.)

I agree with this too. I'll update the patch asap to prevent background work
from running unless all high priority work has completed.

[reveman, 2016-02-12 05:46:33]

Description was changed from

==========
content: Allow multiple raster threads on Android.

This improves performance on devices with 4 cores.

BUG=504515
==========

to

==========
content: Allow multiple raster threads on Android.
    
This should improve performance on devices with 4 cores while
also reducing the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.
    
BUG=504515
==========

[reveman, 2016-02-12 05:56:44]

Rebased this on Eric's WorkerPool changes.

This should now do what we discussed, never run pre-paint tasks unless all
foreground work is done and only run pre-paint on a single thread with
background priority. I've also adjusted the thread priorities so they match
Android, which means pre-paint is now running on a single small core on
big.LITTLE devices. This should hopefully improve Chrome's power usage while
also providing better performance for visible contents. 

PTAL

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:30: // URGENT_DISPLAY (-8).
Seems like a bit of an abuse to take URGENT_DISPLAY that is meant for ui
services on Android into account here. -4 not enough?

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:33: {ThreadPriority::BACKGROUND, 10},
Note that this will also make the renderer main thread use this priority when
smoothness is preferred.

[reveman, 2016-02-12 05:56:55]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

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

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

[commit-bot: I haz the power, 2016-02-12 09:09:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 09:09:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2016-02-12 16:49:52]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 16:50:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1616953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1616953003/60001

[commit-bot: I haz the power, 2016-02-12 18:25:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 18:25:07]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-02-23 16:28:25]

aelias, ping for review

[aelias_OOO_until_Jul13, 2016-02-23 23:01:41]

Thanks for following up on my concerns, I appreciate it!

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:30: // URGENT_DISPLAY (-8).
On 2016/02/12 at 05:56:44, reveman wrote:
> Seems like a bit of an abuse to take URGENT_DISPLAY that is meant for ui
services on Android into account here. -4 not enough?

Yeah, it seems a bit weird.  This was introduced in
https://chromiumcodereview.appspot.com/12741012 but there's no explanation for
the choice in the patch or code review discussion.  If you want to change it to
-4, fine by me.

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:33: {ThreadPriority::BACKGROUND, 10},
On 2016/02/12 at 05:56:44, reveman wrote:
> Note that this will also make the renderer main thread use this priority when
smoothness is preferred.

Yeah, that's concerning.  I don't think we want to force the main thread onto a
little core, that seems too aggressive.  Since you're significantly changing our
thread policy anyway, could you simply delete the main-thread-priority-change
effect of smoothness preferred?  It's one of those ugly complexities in our
scheduling policy, introduced back when we had a very different rendering
architecture (I believe it was in the days when raster was on main thread, so
the effect was primarily to throttle raster) -- I question its benefit so I
think it's time to try cleaning it out.

https://codereview.chromium.org/1616953003/diff/60001/base/threading/platform...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1616953003/diff/60001/base/threading/platform...
base/threading/platform_thread_android.cc:27: // - BACKGROUND corresponds to
Android's PRIORITY_BACKGROUND = 10 value and can
Could you write in the comment here that this priority will force the thread
onto a little core in big.LITTLE CPUs?  (I couldn't say so when I wrote the
previous comment because Android-team wanted to keep the existence of big.LITTLE
Nexus devices secret, but it's public knowledge now.)

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
File content/browser/gpu/compositor_util.cc (left):

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
content/browser/gpu/compositor_util.cc:195: num_raster_threads = 1;
I'm still skeptical of this though.  As I said earlier, I believe "any
additional raster threads should have niceness 10".  If I'm reading the patch
correctly, it looks like this change will add an extra thread with niceness 0 on
a 4-core device.

We have a ton of threads competing for our cores as it is.  I question that it's
a good idea to improve a raster benchmark by means of hogging more resources. 
Improving benchmarks by pure optimization is great, but this kind of change is
instead making a tradeoff.  Why do you believe it's the right tradeoff?  First
of all, can you point to a real-world site where I could experience the benefit
of the extra foreground raster thread on Android?

[reveman, 2016-02-24 13:00:17]

Description was changed from

==========
content: Allow multiple raster threads on Android.
    
This should improve performance on devices with 4 cores while
also reducing the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.
    
BUG=504515
==========

to

==========
content: Allow multiple raster threads on Android.
    
This should improve performance on devices with 4 cores while
also reducing the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.
    
BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[reveman, 2016-02-24 16:30:00]

Description was changed from

==========
content: Allow multiple raster threads on Android.
    
This should improve performance on devices with 4 cores while
also reducing the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.
    
BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
content: Improve thread priority for raster threads.
    
This reduces the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.

Threads used for foreground work are now given normal priority
on all platforms.
    
BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[reveman, 2016-02-24 17:08:54]

ptal

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:30: // URGENT_DISPLAY (-8).
On 2016/02/23 at 23:01:40, aelias wrote:
> On 2016/02/12 at 05:56:44, reveman wrote:
> > Seems like a bit of an abuse to take URGENT_DISPLAY that is meant for ui
services on Android into account here. -4 not enough?
> 
> Yeah, it seems a bit weird.  This was introduced in
https://chromiumcodereview.appspot.com/12741012 but there's no explanation for
the choice in the patch or code review discussion.  If you want to change it to
-4, fine by me.

Created a separate patch for this: https://codereview.chromium.org/1730053003

https://codereview.chromium.org/1616953003/diff/40001/base/threading/platform...
base/threading/platform_thread_android.cc:33: {ThreadPriority::BACKGROUND, 10},
On 2016/02/23 at 23:01:40, aelias wrote:
> On 2016/02/12 at 05:56:44, reveman wrote:
> > Note that this will also make the renderer main thread use this priority
when smoothness is preferred.
> 
> Yeah, that's concerning.  I don't think we want to force the main thread onto
a little core, that seems too aggressive.  Since you're significantly changing
our thread policy anyway, could you simply delete the
main-thread-priority-change effect of smoothness preferred?  It's one of those
ugly complexities in our scheduling policy, introduced back when we had a very
different rendering architecture (I believe it was in the days when raster was
on main thread, so the effect was primarily to throttle raster) -- I question
its benefit so I think it's time to try cleaning it out.

Removed it as part of this patch.

https://codereview.chromium.org/1616953003/diff/60001/base/threading/platform...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1616953003/diff/60001/base/threading/platform...
base/threading/platform_thread_android.cc:27: // - BACKGROUND corresponds to
Android's PRIORITY_BACKGROUND = 10 value and can
On 2016/02/23 at 23:01:40, aelias wrote:
> Could you write in the comment here that this priority will force the thread
onto a little core in big.LITTLE CPUs?  (I couldn't say so when I wrote the
previous comment because Android-team wanted to keep the existence of big.LITTLE
Nexus devices secret, but it's public knowledge now.)

Done.

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
File content/browser/gpu/compositor_util.cc (left):

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
content/browser/gpu/compositor_util.cc:195: num_raster_threads = 1;
On 2016/02/23 at 23:01:41, aelias wrote:
> I'm still skeptical of this though.  As I said earlier, I believe "any
additional raster threads should have niceness 10".  If I'm reading the patch
correctly, it looks like this change will add an extra thread with niceness 0 on
a 4-core device.
> 
> We have a ton of threads competing for our cores as it is.  I question that
it's a good idea to improve a raster benchmark by means of hogging more
resources.  Improving benchmarks by pure optimization is great, but this kind of
change is instead making a tradeoff.  Why do you believe it's the right
tradeoff?  First of all, can you point to a real-world site where I could
experience the benefit of the extra foreground raster thread on Android?

I'm not really looking to improve a specific benchmark or pushing for a specific
number of raster threads. I'd like to remove platform specific special cases.
Thanks to all the recent improvements to scheduling of raster work I'm hoping
that we can now use the same logic here on all platforms. ie. if it doesn't make
sense to use two raster threads on a 4 core Android device then that same logic
should apply to a 4 core arm chromebook or 4 core windows desktop.

The general use case for multiple raster threads is simply to allow chrome's
raster performance to scale on more powerful devices with multiple cores. More
precisely, not have idle cores when there's visible raster work to be done.
Pages with lots of images (especially animated images) tend to benefit the most
and we've seen good improvements on desktop and chromebook devices from this.

I'm OK changing the logic above to "num_raster_threads = num_processors / 4" if
we agree that that makes more sense. Or change the thread priority of raster
threads that run foreground work. The goal is to end up with logic that works
across all platforms.

I moved this num_raster_threads change to a different patch
(https://codereview.chromium.org/1733703002) as I'd rather not have the
otherwise really useful changes done by this patch blocked on this issue.

[aelias_OOO_until_Jul13, 2016-02-24 17:21:41]

lgtm

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
File content/browser/gpu/compositor_util.cc (left):

https://codereview.chromium.org/1616953003/diff/60001/content/browser/gpu/com...
content/browser/gpu/compositor_util.cc:195: num_raster_threads = 1;
On 2016/02/24 at 17:08:54, reveman wrote:
> On 2016/02/23 at 23:01:41, aelias wrote:
> > I'm still skeptical of this though.  As I said earlier, I believe "any
additional raster threads should have niceness 10".  If I'm reading the patch
correctly, it looks like this change will add an extra thread with niceness 0 on
a 4-core device.
> > 
> > We have a ton of threads competing for our cores as it is.  I question that
it's a good idea to improve a raster benchmark by means of hogging more
resources.  Improving benchmarks by pure optimization is great, but this kind of
change is instead making a tradeoff.  Why do you believe it's the right
tradeoff?  First of all, can you point to a real-world site where I could
experience the benefit of the extra foreground raster thread on Android?
> 
> I'm not really looking to improve a specific benchmark or pushing for a
specific number of raster threads. I'd like to remove platform specific special
cases.

Understood, that makes sense to me.  I've been continuing to mull it over on my
end, we can figure it out on the other patch.  Good idea on splitting it off
into another patch, it will also make the effect on the perf bots more obvious.

[reveman, 2016-02-24 17:49:08]

reveman@chromium.org changed reviewers:
+ halliwell@chromium.org, thakis@chromium.org

[reveman, 2016-02-24 17:49:09]

+thakis for base/ and chrome/
+halliwell for chromecast/
ping sievers for content/

[Nico, 2016-02-24 17:50:25]

base, chrome rs-lgtm based on lgtms from android experts upthread :-)

[halliwell, 2016-02-24 19:40:17]

On 2016/02/24 17:50:25, Nico wrote:
> base, chrome rs-lgtm based on lgtms from android experts upthread :-)

lgtm

[reveman, 2016-02-25 00:07:18]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 00:11:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1616953003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1616953003/120001

[commit-bot: I haz the power, 2016-02-25 00:22:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-25 00:22:19]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[reveman, 2016-02-25 00:35:15]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 00:38:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1616953003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1616953003/140001

[no sievers, 2016-02-25 01:16:18]

lgtm

[commit-bot: I haz the power, 2016-02-25 02:00:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-25 02:00:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2016-02-25 02:49:02]

The CQ bit was checked by reveman@chromium.org

[reveman, 2016-02-25 02:49:03]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org,
vmiura@chromium.org, aelias@chromium.org, thakis@chromium.org,
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1616953003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-25 02:50:58]

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

[commit-bot: I haz the power, 2016-02-25 03:42:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-25 03:43:59]

Description was changed from

==========
content: Improve thread priority for raster threads.
    
This reduces the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.

Threads used for foreground work are now given normal priority
on all platforms.
    
BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
content: Improve thread priority for raster threads.

This reduces the amount of resources that background work
can use by making the background thread that runs pre-paint
tasks use a priority that makes it run on a single small core
on big.LITTLE devices.

Threads used for foreground work are now given normal priority
on all platforms.

BUG=504515
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/56d6a40f9f84140e8a806325beec52bea3f3bd2a
Cr-Commit-Position: refs/heads/master@{#377490}
==========

[commit-bot: I haz the power, 2016-02-25 03:44:01]

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