Closed Bug 1071156 Opened 5 years ago Closed 5 years ago

Raise the Compositor Thread Priority

Categories

(Core :: Graphics, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
feature-b2g 2.2+

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(2 files, 1 obsolete file)

We should be able to composite at 60 fps even if we raise the compositor thread priority. Currently, if we raise the compositor priority to -1 or -4 (which is what android uses), lots of things slow down. Find out why this happens.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
We could extend the profiler to be able to profile all the threads in the process. This would probably uncover threads that are used by the driver and might help us get a picture of what is happening on non gecko threads (gl driver). Filed bug 1071165 which might would help if we decide to go with that approach.
Depends on: 1071165
Raising it to -4 on a flame seems to work quite well. We tested scrolling around and browsing the web with audio playing in the background. Audio from the music app gets a priority of -16, video decoding gets a -2, aac decoding gets a -2 thread priority. My test case of nytimes.com, which checkerboards / long delays with touch seems to work with a compositor at -4. The video on the page works and audio plays in the background while loading nytimes.com until the video auto plays.
See Also: → 1061969
Attached patch Raise Compositor Priority to -4 (obsolete) — Splinter Review
-4 so we match Android's compositor priority.
Attachment #8493367 - Flags: review?(roc)
Many of the issues I initially reported were from my memory a couple of months ago. Those things don't seem to be occurring anymore.
Summary: Find why raising the compositor priority thread makes everything else really slow → Raise the Compositor Thread Priority
(In reply to Mason Chang [:mchang] from comment #4)
> Many of the issues I initially reported were from my memory a couple of
> months ago. Those things don't seem to be occurring anymore.

I'd be careful in testing this on a Flame only. The Flame is a dual-core device but most of our other phones aren't. This change might cause CPU starvation for other threads on single-core phones which wouldn't be visible on a Flame.
On ICS and JB, compositor thread was blocked longer time for composition if it try to compose layers.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> On ICS and JB, compositor thread was blocked longer time for composition if
> it try to compose layers.

It is hw platform dependent thing.
css-3d is one of very complex layer things. It might be better do testing in such situations. If we want to raise the priority more, we might need a lot of testings to confirm if there is no regression. Compositor thread activity could block kernel drivers work that has timing limitations.
We need to evaluate a situation that uses a lot of intermediate surface in compositor.
Not worried about pre-KK builds, and not worried about single core devices.  There is nothing in the pipeline that fits that description.
Sorry, I misunderstood that the patch want to raise the priority to real-time priority.
FWIW I don't think we can starve the driver work since the will eventually block on the driver flushing the work. But it's possible that we will prevent it from starting the work earlier giving a performance regression.
Start up time measurements for moz-visually-app-complete:

moz-app-visually-complete
Compositor - -4 priority
Calendar - 1651.06
Camera - 1860
Clock - 1136.77
Contacts - 1127
Dialer - 734.39
Email - 456
FM - 828
Gallery - 1102
Music - 1515
Settings - 2860
SMS - 1156
Video - 1087

Master - 0 priority
Calendar - 1639
Camera - 1875
Contacts - 1131
Dialer - 722.4
Email - 474.6
FM - 791
Gallery - 1152
Settings - 2896

So sometimes a roughly ~10-15ms start up regression, which is the range of noise. Also, animations feel smoother, so even if we spend more time in the compositor for smoother animations at the expense of some start up time, it's better for the user.
(In reply to Mason Chang [:mchang] from comment #13)
> So sometimes a roughly ~10-15ms start up regression, which is the range of
> noise.

Note that if this isn't noise it will show up on datazilla consistently so keep an eye on that after having landed this. That being said I agree that a single frame of delay is an acceptable price to pay for extra smoothness in animations and transitions.
Blocks: 1072381
Comment on attachment 8493367 [details] [diff] [review]
Raise Compositor Priority to -4

Review of attachment 8493367 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ -736,5 @@
>  //
>  // If RT priority is disabled, then the compositor nice value is used.  The
>  // code will default to ANDROID_PRIORITY_URGENT_DISPLAY which is -8.  Per gfx
>  // request we are keeping the compositor at nice level 0 until we can complete
>  // the investigation in bug 982972.

This comment should be updated before this is committed, or it's going to look rather confusing?
Carrying r+, updated comment around the priority.
Attachment #8493367 - Attachment is obsolete: true
Attachment #8494668 - Flags: review+
Try looks good. I also tested BenWa's suggestion of spin locking the compositor thread for 80ms per composite, which is attached. This represents a bad scenario where the compositor hogs the CPU. We see that the apps still load and we can still scroll, although janky which is expected. Loading apps took a small hit, but nothing unusable, which is quite good considering this is a very base scenario.
Keywords: checkin-needed
I want to track this as 2.2 "feature", because lot of people need to know about it...
feature-b2g: --- → 2.2+
Keywords: checkin-needed
Keywords: checkin-needed
I'm applying the change to test on my Nexus S, but for now it seems to be not that bad.
https://hg.mozilla.org/mozilla-central/rev/66d5d9147393
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.