Closed Bug 1091037 Opened 10 years ago Closed 9 years ago

ImageBridgeChild thread's priority is not high enough on gonk

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 1 obsolete file)

Image thread runs as default thread priority. 
 - nice=1 ,when application is in foreground
 - nice=18, when application is in background.

ImageBridge just doing forwarding TextureClient to Compositor. ImageBridge need to handle data as quickly as possible. And it does not consume cpu continuouly long time. It's priority should be more high than current gecko.
On gonk, if thread's nice value is less than or equal to 0, it's priority is not changed when application goes to background.
Assignee: nobody → sotaro.ikeda.g
Hmm, hal::SetCurrentThreadPriority() is not allowed to be called from content process.
 https://dxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#357
Instead of permitting hal::SetCurrentThreadPriority(), it seems better that ImageBridgeParent calls hal::SetCurrentThreadPriority() to raise ImageBridge thread's priority.
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Instead of permitting hal::SetCurrentThreadPriority(), it seems better that
> ImageBridgeParent calls hal::SetCurrentThreadPriority() to raise ImageBridge
> thread's priority.

It is from security point of view.
Attachment #8601027 - Flags: feedback?(nical.bugzilla)
Attachment #8601027 - Flags: feedback?(gsvelto)
Comment on attachment 8601027 [details] [diff] [review]
patch - Raise ImageBridge thread priority

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

I don't know the android APIs very well but the general idea looks good to me. It also makes sense to do this on the other platforms.
Attachment #8601027 - Flags: feedback?(nical.bugzilla) → feedback+
Comment on attachment 8601027 [details] [diff] [review]
patch - Raise ImageBridge thread priority

This looks good to me but there's an important thing you have to bear in mind. Since bug 1081871 landed process priorities on b2g are not adjusted anymore using nice values but using control groups instead. Adjusting nice values of a process' threads will thus only have an effect relatively to the other threads in that process and not globally anymore. So lowering the nice value of a content process' thread below the nice value of one of the main process' threads is not going to give it higher priority.
Attachment #8601027 - Flags: feedback?(gsvelto) → feedback+
Thanks. I did not noticed bug 1081871 fix. For ImageBridge thread "relatively to the other threads in that process" is enough.
Attachment #8601027 - Flags: review?(nical.bugzilla)
Attachment #8601027 - Flags: review?(gsvelto)
Update nits.
Attachment #8601027 - Attachment is obsolete: true
Attachment #8601027 - Flags: review?(nical.bugzilla)
Attachment #8601027 - Flags: review?(gsvelto)
Attachment #8602248 - Flags: review?(nical.bugzilla)
Attachment #8602248 - Flags: review?(gsvelto)
Attachment #8602248 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8602248 [details] [diff] [review]
patch - Raise ImageBridge thread priority

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

LGTM, r+ for the HAL bits.
Attachment #8602248 - Flags: review?(gsvelto) → review+
https://hg.mozilla.org/mozilla-central/rev/d0b48a557161
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.