Closed Bug 980027 Opened 11 years ago Closed 11 years ago

honor elevated thread priorities for libmedia and compositor threads in gonk

Categories

(Core :: Graphics, defect, P2)

Product:
Core
Component:
Graphics
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
b2g-v1.4 --- affected
b2g-v2.0 --- affected

People

(Reporter: bkelly, Assigned: mchang)

References

Details

(Keywords: perf, Whiteboard: [c=uniformity p=4 s= u=])

Attachments

(3 files, 16 obsolete files)

Part1: Provide Mechanism to set thread priority via HAL. r=gsevelto, dhylands
(deleted), patch
mchang
: review+
Details | Diff | Splinter Review
Part 2: Set compositor thread priority to platform-based value. r=benwa, gsevelto
(deleted), patch
mchang
: review+
Details | Diff | Splinter Review
Part 3: Add compositor priority prefs to b2g.js. r=gsevelto
(deleted), patch
mchang
: review+
Details | Diff | Splinter Review 
In bug 977975 I see improved composition times and more consistent spacing when running the compositor thread at a high real-time priority.  Forking this bug off to get that landed behind a pref.

See the difference between these profiles on nexus-4.

Normal unix time-share priority:

  http://people.mozilla.org/~bgirard/cleopatra/#report=223d8d70950e35b4ab3db801363c1887b19f8f51

RT priority:

  http://people.mozilla.org/~bgirard/cleopatra/#report=a6ad7d33c304ebf393a5b9a64ca462ac16591912
Nice! We had ran this experiment with fennec a few years but didn't get good results. I'm not surprised with b2g we can do better.

I don't know much about scheduling but isn't real-time excessive? Would a high priority (negative nice factor) be sufficient? We don't want the compositor to impact a legitimate real time thread.
IMO based on the profiles we should try and get this in for 1.5 (or earlier).
(In reply to Benoit Girard (:BenWa) from comment #1)
> I don't know much about scheduling but isn't real-time excessive? Would a
> high priority (negative nice factor) be sufficient? We don't want the
> compositor to impact a legitimate real time thread.

IMO we have a real-time budget (16ms per frame) we are working towards, so RT priority makes sense.

Currently my patch is using SCHED_FIFO, but we could probably use SCHED_RR with an interval over our frame budget.  Conceptually, though, I think we want the compositor to complete its work without interruption as quickly as possible, so SCHED_FIFO makes sense to me.

Given that I had to raise the priority greater than the minimal RT value to get this improvement, I don't think negative nice values will be adequate.
Part 1 allows us to have RT priorities on threads without resetting them to the configured nice level used for all other process threads.
Attachment #8386313 - Flags: review?(gsvelto)
Blocks: 980030
Will post Part 2 later tonight.
Will this priority preempt any audio threads?
Right, good frame rate is important but I imagine something that is really real time like audio is more important (crack in the audio are more noticeable then a missed frame).

Similarly a thread like the sampler can't have a lower priority then the thing it's sampling otherwise we're going to get a lot of sampling gaps.
Agreed.  RT priorities span from 1 to 99, though.  I plan to use a pref so we can tune the right value or disable it.  We may want to add tunables for the other threads we care about such as audio.

The thing I want to avoid, though, is the dynamic priority changes time-share scheduling permits.  Right now the system could lower the compositor thread priority if it uses a lot of CPU (has work to do) in order to be more "fair".  I don't think we want this behavior.

I'm not sure what the audio thread priorities are currently using.  Are these gecko threads or gonk threads we're talking about?
RT priorities could affect to all audio handling thread. Before applying the change, confirmation of effect to audio seems necessary.

To change to RT priorities means that the compositor thread becomes higher priority than all audio task's thread. Audio also has real time performance request. Audio's intermittent stop problem could cause very bad user's impression.
From Comment 9, If the change only boost the performance only on nexus-4. It might be better not to apply to gecko.
Can we not similarly raise the priority of audio processing?  Is it running on the main thread or something?

Can you recommend a set of tests to validate audio is not broken?
Comment on attachment 8386313 [details] [diff] [review]
Part 1: Do not reset nice level on RT priority threads. (v1)

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

While this patch in isolation LGTM I'd like for us to take a more thorough approach to this problem. One issue we currently have and that you can find manifesting itself in bug 932701 is that all threads that are not the main thread are currently scheduled to have a nice value of one point more than the main thread. They may have been created with a different nice value but once the process priority manager kicks in and reschedules them it will set all of their nice values to the main thread's nice value plus one. In general we should preserve the relative nice value vs the main thread if possible and this is somewhat related to what you're trying to do. So instead of special-casing RT-scheduled threads I'd like to see a more organic solution for this problem that would handle thread priorities / scheduler policy in a more sensible way.

That being said I share the others' worries about audio threads though since we can already get jittery audio as it is. I'd argue for fixing bug 932701 first, possibly giving RT scheduling to the necessary audio/media threads and then do the same for the compositor thread - but with a lower priority. We should also do some thorough testing to ensure that this won't starve regular processes which might become a significant issue once we start using RT scheduling.
Attachment #8386313 - Flags: review?(gsvelto) → feedback+
(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> So instead of special-casing RT-scheduled threads I'd like to see a more
> organic solution for this problem that would handle thread priorities /
> scheduler policy in a more sensible way.

I'm not sure what you mean by an "organic solution".  I think most of our threads are appropriate for unix time-share scheduling because they don't really have timing guarantees to meet.  They just need to run at best effort for the system.

Things like composition and audio processing do have time guarantees to meet.  It seems very strange to me to say we should use something like the time-share scheduler for those kinds of workloads.  Even with lower nice levels time-share only guarantees that on *average* those threads will get more CPU time, but it does nothing to help with our timeliness guarantees.  You can can still get one of those threads dynamically de-prioritized if it uses the CPU long enough to be classified as "non-interactive".  This is enough to cause audio to skip or our FPS to jank during a fast scroll.

Maybe I don't understand what you mean, though.

> That being said I share the others' worries about audio threads though since
> we can already get jittery audio as it is. I'd argue for fixing bug 932701
> first, possibly giving RT scheduling to the necessary audio/media threads
> and then do the same for the compositor thread - but with a lower priority.
> We should also do some thorough testing to ensure that this won't starve
> regular processes which might become a significant issue once we start using
> RT scheduling.

I agree with evaluating the audio/media threads for RT priority, but I really don't see why this should wait for bug 932701.  These are fundamentally different types of workloads.
Also, I think it would be easier to prove this was safe if composition was running on a thread separate from an IPC message loop thread which permitted arbitrary code to be executed.  I wrote bug 980321 to track potentially moving composition to a separate thread.

Given people's concerns I think I'm going to go and mark that as a dependency here.  I don't think we have a plan to accomplish that, though.
Depends on: 980321
(In reply to Ben Kelly [:bkelly] from comment #13)
> I'm not sure what you mean by an "organic solution".  I think most of our
> threads are appropriate for unix time-share scheduling because they don't
> really have timing guarantees to meet.  They just need to run at best effort
> for the system.

Yeah but we're currently not respecting the relative nice values they should be running at. So for example, AudioTrackThread(s) from libmedia are created at nice level -16 (for example by typing in the dialer with keypad sounds enabled during a call) but once the app goes through a rescheduling cycle those threads will get a nice value of 1 like all others.

> Things like composition and audio processing do have time guarantees to
> meet.  It seems very strange to me to say we should use something like the
> time-share scheduler for those kinds of workloads.  Even with lower nice
> levels time-share only guarantees that on *average* those threads will get
> more CPU time, but it does nothing to help with our timeliness guarantees. 
> You can can still get one of those threads dynamically de-prioritized if it
> uses the CPU long enough to be classified as "non-interactive".  This is
> enough to cause audio to skip or our FPS to jank during a fast scroll.

I completely agree; what I meant is that if we want to introduce a way to special-case threads so that they have a non-standard scheduling policy then I'd like that mechanism to be generic enough to cover regular priorities too. That would fix the issue I've highlighted above as well as providing the necessary infrastructure for RT-scheduled threads.

> I agree with evaluating the audio/media threads for RT priority, but I
> really don't see why this should wait for bug 932701.  These are
> fundamentally different types of workloads.

We've already run into issues with jittery audio (see bug 917193 for which we have an ugly workaround in place) so if you're going to introduce real-time scheduling for certain threads I'd give some love to the audio threads first as they're the ones most sensible to jitter.

On a side note, are you aware of bug 980241? I think running the compositor at RT priority might be required to reliably sync refresh cycles to VSYNCs so there might be some interactions with that.
Ah, thanks for explaining.  I agree.

If we move media/audio threads to an RT priority, do you think it would make sense to do that under bug 932701?

And triggering composition based on vsync in bug 980241 seems like a most excellent idea.  I think we would want/need bug 980321 for that as well.
(In reply to Ben Kelly [:bkelly] from comment #16)
> If we move media/audio threads to an RT priority, do you think it would make
> sense to do that under bug 932701?

I think it would be OK since we know that just giving those threads lower nice values won't really help and we need RT scheduling for reliable results.

> And triggering composition based on vsync in bug 980241 seems like a most
> excellent idea.  I think we would want/need bug 980321 for that as well.

Indeed, let's link all these bugs so we don't lose track of the potential interactions :)
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=980241
Here is the thread definition in Android. FYR~
https://android.googlesource.com/platform/frameworks/native/+/jb-mr1.1-dev-plus-aosp/include/utils/ThreadDefs.h

    ANDROID_PRIORITY_URGENT_DISPLAY =  HAL_PRIORITY_URGENT_DISPLAY,   <== -8
    ANDROID_PRIORITY_AUDIO          = -16,

Android's Compositor runs with URGENT_DISPLAY which is lower than AUDIO. (BTW. Both are RT RR)

I don't understand the meaning of gfx IPC thread discussing in bug 980321. With OMTC, CompositeToTarget() is just running on Compositor thread, isn't it ?

With the implementation in bug 980241, Compositor can have 16.7ms budget to compose one frame. It provides more tolerance about scheduling latency, render/composition time variance, but I still think bug 980027 is important. (Thinking about what if one poor device takes about ~15ms to compose one frame, or the total area of Layers is large.)
(In reply to Vincent Lin[:vilin] from comment #18) 
> Android's Compositor runs with URGENT_DISPLAY which is lower than AUDIO.
> (BTW. Both are RT RR)

Thanks for the information!  That is useful for comparison.

> I don't understand the meaning of gfx IPC thread discussing in bug 980321.
> With OMTC, CompositeToTarget() is just running on Compositor thread, isn't
> it ?

It does run on the compositor thread, but that thread is an IPC message_loop as well.  So the compositor work is essentially in contention for thread time with synchonous IPC calls like gralloc allocation, layer updates, etc.

So splitting composition to a thread separate from the IPC thread would allow us to prioritize that work over the IPC work.

Hope that makes sense.
(In reply to Vincent Lin[:vilin] from comment #18)
>     ANDROID_PRIORITY_URGENT_DISPLAY =  HAL_PRIORITY_URGENT_DISPLAY,   <== -8
>     ANDROID_PRIORITY_AUDIO          = -16,
> 
> Android's Compositor runs with URGENT_DISPLAY which is lower than AUDIO.
> (BTW. Both are RT RR)

I see the media threads getting a nice level of -16, but I don't see them running at RT priority.

Is that because we don't go through the trampoline in here to set the policy when the priority changes?

  https://android.googlesource.com/platform/frameworks/native/+/jb-mr1.1-dev-plus-aosp/libs/utils/Threads.cpp
So after exploring a bit deeper, it appears android uses cgroups to control CPU usage between background and foreground processes.

As far as I can tell its not setting RT priorities.

Right now we are largely missing out on the benefit of the cgroup system because we directly call setpriority() in a number of places.  If we used androidSetThreadPriority(), however, we would get cgroup policies set.

The impact of setting the cgroups is defined here:

  https://android.googlesource.com/platform/system/core/+/jb-mr1.1-dev-plus-aosp/rootdir/init.rc

Threads in the bg_non_interactive cgroup are limited to at most 5% of the CPU.
Correction, I can get media to play with RT priority if I enable FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE.  In that case it appears to use RT priority 2 and SCHED_FIFO.  (As reported in /proc/<pid>/task/<tid>/stat.)
The RT priority value is defined in audioflinger/Threads.cpp:

  // Priorities for requestPriority
  static const int kPriorityAudioApp = 2;
  static const int kPriorityFastMixer = 3;

The SCHED_FIFO is set in gonk-misc/gonksched.cpp:

  https://github.com/mozilla-b2g/gonk-misc/blob/master/gonksched.cpp#L88
Since FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE is normally compiled off, the AudioTrack thread only gets RT priority if its sample rate matches the native sample rate.  For the nexus-4, this appears to be 48kHZ.

At least our reference workloads use 44.1kHZ, though.  I'm not sure about our other sound effects.  (Maybe we should match these to the native device sample rates when we package the system...)

In general, this suggests to me that the audio processes will not run with RT priority unless we can enable FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE.

My inclination is to enable it since smooth audio is a requirement for us, but since its disabled by default in android I hesitate to make that recommendation.
Here is a rough patch to expand the threads reporting in b2g-info -t.  It adds values for priority, RT priority, and scheduling policy.

I'd like to clean up the headers to identify what each thread column is.  Also, the regex is rather slow with the expanded fields.  Maybe we should use sscanf instead.
So after investigating the thread priority and cgroup arrangement in android I decided to change my patch a bit.

This new patch does the following:

1) Use androidSetThreadPriority() instead of setpriority() to take advantage of android cgroup configuration.
2) Do not batch set the priority of threads with priorities elevated above ANDROID_THREAD_NORMAL or running with an RT policy.  This will protect the libmedia threads.  (Verified using the b2g-info patch above.)
3) Allow gecko threads to set their priority to an elevated value that makes sense for their functionality on the given platform.  Currently this only support THREAD_PRIORITY_COMPOSITOR, but could be expanded to include web audio, etc.
4) Read the nice and RT priority values to use for THREAD_PRIORITY_COMPOSITOR.  Right now it defaults to ANDROID_PRIORITY_URGENT_DISPLAY and no RT priority.

Note, I was not sure how to get HAVE_ANDROID_OS defined correctly, so right now its directly defined in GonkHal.cpp.

Also, I restrict the new API to the parent process for now since I wasn't sure the best way to do child processes with the sandbox stuff.
Attachment #8386313 - Attachment is obsolete: true
Attachment #8388148 - Flags: feedback?(gsvelto)
Attachment #8388148 - Flags: feedback?(dhylands)
Whiteboard: [c= p=2 s= u=] → [c= p=4 s= u=]
Set the compositor thread to the platform specific value use HAL.
Attachment #8388150 - Flags: feedback?(gsvelto)
Attachment #8388150 - Flags: feedback?(bgirard)
Add b2g.js prefs to control the compositor priority.  Right now I set it to RT priority 1.  This may only make sense if we can turn on FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE in the audioflinger.
Attachment #8388151 - Flags: feedback?(gsvelto)
Attached patch audioflinger_fast_tracks.patch (obsolete) (deleted) — — Splinter Review 
This patch to frameworks/av turns on FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE which is necessary for us to reliably get RT priorities set in libmedia.

Michael, what do you think about enabling this?  Is there a good way to do it without upstreaming a patch to Android.mk?
Attachment #8388152 - Flags: feedback?(mwu)
I will collect profiles at the various priority levels tomorrow to help evaluate a good setting.
I think we can make an improvement here without fixing the IPC/compositor contention on the thread.
No longer depends on: 980321
Summary: run compositor thread at real-time priority based on pref → honor elevated thread priorities for libmedia and compositor threads in gonk
Comment on attachment 8388150 [details] [diff] [review]
Part 2: Set compositor thread priority to platform-based value.

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

In addition to watching out for impacts to other high priority threads like the media thread we need to make sure the compositor thread doesn't steal a lot of time from the main thread causing an increasing in checkerboarding.
Attachment #8388150 - Flags: feedback?(bgirard) → feedback+
Comment on attachment 8388148 [details] [diff] [review]
Part 1: Provide mechanism to set thread priority via HAL.

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

If we try to go down the cgroups road again we'll need additional testing (lots of it) because we don't have a good track record with them. You can have a look at the discussion in bug 861441 (and check attachment 739111 [details] [diff] [review] which was our first try at using cgroups); long story short we run into all sorts of weird/buggy behavior when using cgroups, in general it looked pretty flaky so we decided to stick with pure nice-based scheduling as it was definitely more robust.

That being said this patch would do the job even w/o using androidSetThreadPriority() so in general it looks good to me.

::: hal/gonk/GonkHal.cpp
@@ +1313,5 @@
>          aPid, errno);
>      return;
>    }
>  
> +  int rv = androidSetThreadPriority(aPid, aNice);

This puts a thread in the foreground/background cgroup depending on its nice value; if it's higher than 10 that works as background, if it's lower then it's foreground. With our current set of priorities this means that applications with BACKGROUND_HIGH priority will be in the foreground cgroup. I'm not sure if this is desirable or not.
Attachment #8388148 - Flags: feedback?(gsvelto) → feedback+
Attachment #8388150 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 8388151 [details] [diff] [review]
Part 3: Add compositor priority prefs to b2g.js and run at RT priority 1.

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

::: b2g/app/b2g.js
@@ +683,5 @@
> +
> +// By default the compositor thread on gonk runs without real-time priority.  RT
> +// priority can be enabled by setting this pref to a value between 1 and 99.
> +// Note that audio processing currently runs at RT priority 2 or 3 at most.
> +pref("hal.gonk.compositor.rt_priority", 1);

LGTM but I assume that we'll use 0 by default until we have proven that this does not starve other threads and processes in the system :)
Attachment #8388151 - Flags: feedback?(gsvelto) → feedback+
(In reply to Gabriele Svelto [:gsvelto] from comment #33)> This puts a thread in the foreground/background cgroup depending on its nice
> value; if it's higher than 10 that works as background, if it's lower then
> it's foreground. With our current set of priorities this means that
> applications with BACKGROUND_HIGH priority will be in the foreground cgroup.

Correction, this    ^^^^^^^^^^^^^^^ should have been BACKGROUND_PERCEIVABLE
(In reply to Gabriele Svelto [:gsvelto] from comment #33)
> If we try to go down the cgroups road again we'll need additional testing
> (lots of it) because we don't have a good track record with them. You can
> have a look at the discussion in bug 861441 (and check attachment 739111 [details] [diff] [review]
> [details] [diff] [review] which was our first try at using cgroups); long
> story short we run into all sorts of weird/buggy behavior when using
> cgroups, in general it looked pretty flaky so we decided to stick with pure
> nice-based scheduling as it was definitely more robust.

Ah, I was not aware we had tried using the android cgroups in the past.  Let me read up on the history here.

I guess my concern with not using cgroups is that we are spawning threads from android libraries that are adding themselves to the cgroups.  I was thinking we would want everyone to be working in the cgroup system or not.

By the way, even with this patch, I think most of our parent process threads are not added to the cgroups because the parent process does not go through this bulk priority change code.  We would probably need to hunt down all cases setpriority() and pthread_create() in order to get them added.  Without the parent process threads in the cgroup system the 5% limit on background threads would not be including the parent threads in their calculations.

> This puts a thread in the foreground/background cgroup depending on its nice
> value; if it's higher than 10 that works as background, if it's lower then
> it's foreground. With our current set of priorities this means that
> applications with BACKGROUND_HIGH priority will be in the foreground cgroup.
> I'm not sure if this is desirable or not.

I think that's what we want, though, if the child process is doing work in its main thread necessary for the audio/etc to be heard.  All the cgroup should do is allow the child to use more than 5% of the CPU.

(In reply to Gabriele Svelto [:gsvelto] from comment #34)
> > +pref("hal.gonk.compositor.rt_priority", 1);
> 
> LGTM but I assume that we'll use 0 by default until we have proven that this
> does not starve other threads and processes in the system :)

Yes, I want to do profiles at a variety of priority levels to show the impact and pick the right value.  This was more just for explanatory purposes at this point.
(In reply to Benoit Girard (:BenWa) from comment #32)
> In addition to watching out for impacts to other high priority threads like
> the media thread we need to make sure the compositor thread doesn't steal a
> lot of time from the main thread causing an increasing in checkerboarding.

I think if that's the case, then too much work is being done on the compositor thread.

If anything, though, I expect the compositor IPC stalling and blocking the child process has a bigger impact on checkerboarding right now.  If we can allow the compositor thread to respond more quickly and finish its work without my context switched out, then the stalling should be reduced a bit.  Really we need async messaging in bug 980321.
Hmm, it appears we were trying to create a new, gonk-specific cgroup hierarchy in bug 861441.  I think thats a bit different than just use the cgroups configured and used by android.  As you noted in the other bug, they only do things based on task and not process which seems more reliable.

Also, I imagine that androidSetThreadPriority() quite well tested on android.

(In reply to Ben Kelly [:bkelly] from comment #36)
> (In reply to Gabriele Svelto [:gsvelto] from comment #33)
> By the way, even with this patch, I think most of our parent process threads
> are not added to the cgroups because the parent process does not go through
> this bulk priority change code.  We would probably need to hunt down all
> cases setpriority() and pthread_create() in order to get them added. 
> Without the parent process threads in the cgroup system the 5% limit on
> background threads would not be including the parent threads in their
> calculations.

And I think my statement here is wrong.  All tasks are added to the top-level cgroup automatically which has the correct cpu.shares.  I verified this on device.  Sorry for the confusion.
Blocks: 970751
I'm still collecting profiles at different priority levels.  Its taking a bit longer than I would like due to hitting other bugs, etc.

That being said, I'm leaning towards this plan:

1) Remove androidSetThreadPriority() due to uncertainty about cgroups raised by Gabriele.
2) Leave hal.gonk.compositor.rt_priority set to 0 for now so it runs niced at -8, but not with RT priority.

I think this will help us in the short term as the libmedia threads will run at the correct priority in the background.

Then, moving forward:

3) Add a follow-up bug to consider androidSetThreadPriority.
4) Add a follow-up bug to add support for hal::SetCurrentThreadPriority() from the child process.  This will need an IPC call added since we want to remove these priority functions from the seccomp whitelist.
Here are some profiles on different devices at various priority levels.  I tested these conditions:

1) Original code without any patches from this bug
2) These patches with compositor at nice -8, but no RT priority
3) These patches with compositor at nice -8 and RT priority 1

I did not test higher RT priorities since our libmedia threads run at RT priority 2 or 3 at most.

I ran tested these conditions on a buri, nexus-4, and tarako.  I also tried nexus-5, but due to bug 981784 that device is pretty much unusable after tiling landed.

Here are the profiles.

No patches:
buri) http://people.mozilla.org/~bgirard/cleopatra/#report=0f1d099b7174583bdc6c64617580d3bf9969a506
nexus-4) http://people.mozilla.org/~bgirard/cleopatra/#report=d2c171b7e65c58b4ebfd763e7b22a17786af8983
tarako) http://people.mozilla.org/~bgirard/cleopatra/#report=4f3195a6054beabf556dec2c4ad7ec8ac88d656f

Compositor nice -8:
buri) http://people.mozilla.org/~bgirard/cleopatra/#report=d2abfb9c0082eb6ae40f21a4e4805caf6baef4ba
nexus-4) http://people.mozilla.org/~bgirard/cleopatra/#report=67b1f09df572d4e15d07b6ebea2adf5c27d5e3c1
tarako) http://people.mozilla.org/~bgirard/cleopatra/#report=239739a3e7d337d9fba07f9bf64628f1514843ed

Compositor nice -8, RT 1:
buri) http://people.mozilla.org/~bgirard/cleopatra/#report=ff513ebb5b94ed5c1bef96c8889ba242c61b0082
nexus-4) http://people.mozilla.org/~bgirard/cleopatra/#report=cc75b75ed1201bbf9cf8930050f868540077d27b
tarako) http://people.mozilla.org/~bgirard/cleopatra/#report=a27111bfaa447dfc839ef8cdd024707097cfe5cf

So what does all this mean?

a) I think RT 1 is clearly a negative impact for buri.  With RT 1 all composites consistently take 30ms.  I suspect we are forcing bug 980171, or some variant of it, since the firmware framebuffer disp_loop thread probably does not run with RT priority.
b) RT 1 seems to provide an improvement to nexus-4 and tarako.
c) Running with nice -8, but no RT priority seems to provide some slight improvement for the devices, but its not clear.  I don't think they are any worse than at nice 0.

Based on these results I'd like to move forward with the plan in comment 39.  I don't think we can enable RT priority at this time, especially given the buri results.
Comment on attachment 8388148 [details] [diff] [review]
Part 1: Provide mechanism to set thread priority via HAL.

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

::: hal/gonk/GonkHal.cpp
@@ +66,5 @@
>  #include "OrientationObserver.h"
>  #include "UeventPoller.h"
>  #include <algorithm>
>  
> +#define HAVE_ANDROID_OS 1   // for androidSetThreadPriority()

I promised Ben I would dig into how this gets set in a normal android build.

So here it is:

The file system/core/include/arch/linux-arm/AndroidConfig.h is the place that this normally is defined (for arm).

build/core/config.mk around line 162 has this:

> # $(1): os/arch
> define select-android-config-h
> system/core/include/arch/$(1)/AndroidConfig.h
> endef

line 101 of build/core/combo/TARGET_linux-arm.mk has:

> android_config_h := $(call select-android-config-h,linux-arm)
> arch_include_dir := $(dir $(android_config_h))

and immediately after that you can see

> TARGET_GLOBAL_CFLAGS = \
> ...
> 			-include $(android_config_h) \
> ...

So every source file compiled from within Android has AndroidConfig.h automagically #included

So I think that doing the #define HAVE_ANDROID_OS 1   is probably the simplest thing to do in this case.
If this was going to be more prevelant, then I would say that we should probably pass along arch_include_dir into the gecko build environment and have it add a -I to appropriate makefiles and then adding #include <AndroidConfig.h> where needed.

Personally, I think that putting HAVE_ANDROID_OS in configure isn't the right thing to do.

@@ +1507,5 @@
> +  if (rtPriority > 0) {
> +    sched_param schedParam;
> +    schedParam.sched_priority = rtPriority;
> +    LOG("Setting thread %d to RT priority %d", tid, rtPriority);
> +    rv = sched_setscheduler(tid, SCHED_FIFO, &schedParam);

This might need to be whitelisted in seccomp.
Attachment #8388148 - Flags: feedback?(dhylands) → feedback+
(In reply to Dave Hylands [:dhylands] from comment #41)
> @@ +1507,5 @@
> > +  if (rtPriority > 0) {
> > +    sched_param schedParam;
> > +    schedParam.sched_priority = rtPriority;
> > +    LOG("Setting thread %d to RT priority %d", tid, rtPriority);
> > +    rv = sched_setscheduler(tid, SCHED_FIFO, &schedParam);
> 
> This might need to be whitelisted in seccomp.

Is this the case even if I only perform this function in the parent process?  Right now I am enforcing that in the Hal sandbox code (I think).

Also, its currently defined here:

  http://mxr.mozilla.org/mozilla-central/source/security/sandbox/linux/seccomp_filter.h#134
Blocks: 932701
Comment on attachment 8388147 [details] [diff] [review]
Report RT scheduling policy and priority in b2g-info -t

I spun the b2g-info thread improvement patch off into bug 982268.
Attachment #8388147 - Attachment is obsolete: true
(In reply to Ben Kelly [:bkelly] from comment #42)
> (In reply to Dave Hylands [:dhylands] from comment #41)
...snip...
> > This might need to be whitelisted in seccomp.
 
> Is this the case even if I only perform this function in the parent process?

You're right - the parent isn't sandboxed (I couldn't remember).
Comment on attachment 8388152 [details] [diff] [review]
audioflinger_fast_tracks.patch

Spoke to Michael on IRC.  He does not recommend enabling FAST_TRACKS_AT_NON_NATIVE_SAMPLE_RATE.  Nuke this patch.

I think this will keep compositor at non-RT priorities for a while since many libmedia threads will not run with RT priority.
Attachment #8388152 - Attachment is obsolete: true
Attachment #8388152 - Flags: feedback?(mwu)
Updated to not use androidSetThreadPriority().  This also avoids the whole HAVE_ANDROID_OS issue.
Attachment #8389437 - Flags: review?(gsvelto)
Attachment #8388148 - Attachment is obsolete: true
Leave the prefs and their comments in the file, but comment them out so the defaults are used.  This leaves the compositor at nice level -8 without RT priority.
Attachment #8388151 - Attachment is obsolete: true
Attachment #8389438 - Flags: review?(gsvelto)
Attachment #8388150 - Flags: review?(bgirard)
Note, in addition to the investigation above into media thread priorities, I also did some runs with music running in the background.  I did not encounter any skips in the audio with these patches.  In contrast, with RT priority 99 on the compositor I could get an audio skip swiping the homepage.

If there are more specific tests I should run, please let me know.
Blocks: 982325
See bug 982325 for follow-on work to support the new hal::SetCurrentThreadPriority() function in child process threads.  We'll need this in order to support things like the profiler thread, web audio threads, etc.
Try build with patches under review:

  https://tbpl.mozilla.org/?tree=Try&rev=2b4781d5701b
Updated patch to fix the moz.build issue which broke the other platform builds on try.

New try build:

  https://tbpl.mozilla.org/?tree=Try&rev=ea9aee05b571
Attachment #8389437 - Attachment is obsolete: true
Attachment #8389437 - Flags: review?(gsvelto)
Attachment #8389531 - Flags: review?(gsvelto)
Comment on attachment 8389531 [details] [diff] [review]
Part 1: Provide mechanism to set thread priority via HAL. (v3)

Sotaro, if you have time, please take a look at this updated patch.  I really do want this to work for media.

Please note that RT priority is defaulted to off.

Also we explicitly avoid overriding the priority of threads with priority exceeding ANDROID_PRIORITY_NORMAL.  This allows the libmedia AudioTask decoder threads to remain at nice level -16.
Attachment #8389531 - Flags: feedback?(sotaro.ikeda.g)
(In reply to Ben Kelly [:bkelly] from comment #52)
> Comment on attachment 8389531 [details] [diff] [review]
> Part 1: Provide mechanism to set thread priority via HAL. (v3)
> 
> Sotaro, if you have time, please take a look at this updated patch.  I
> really do want this to work for media.

It seems to work to the problem like Bug 924383. Almost all android's codec and audio related threads has a thread priority more than ANDROID_PRIORITY_NORMAL.

But this method seems to assume that an audio thread gets more than ANDROID_PRIORITY_NORMAL. Around gecko, there are some types of threads. Some threads might be better to use this way of doing. On the other hand, there seems some threads that want to get relatively high priority, but applying more than ANDROID_PRIORITY_NORMAL seem too much. In the latter case, another way might be better to think about it later as different problem.
> 
> Also we explicitly avoid overriding the priority of threads with priority
> exceeding ANDROID_PRIORITY_NORMAL.  This allows the libmedia AudioTask
> decoder threads to remain at nice level -16.

The above thread seems for handling AudioTrack's callback.
Off the top of my head, there are the following threads. attachment 8389531 [details] [diff] [review] seems to fix the problem [1] [2].  From [4]-[8], it seems better to increase the thread priority more than ANDROID_PRIORITY_NORMAL. But to change all related thread to more than ANDROID_PRIORITY_NORMAL seems not a good idea.

- [1] AudioTrack's callback thread
    + Has ANDROID_PRIORITY_AUDIO(-16) priority.
    + Handle callback from AudioFlinger

- [2] android::OMX::CallbackDispatcherThread
    + Has ANDROID_PRIORITY_FOREGROUND(-2) priority.
    + Used for audio decoding, when stagefright's audio software codec is used.
    + b2g instantiate hw codec in mediaserver, when hw audio codec is used.
    + b2g instantiate software codec in an app process, when software audio codec is used.   

- [3] Binder thread
    + receive audio decoding data from android's mediaserver
    + audio decoding is done within the mediaserver when stagefright's hw audio codec is used.
    + binder driver adjust thread's priority.

- [4] MediaDecoderStateMachine's Decoding thread
    + gecko's platform independent sw codec's decoding is done on this thrad.
    + Audio and video decoding are done on the thread.
    + Bug 871431 is going to separate the audio and video decoding to different threads.

- [5] MediaDecoderStateMachine's AudioThread
    + Write audio data to AudioStream

- [6] MediaStreamGrph's thread
    + Used for WebAudio,WebRTC and MediaRecording
    + Handle Audio and video data

- [7] MediaRecording related threads
    + Used when do recording.

- [8] WebRTC related threads
    + Used when WebRTC use cases
(In reply to Sotaro Ikeda [:sotaro] from comment #55)
> Off the top of my head, there are the following threads. attachment 8389531 [details] [diff] [review]
> [details] [diff] [review] seems to fix the problem [1] [2].  From [4]-[8],
> it seems better to increase the thread priority more than
> ANDROID_PRIORITY_NORMAL. But to change all related thread to more than
> ANDROID_PRIORITY_NORMAL seems not a good idea.

If the task does not consume the much cpu time and have a real time requirement. It might be OK to increase the thread's priority more than ANDROID_PRIORITY_NORMAL. But if it is not, we do not have a enough reason to get more high priority than foreground app.
It might be better to get a feed back from :padenot and :cpearce if get a feedback to around thread priority related media.
> 
> - [4] MediaDecoderStateMachine's Decoding thread
>     + gecko's platform independent sw codec's decoding is done on this thrad.
>     + Audio and video decoding are done on the thread.
>     + Bug 871431 is going to separate the audio and video decoding to
> different threads.
> 

Correction:

Bug 979104 is going to separate the audio and video decoding to different threads.
Attachment #8389531 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Thanks Sotaro.  Its very helpful to have the list of media threads.

> - [3] Binder thread
>    + receive audio decoding data from android's mediaserver
>    + audio decoding is done within the mediaserver when stagefright's hw audio codec is used.
>    + binder driver adjust thread's priority.

This thread I'm not sure we can easily adjust.  My inclination would be to leave it where it currently is for now.  If its an IPC oriented thread then its going to be classified as interactive by the scheduler most of the time and nice level won't effect it much.

Does android audio processing go through this thread?

> - [4] MediaDecoderStateMachine's Decoding thread
>     + gecko's platform independent sw codec's decoding is done on this thrad.
>     + Audio and video decoding are done on the thread.
>     + Bug 871431 is going to separate the audio and video decoding to
> different threads.
> 
> - [5] MediaDecoderStateMachine's AudioThread
>     + Write audio data to AudioStream
> 
> - [6] MediaStreamGrph's thread
>     + Used for WebAudio,WebRTC and MediaRecording
>     + Handle Audio and video data
> 
> - [7] MediaRecording related threads
>     + Used when do recording.
> 
> - [8] WebRTC related threads
>     + Used when WebRTC use cases

I think we will be able to adjust the priority of threads [4] to [8] with the hal::SetCurrentThreadPriority() API mechanism.  To do this, though, I will need to fix bug 982325.

Also, my inclination would be to raise them to ANDROID_PRIORITY_AUDIO (nice -16).  If they are really directly involved with playing media that we don't want to skip, then this seems appropriate.  As an alternative we could use ANDROID_PRIORITY_FOREGROUND (nice -2), but that would suggest one of the threads is misbehaving in some way and maybe it should be adjusted to throttle itself instead.

Chris, Paul, what do you think?

Sotaro, also do you think we need to adjust these threads before raising the priority of compositor to nice -8?

If so, I will probably try to land these patches now, but with compositor pref'd to nice level 0.  This will let us get the AudioTask nice level protected immediately and allow us to experiment more easily with different levels.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(paul)
Flags: needinfo?(cpearce)
Comment on attachment 8389531 [details] [diff] [review]
Part 1: Provide mechanism to set thread priority via HAL. (v3)

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

This looks already fairly good but I think we can streamline the SetCurrentThreadPriority() function further, see my comments below:

::: hal/HalTypes.h
@@ +98,5 @@
>    PROCESS_CPU_PRIORITY_NORMAL,
>    NUM_PROCESS_CPU_PRIORITY
>  };
>  
> +enum ThreadPriority {

Please add comments describing the different priority levels, today we've got just two but they might become more and it's nice for a reader to immediately know what type of threads' they're meant for.

::: hal/gonk/GonkHal.cpp
@@ +1476,5 @@
>  void
> +SetCurrentThreadPriority(ThreadPriority aPriority)
> +{
> +  int androidPriority = ANDROID_PRIORITY_NORMAL;
> +  int rtPriority = 0;

I think it would be cleaner if you had only |priority| and |scheduler| variables, like this:

int scheduler = SCHED_OTHER;
int priority = ANDROID_PRIORITY_NORMAL;

Then you could do...

@@ +1482,5 @@
> +  switch(aPriority) {
> +  case THREAD_PRIORITY_NORMAL:
> +    androidPriority = ANDROID_PRIORITY_NORMAL;
> +    break;
> +  case THREAD_PRIORITY_COMPOSITOR:

... this here and similar adjustments in the switch:

if (Preferences::GetInt("hal.gonk.compositor.rt_priority", 0)) {
  // Realtime scheduling is enabled
  scheduler = SCHED_RR;
  priority = Preferences::GetInt("hal.gonk.compositor.rt_priority", 0);
} else {
  priority = Preferences::GetInt("hal.gonk.compositor.nice", ANDROID_PRIORITY_URGENT_DISPLAY);
}

@@ +1494,5 @@
> +  }
> +
> +  int tid = gettid();
> +
> +  LOG("Setting thread %d to nice level %d", tid, androidPriority);

And here you'll be able to simplify this into something like:

sched_param schedParam;
schedParam.sched_priority = priority;

int rv = sched_setscheduler(tid, scheduler, &schedParam);
if (rv) {
  // Report the error
}
Attachment #8389531 - Flags: review?(gsvelto)
Comment on attachment 8389438 [details] [diff] [review]
Part 3: Add compositor priority prefs to b2g.js. (v2)

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

LGTM but why are the new prefs commented out? The default values are safe and sound so you should just un-comment them.
Attachment #8389438 - Flags: review?(gsvelto) → review+
Hmm, I actually meant to remove THREAD_PRIORITY_NORMAL for now.  I'm not sure it makes sense or when it would be used.  I think I would prefer if code only called this API to set their priority to a special, functional value.  So THREAD_PRIORITY_COMPOSITOR would be the only value now, but we would likely want additional ones in the future like THREAD_PRIORITY_PROFILER, THREAD_PRIORITY_WEBAUDIO, etc.  We could add back THREAD_PRIORITY_NORMAL later if needed.

But yes, I will add comments.

(In reply to Gabriele Svelto [:gsvelto] from comment #60)
> I think it would be cleaner if you had only |priority| and |scheduler|
> variables, like this:
> 
> int scheduler = SCHED_OTHER;
> int priority = ANDROID_PRIORITY_NORMAL;

Hmm, I had originally intended to set the nice value and the RT priority because the nice had additional effects when using androidSetThreadPriority().  I guess I can drop that now we are using setpriority(), but I'm not 100% sure RT priority is a complete replacement for the nice value.

(In reply to Gabriele Svelto [:gsvelto] from comment #61)
> LGTM but why are the new prefs commented out? The default values are safe
> and sound so you should just un-comment them.

I guess its just a style I've seen in other config files before.  Show and document settings that are useful, but leave the values commented out with their default value.  I can uncomment them.
Also it appears I may have a symbol conflict with THREAD_PRIORITY_NORMAL on windows.  The try builds are all busted on windows.
Yea, windows uses THREAD_PRIORITY_NORMAL here:

  http://msdn.microsoft.com/en-us/library/windows/desktop/ms686277%28v=vs.85%29.aspx
(In reply to Ben Kelly [:bkelly] from comment #62)
> So THREAD_PRIORITY_COMPOSITOR would be the only value now, but we
> would likely want additional ones in the future like
> THREAD_PRIORITY_PROFILER, THREAD_PRIORITY_WEBAUDIO, etc.  We could add back
> THREAD_PRIORITY_NORMAL later if needed.

Sounds good, let's get rid of it, especially now that you've found out it conflicts with some other definition.

> Hmm, I had originally intended to set the nice value and the RT priority
> because the nice had additional effects when using
> androidSetThreadPriority().  I guess I can drop that now we are using
> setpriority(), but I'm not 100% sure RT priority is a complete replacement
> for the nice value.

I'll check the kernel scheduler to see if the nice and RT priority values are taken into account separately or not. From a high-level perspective I think they shouldn't otherwise the interface of sched_setscheduler() doesn't make much sense but it's always better to be sure.

> I guess its just a style I've seen in other config files before.  Show and
> document settings that are useful, but leave the values commented out with
> their default value.  I can uncomment them.

If this is common practice then I'm happy with it as it is, I just didn't know :)
Comment on attachment 8388150 [details] [diff] [review]
Part 2: Set compositor thread priority to platform-based value.

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

I'm ok with these changes but I don't want to change the compositor thread yet. Changing the niceness doesn't seem to have a large impact (I couldn't see any test/numbers to confirm this, and the profiles are within an earshot). Changing the RT value seems to make a huge difference but the problem on the buri is serious. I'd rather us try to understand the problem there instead.

My guess is making the compositor RT has a bad interaction with the libui display loop. I suggest that we investigate it further. We should get profiles include the libui thread. You can either try to call the SPS thread register symbol on the libui display thread or use another profile like perf. Once we can see into that thread I suspect the before/after RT profiles will be more informative. We may also need to set the profiler as RT otherwise the profiles wont interrupt the compositor and wont give good samples.

Also what does android' project butter do here?
Attachment #8388150 - Flags: review?(bgirard) → review-
The RT priorities are true real time priorities.

If a thread with an RT priority is the highest priority thread which is ready-to-run, then it will run. It will only be prempted by ISRs and higher priority threads.

I noticed that your patch used SCHED_FIFO. This treats the currently running thread as being higher priority than other threads of the same priority.

If there is only one thread at a given priority, then there is no difference between SCHED_FIFO and SCHED_RR. When I've played with RT threads in the past, I've normally picked SCHED_RR since this is what most realtime OSes would use.

Since the buri is being affected, there might be some internal thread running which needs to be boosted but which isn't.
(In reply to Ben Kelly [:bkelly] from comment #59)
> Thanks Sotaro.  Its very helpful to have the list of media threads.
> 
> > - [3] Binder thread
> >    + receive audio decoding data from android's mediaserver
> >    + audio decoding is done within the mediaserver when stagefright's hw audio codec is used.
> >    + binder driver adjust thread's priority.
> 
> This thread I'm not sure we can easily adjust.  My inclination would be to
> leave it where it currently is for now.  If its an IPC oriented thread then
> its going to be classified as interactive by the scheduler most of the time
> and nice level won't effect it much.
> 
> Does android audio processing go through this thread?

It seems better to keep it as un-touched.

In android case, if audio is playbacked by media server. it does not go through. When MediaCodec is instantiated in an application, binder ipc is used for deliver audio data to application process.
Flags: needinfo?(sotaro.ikeda.g)
We have to be very very careful to change the thread priority. I commented audio in Comment 9. But there are other requirement. Some events need to delivered as soon as possible. B2G is very very bad at it.

ProcessPriorityManager was originally created to fix "Incoming calls" use cases like Bug 847592. We have to be very careful to change the thread priority.

I basically opposed to raise all audio related threads too high priority.
(In reply to Ben Kelly [:bkelly] from comment #59)
> Chris, Paul, what do you think?

It's worth trying. As Sotaro says, it's probably bad to elevate priorities too much, as we then just get into an arms race with other threads.

Note that I landed patches to use a SharedThreadPool for MediaDecoderStateMachine decode and state machine threads. So you'll need to plumb priorities through to those pool's threads.

Also I don't know if we actually do decoding on B2g of MPEG codecs on the MediaDecoderStateMachine decode threads. That thread calls the platform decoders APIs, but those calls may block waiting for output from another system thread. Built in sw codecs (Ogg, WebM) do decode on this thread however.
Flags: needinfo?(cpearce)
Ok, I looked at the buri issue closer.  It turns out the fb_setUpdateRect() symbol from the profile in comment 40 was bogus.  I had a locally compiled version of that .so, but it wasn't installed on the device since I had reverted back to OEM firmware.

Sync'ing my library up I get this profile:

  http://people.mozilla.org/~bgirard/cleopatra/#report=3948400a8133b559f3850e164f7e28b27c57d9ff

This shows us blocked in fb_lockBuffer() which is exactly the issue from bug 980171.

Setting the framebuffer disp_loop() thread to RT1 to match the compositor greatly improves things:

  http://people.mozilla.org/~bgirard/cleopatra/#report=af28b72e2a28a81637691799f5d7b0149367a369

Not only are we not blocked for 30ms any more, but fb_lockBuffer seems to max out at 15ms now.  This still puts us over budget periodically, but not as bad as as the 25ms I was seeing at nice level 0 in bug 980171.

I'll write a new bug to track possibly changing the disp_loop() thread priority.  Its unclear to me the best way to do this since CAF does not seem particularly interested in upstreaming buri changes at the moment.
I wrote bug 982968 to investigate setting the RT priority on the gralloc framebuffer thread described in comment 71.  We'll see what CAF says.
I wrote bug 982972 to continue the investigation into running compositor at RT priority 1.  I made special note that this must be not cause audio/video skips and have sign off the media team.  It also depends on bug 982968.
Updated Hal patch based on review feedback.

Note, I could not exactly do what you suggested since SCHED_OTHER does not use the sched_param.sched_priority value.  From the sched_setscheduler() man page:

"For  processes  scheduled  under  one  of  the normal scheduling policies (SCHED_OTHER, SCHED_IDLE, SCHED_BATCH), sched_priority is not used in scheduling decisions (it must be specified as 0)."
Attachment #8389531 - Attachment is obsolete: true
Attachment #8390272 - Flags: review?(gsvelto)
Updated b2g.js preferences to keep the compositor at nice level 0 for now per Benoit's request.  In addition, the comment indicates gfx review is required to change these values.
Attachment #8389438 - Attachment is obsolete: true
Attachment #8390274 - Flags: review?(gsvelto)
Attachment #8390274 - Flags: review?(bgirard)
Comment on attachment 8388150 [details] [diff] [review]
Part 2: Set compositor thread priority to platform-based value.

Ask for re-review of this in light of the preference changes in part 3.
Attachment #8388150 - Flags: review- → review?(bgirard)
New try build:

 https://tbpl.mozilla.org/?tree=Try&rev=0db7296bcd6b
(In reply to Ben Kelly [:bkelly] from comment #71)
> Not only are we not blocked for 30ms any more, but fb_lockBuffer seems to
> max out at 15ms now.  This still puts us over budget periodically, but not
> as bad as as the 25ms I was seeing at nice level 0 in bug 980171.

Wow excellent news! Great work. Maybe we can play with the niceness of this + the compositor if we don't want to go to RT1.

> I'll write a new bug to track possibly changing the disp_loop() thread
> priority.  Its unclear to me the best way to do this since CAF does not seem
> particularly interested in upstreaming buri changes at the moment.

Perhaps these changes are needed everywhere but buri is most likely to miss the vsync and be pushed to 30ms instead of 15ms so it's affect the worse.
Attachment #8388150 - Flags: review?(bgirard) → review+
Attachment #8390274 - Flags: review?(bgirard) → review+
Flags: needinfo?(paul)
Comment on attachment 8390274 [details] [diff] [review]
Part 3: Add compositor priority prefs to b2g.js. (v3)

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

LGTM
Attachment #8390274 - Flags: review?(gsvelto) → review+
Comment on attachment 8390272 [details] [diff] [review]
Part 1: Provide mechanism to set thread priority via HAL. (v4)

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

(In reply to Ben Kelly [:bkelly] from comment #74)
> Note, I could not exactly do what you suggested since SCHED_OTHER does not
> use the sched_param.sched_priority value.  From the sched_setscheduler() man
> page:
> 
> "For  processes  scheduled  under  one  of  the normal scheduling policies
> (SCHED_OTHER, SCHED_IDLE, SCHED_BATCH), sched_priority is not used in
> scheduling decisions (it must be specified as 0)."

Good catch and thanks for having put it in the comment; I had missed that line in the man page and this is quite important.
Attachment #8390272 - Flags: review?(gsvelto) → review+
Thanks!  I don't see any consistent failure in the try build, so pushed to b2g-inbound:

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/098a43b537e9
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6344d6606517
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/5cd05df56f67
https://hg.mozilla.org/mozilla-central/rev/098a43b537e9
https://hg.mozilla.org/mozilla-central/rev/6344d6606517
https://hg.mozilla.org/mozilla-central/rev/5cd05df56f67
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Priority: -- → P2
Whiteboard: [c= p=4 s= u=] → [c= p=4 s=2014.03.14 u=]
Sorry but we have to back this out.

This is causing a serious race in hal with the pref service. We are not allowed to use the pref service off main thread.
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 881.930]
0xb42cb656 in PL_DHashTableOperate (table=0xb6487154, key=<optimized out>, op=<optimized out>) at ../../../xpcom/glue/pldhash.cpp:610
610	    DECREMENT_RECURSION_LEVEL(table);
(gdb) bt
#0  0xb42cb656 in PL_DHashTableOperate (table=0xb6487154, key=<optimized out>, op=<optimized out>) at ../../../xpcom/glue/pldhash.cpp:610
#1  0xb431af8a in pref_HashTableLookup (key=<optimized out>) at ../../../../modules/libpref/src/prefapi.cpp:733
#2  0xb431affe in PREF_GetIntPref (pref_name=<optimized out>, return_int=0xaf9ffcdc, get_default=<optimized out>) at ../../../../modules/libpref/src/prefapi.cpp:514
#3  0xb45b591e in GetInt (aDefault=0, aPref=0xb5c3c971 "hal.gonk.compositor.rt_priority") at ../dist/include/mozilla/Preferences.h:116
#4  mozilla::hal_impl::SetCurrentThreadPriority (aPriority=<optimized out>) at ../../hal/gonk/GonkHal.cpp:1493
#5  0xb44af438 in DispatchToFunction<void (*)()> (function=<optimized out>, arg=<optimized out>) at ../../../ipc/chromium/src/base/tuple.h:439
#6  RunnableFunction<void (*)(), Tuple0>::Run (this=<optimized out>) at ../../../ipc/chromium/src/base/task.h:415
#7  0xb44aa9ec in MessageLoop::RunTask (this=0xaf9ffde0, task=0xadb8d120) at ../../../ipc/chromium/src/base/message_loop.cc:344
Flags: needinfo?(kwierso)
Done: https://hg.mozilla.org/mozilla-central/rev/aba5b8498b30
Status: RESOLVED → REOPENED
Flags: needinfo?(kwierso)
Resolution: FIXED → ---
(In reply to Gregor Wagner [:gwagner] from comment #83)
> This is causing a serious race in hal with the pref service. We are not
> allowed to use the pref service off main thread.

Ouch, I forgot about that during review.
Sorry about that.  The problem with OMT-prefs is something I learned about looking at a later bug, but did not consider the implications here.  I'll add some code to load and cache the prefs.
I believe this landed just before the branch, so we need a backout on mozilla-aurora.
Backed out from Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ba50e20e7bb
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
Target Milestone: mozilla30 → ---
Whiteboard: [c= p=4 s=2014.03.14 u=] → [c= p=4 s= u=]
Assignee: bkelly → mchang
Whiteboard: [c= p=4 s= u=] → [c=uniformity p=4 s= u=]
Blocks: 991420
Depends on: 999115
Updated part 1 to read preferences off the main thread. We do this by having threads call hal:SetCurrentThreadPriority, which creates an NS_Runnable on the main thread to read the prefs and set the priority of the thread. Many of the design decisions were discussed in bug 999115.
Attachment #8390272 - Attachment is obsolete: true
Attachment #8414050 - Flags: review?(gsvelto)
Carrying over r+ for part 2. Rebased for mozilla-central tip.
Attachment #8388150 - Attachment is obsolete: true
Attachment #8414051 - Flags: review+
No longer depends on: 999115
Comment on attachment 8414050 [details] [diff] [review]
Part 1: Provide Mechanism to set thread priority via HAL (v5)

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

We're almost there, just a few nits and a couple of minor changes. I'd also like to add one more thing, while I've done some reviews regarding this code I'm not a peer so I'm not 100% sure I can r+ this without somebody else looking at the code too; you should double-check to see if you may need another reviewer before landing.

::: hal/HalTypes.h
@@ +105,5 @@
> +// Values that can be passed to hal::SetThreadPriority().  These should be
> +// functional in nature, such as COMPOSITOR, instead of levels, like LOW/HIGH.
> +// This allows us to tune our priority scheme for the system in one place such
> +// that it makes sense holistically for the overall operating system.  On gonk
> +// or android we may want different priority schemes than on windows, etc.

Please use the /** */ comment form.

@@ +128,5 @@
> +// Convert a ThreadPriority enum value to a string.  The strings returned by
> +// this function are statically allocated; do not attempt to free one!
> +//
> +// If you pass an unknown process priority (or NUM_THREAD_PRIORITY), we
> +// fatally assert in debug builds and otherwise return "???".

Same here.

::: hal/SetThreadPriorityRunnable.h
@@ +16,5 @@
> +extern void SetThreadPriority(int aThreadId, hal::ThreadPriority aThreadPriority);
> +
> +/**
> + * This class sets the priority of threads given a the kernel thread's id and a
> + * hal::ThreadPriority. We need this class because many priorities are set via

I think "This class sets the priority of threads given a the kernel thread's id and a value taken from hal::ThreadPriority." would be clearer.

@@ +19,5 @@
> + * This class sets the priority of threads given a the kernel thread's id and a
> + * hal::ThreadPriority. We need this class because many priorities are set via
> + * the preference service and prefs can only be read on the main thread.
> + * Users can create an instance of this class off main thread, and this will
> + * set the given thread priority and read the correct prefs on the main thread.

Instead of this last phrase you should specify that this runnable must always to be dispatched to the main thread and will fail otherwise. The existing comment sounds as if the runnable would be doing the right think irrespective of which thread it's dispatched to.

::: hal/gonk/GonkHal.cpp
@@ +27,4 @@
>  #include <sys/resource.h>
>  #include <time.h>
>  #include <asm/page.h>
> +#include <sched.h>

The other system includes seem to be in alphabetical order, can you put this below #include <regex.h>?

@@ +68,4 @@
>  #include "UeventPoller.h"
>  #include <algorithm>
>  
> +#include "SetThreadPriorityRunnable.h"          // For Set Thread Runnable

// For SetThreadPriorityRunnable

Also, could you move this between OrientationObserver.h and UeventPoller.h? That'll keep the includes in alphabetical order.

@@ +1484,5 @@
>    }
>  }
>  
> +static
> +bool

I'd put 'static bool' on a single line, this seems the most common patter in our code and the preferred one in the coding style guide.

@@ +1502,5 @@
> +  int rv = setpriority(PRIO_PROCESS, aTid, aValue);
> +
> +  if (rv) {
> +    LOG("Failed to set thread %d to priority level %s; error code %d",
> +        aTid, ThreadPriorityToString(aThreadPriority), rv);

The setpriority() returned error code is always -1 when the call fails. Use strerror(errno) instead to print out the actual error that occurred.

@@ +1525,5 @@
> +  int rv = sched_setscheduler(aTid, policy, &schedParam);
> +
> +  if (rv) {
> +    LOG("Failed to set thread %d to real time priority level %s; error code %d",
> +        aTid, ThreadPriorityToString(aThreadPriority), rv);

Likewise.

@@ +1533,5 @@
> +void
> +SetCompositorPriority(int aCompositorThreadId)
> +{
> +  int realTimePriority = Preferences::GetInt("hal.gonk.compositor.rt_priority", 0);
> +  int niceValue = Preferences::GetInt("hal.gonk.compositor.nice", ANDROID_PRIORITY_URGENT_DISPLAY);

It would be nice if you could encode the thread priority name in the preference like we do when setting priority for processes:

http://hg.mozilla.org/mozilla-central/file/d7c07694f339/hal/gonk/GonkHal.cpp#l1445

You wouldn't need a specific function for setting compositor priority but could make a generic one instead.

@@ +1550,5 @@
> +  // we create a race condition in HAL
> +  MOZ_ASSERT(NS_IsMainThread(), "Can only set thread priorities on main thread");
> +
> +  switch (aThreadPriority) {
> +  case THREAD_PRIORITY_COMPOSITOR:

Indent the case statements as per the coding style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +1551,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Can only set thread priorities on main thread");
> +
> +  switch (aThreadPriority) {
> +  case THREAD_PRIORITY_COMPOSITOR:
> +  {

Likewise you don't need braces here.

@@ +1565,5 @@
> +void
> +SetCurrentThreadPriority(ThreadPriority aThreadPriority)
> +{
> +  switch (aThreadPriority) {
> +  case THREAD_PRIORITY_COMPOSITOR:

Indent the case statement likewise, the braces are OK here since you're declaring a variable.

@@ +1569,5 @@
> +  case THREAD_PRIORITY_COMPOSITOR:
> +  {
> +    int threadId = gettid();
> +    nsCOMPtr<nsIRunnable> runnable =
> +          new SetThreadPriorityRunnable(threadId, aThreadPriority);

This should be indented by two spaces.
Attachment #8414050 - Flags: review?(gsvelto) → review-
(In reply to Gabriele Svelto [:gsvelto] from comment #92)
> We're almost there, just a few nits and a couple of minor changes. I'd also
> like to add one more thing, while I've done some reviews regarding this code
> I'm not a peer so I'm not 100% sure I can r+ this without somebody else
> looking at the code too; you should double-check to see if you may need
> another reviewer before landing.

Looking at the module owners for hal, it appears none of them work actively on the project any more:

  https://wiki.mozilla.org/Modules/Core#HAL

We should probably get that fixed...  I think Dave Hylands might be a good person, though, if you want a second opinion.
Thanks for the review. Updated with your feedback. From comment 93, asking dhylands for review too.
Attachment #8414050 - Attachment is obsolete: true
Attachment #8414674 - Flags: review?(gsvelto)
Attachment #8414674 - Flags: review?(dhylands)
Attachment #8414674 - Attachment description: part1.patch → Part 1: Provide Mechanism to set thread priority via HAL (v6).
Updated the preference names to capitalize compositor to align with feedback from comment 92.
Attachment #8390274 - Attachment is obsolete: true
Attachment #8414677 - Flags: review?(gsvelto)
(In reply to Ben Kelly [:bkelly] from comment #93)
> Looking at the module owners for hal, it appears none of them work actively
> on the project any more:
> 
>   https://wiki.mozilla.org/Modules/Core#HAL

Yuck, good point. I think we should raise this issue on dev-b2g.
Comment on attachment 8414674 [details] [diff] [review]
Part 1:  Provide Mechanism to set thread priority via HAL (v6).

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

I'd like to see a try run on all platforms with these changes.

f+ since my comments are primarily formatting/minor.

Clearing review since I'd like to see this again.

::: hal/Hal.cpp
@@ +909,5 @@
> +const char *
> +ThreadPriorityToString(ThreadPriority aPriority)
> +{
> +  switch (aPriority) {
> +  case THREAD_PRIORITY_COMPOSITOR:

nit: case should be indented from switch.
See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation

::: hal/HalTypes.h
@@ +118,5 @@
> + * Convert a ProcessPriority enum value (with an optional ProcessCPUPriority)
> + * to a string.  The strings returned by this function are statically
> + * allocated; do not attempt to free one!
> + *
> + * If you pass an unknown process priority (or NUM_PROCESS_PRIORITY), we

nit: I don't think that you need to mention NUM_PROCESS_PRIORITY here.

@@ +132,5 @@
>  /**
> + * Convert a ThreadPriority enum value to a string.  The strings returned by
> + * this function are statically allocated; do not attempt to free one!
> + *
> + * If you pass an unknown process priority (or NUM_THREAD_PRIORITY), we

nit: I don't think that you need to mention NUM_PROCESS_PRIORITY here.

::: hal/gonk/GonkHal.cpp
@@ +1490,5 @@
> +         (aValue <= sched_get_priority_max(aSchedulePolicy));
> +}
> +
> +void
> +SetThreadNiceValue(int aTid, ThreadPriority aThreadPriority, int aValue)

Shouldn't this function be static?

@@ +1492,5 @@
> +
> +void
> +SetThreadNiceValue(int aTid, ThreadPriority aThreadPriority, int aValue)
> +{
> +  MOZ_ASSERT(aThreadPriority < NUM_THREAD_PRIORITY);

Since aThreadPriority is an enum, it essentially has int semantics. So you should also assert >= 0 (throughout)

@@ +1498,5 @@
> +  LOG("Setting thread %d to priority level %s; nice level %d",
> +      aTid, ThreadPriorityToString(aThreadPriority), aValue);
> +  int rv = setpriority(PRIO_PROCESS, aTid, aValue);
> +
> +  if (-1 == rv) {

nit: I don't think that this particular style is very common in gecko.

I think:

if (rv)

is the preferred way.

@@ +1505,5 @@
> +  }
> +}
> +
> +void
> +SetRealTimeThreadPriority(int aTid,

Shouldn't this function be static?

@@ +1528,5 @@
> +  }
> +}
> +
> +void
> +SetThreadPriority(int aTid, hal::ThreadPriority aThreadPriority)

I think that this function should also be static, and you should move the runnable into this source file.

@@ +1537,5 @@
> +
> +  const char* threadName;
> +  switch (aThreadPriority) {
> +    case THREAD_PRIORITY_COMPOSITOR:
> +      threadName = ThreadPriorityToString(aThreadPriority);

I think that this is confusing. There is already the notion of the thread name, and this isn't it.

I'd use threadPriorityStr instead.

@@ +1550,5 @@
> +  int niceValue = Preferences::GetInt(
> +    nsPrintfCString("hal.gonk.%s.nice", threadName).get());
> +
> +  if (IsValidRealTimePriority(realTimePriority, SCHED_FIFO)) {
> +    SetRealTimeThreadPriority(aTid, aThreadPriority, realTimePriority);

Do an early return here.

Then you can unindent the else, and move the niceValue declaration after the if as well.

@@ +1561,5 @@
> +SetCurrentThreadPriority(ThreadPriority aThreadPriority)
> +{
> +  switch (aThreadPriority) {
> +    case THREAD_PRIORITY_COMPOSITOR: {
> +      int threadId = gettid();

gettid returns pid_t, not int. This should be fixed throughout

::: hal/sandbox/SandboxHal.cpp
@@ +364,5 @@
>  
>  void
> +SetCurrentThreadPriority(ThreadPriority aThreadPriority)
> +{
> +  NS_RUNTIMEABORT("Only the main process may set thread priorities.");

nit: This error message appears to be incorrect (for this context anyways).
Attachment #8414674 - Flags: review?(dhylands) → feedback+
Attachment #8414677 - Flags: review?(gsvelto) → review+
Comment on attachment 8414674 [details] [diff] [review]
Part 1:  Provide Mechanism to set thread priority via HAL (v6).

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

Clearing the review flag for now as I don't see any additional issues/nits beside those mentioned by Dave.
Attachment #8414674 - Flags: review?(gsvelto)
Thanks for the feedback, updated with the changes in comment 97. Here is a successful try build:

https://tbpl.mozilla.org/?tree=Try&rev=c6e8a1bb2dde
Attachment #8414674 - Attachment is obsolete: true
Attachment #8416761 - Flags: review?(gsvelto)
Attachment #8416761 - Flags: review?(dhylands)
Comment on attachment 8416761 [details] [diff] [review]
Part 1: Provide Mechanism to set thread priority via HAL (v7)

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

Great! Looks good.

::: hal/gonk/GonkHal.cpp
@@ +1529,5 @@
> +  }
> +}
> +
> +void
> +SetThreadPriority(pid_t aTid, hal::ThreadPriority aThreadPriority)

nit: This one should be static
Attachment #8416761 - Flags: review?(dhylands) → review+
Comment on attachment 8416761 [details] [diff] [review]
Part 1: Provide Mechanism to set thread priority via HAL (v7)

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

Besides the nit pointed out by Dave this LGTM.
Attachment #8416761 - Flags: review?(gsvelto) → review+
Fixed with nits and updated bug number in the patch. Carrying r+.
Attachment #8416761 - Attachment is obsolete: true
Attachment #8417545 - Flags: review+
Updated patch to include bug number and part number in commit message.
Attachment #8414051 - Attachment is obsolete: true
Attachment #8417546 - Flags: review+
Updated patch to include bug number and part number in commit message.
Attachment #8414677 - Attachment is obsolete: true
Attachment #8417547 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5964662eaece
https://hg.mozilla.org/mozilla-central/rev/9083afeee588
https://hg.mozilla.org/mozilla-central/rev/a51b839f72c5
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: