Don't reduce Gecko thread priority for multicore devices

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
It's not very meaningful to reduce Gecko thread priority when the device has multiple cores.
(Assignee)

Comment 1

3 years ago
Created attachment 8483567 [details] [diff] [review]
Don't reduce Gecko thread priority for multicore devices (v1)

For multicore devices, the Gecko thread should be able to run on a separate core than the UI thread, so there's no reason to lower the Gecko thread priority in consideration for the UI thread. Patch also fixes some possible race conditions involving sGeckoThread in ThreadUtils.
Attachment #8483567 - Flags: review?(mark.finkle)
Just curious: does lowering the priority of the Gecko thread hurt anything? If the threads are run on different cores, it seems like the lower priority wouldn't affect anything since there's no contention. Also, is there any guarantee that the Gecko/UI threads will actually be on separate cores for multi-core devices?
(Assignee)

Comment 3

3 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Just curious: does lowering the priority of the Gecko thread hurt anything?
> If the threads are run on different cores, it seems like the lower priority
> wouldn't affect anything since there's no contention. Also, is there any
> guarantee that the Gecko/UI threads will actually be on separate cores for
> multi-core devices?

I guess it depends on the situation. Gecko thread could still be competing with other threads on the device or competing with the background thread. There's no guarantee but the scheduler should be smart enough to figure it out.
Comment on attachment 8483567 [details] [diff] [review]
Don't reduce Gecko thread priority for multicore devices (v1)

I don't mind trying this out. Brian spent a lot of time thinking about this code, so I want to be sure he doesn't have some words of warning.

Sometimes mucking with priorities has a way of doing the opposite thing you want, so let's keep an eye on performance after it lands.
Attachment #8483567 - Flags: review?(mark.finkle)
Attachment #8483567 - Flags: review+
Attachment #8483567 - Flags: feedback?(bnicholson)
Comment on attachment 8483567 [details] [diff] [review]
Don't reduce Gecko thread priority for multicore devices (v1)

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

Since this code was based primarily on trial and error, my only recommendation would be to benchmark startup before and after your patch. When we landed the priority hack, we noticed a significant improvement in startup times for single core devices, so it could be useful to do the same here to make sure nothing regresses (and likewise, to see if anything improves).
Attachment #8483567 - Flags: feedback?(bnicholson) → feedback+
(Assignee)

Updated

3 years ago
Blocks: 807322
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1abb419a83e
https://hg.mozilla.org/mozilla-central/rev/b1abb419a83e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.