Bug 980027 was backed out because it causes a race condition when reading a preference. Preferences can only be read from main thread. Read the compositor thread priority preference on main thread and feed the priority to the compositor / gonk. There are a few ways to do this. 1) Read the priority on main thread startup and cache the value. Then have gonk read the value. 2) Use something like IPDL to send the priority when needed. 3) Set the value during Compositor creation and feed it to gonk there. Ben and I are both still new to this area of code. We think for now, (1) might be the best option.

I went with option (3). This patch also is against the patches in bug 980027. With this patch, when we create the CompositorParent, we're on the main thread and we read the prefs. Then we pass those values into the Compositor::SetThreadPriority, which is on the Compositor thread. I factored out the SetCurrentThreadPriority in HAL to be broken into two pieces: one for real time priorities, and one for nice values. I did this because I didn't want to pass in both a real time value and niceness value. I also think it makes more sense to bring those preferences up and contained in the Compositor. Finally, I asked Gregor how to reproduce the HAL race condition. I tested on a Nexus 4 with a debug build. I could not reproduce the race condition after 10 reboots. If it's all good, since bug 980027 did not land, I can merge this work into 980027. Thanks!

Comment on attachment 8410011 [details] [diff] [review] pref.patch So I know we talked about doing this on the main thread and that is what you are doing here, but I think this breaks the HAL abstraction. I was really trying to hide the concepts of priority behind the HAL interface because they may differ on different platforms. For example, I think RT priority means something very different on windows and we probably don't want to use it there. On some platforms we may not want to actually set the priority at all. So the goal was to have the end client thread (compositor here) just say to HAL, "hey, set my thread to whats appropriate for my functionality on this platform". HAL then does this without exposing any messy platform details. So, its problematic that in this patch we are referencing "gonk" prefs from the compositor which runs on all platforms. So I think we need a place where HAL runs on the main thread and can initialize any preferences it needs appropriately. Something like HAL::InitOnMainThread(). I think this should be hooked into the startup process somehow. Gabriele, what do you think? Finally, you probably want to split any changes into two patches here. One for HAL and one for gfx, as there are different peers for these compositor. I can provide feedback, but can't r+ (as far as I know). Thanks again for taking this issue!

(In reply to Ben Kelly [:bkelly] from comment #2) > I was really trying to hide the concepts of priority behind the HAL > interface because they may differ on different platforms. For example, I > think RT priority means something very different on windows and we probably > don't want to use it there. On some platforms we may not want to actually > set the priority at all. I agree, maybe we should do something similar to the process priorities: provide priority levels that are read from prefs by the code outside of HAL and then map the actual priority levels in HAL appropriately. In this particular case we might have two priority levels for use with hal::SetCurrentThreadPriority(): THREAD_PRIORITY_COMPOSITOR and THREAD_PRIORITY_COMPOSITOR_REALTIME which would use realtime priority on supporting hosts or be just a fallback for the former on hosts w/o realtime priorities. > So I think we need a place where HAL runs on the main thread and can > initialize any preferences it needs appropriately. Something like > HAL::InitOnMainThread(). I think this should be hooked into the startup > process somehow. There's no exact point from which the HAL code starts being used so we'd have to hook that up early in the startup process; if we're reading prefs though we'd also have to run it late enough for the prefs service to be working. I feel this would be problematic so I'm not very much in favor of this approach, I fear it might be fragile. > Finally, you probably want to split any changes into two patches here. One > for HAL and one for gfx, as there are different peers for these compositor. Agreed.

After talking with Ben, we thought it would be better to pass in a thread id to SetThreadPriority and keep the abstractions and reading prefs in the gonk layer. We can only call SetThreadPriority on the main thread. Since the PlatformThreadId is an abstraction for thread ids over the OS, I had to add some functionality to fetch the kernel thread id, which is what returns from gettid(). The current PlatformThreadId gives us the pthread->self() id, which is not the same. We need the kernel thread id to make the appropriate syscalls to the scheduler. Let's see if the second try is better. Thanks!

Comment on attachment 8411555 [details] [diff] [review] rt_compositor_pref.patch Review of attachment 8411555 [details] [diff] [review]: ----------------------------------------------------------------- So, I realized last night after logging off there may be a simpler way. ::: gfx/layers/ipc/CompositorParent.cpp @@ +217,5 @@ > +CompositorParent::SetThreadPriority() > +{ > + MOZ_ASSERT(NS_IsMainThread(), "Can only set compositor priority on main thread"); > + int kernelThreadId = sCompositorThread->kernel_thread_id(); > + hal::SetThreadPriority(kernelThreadId, hal::THREAD_PRIORITY_COMPOSITOR); How about we have |hal::SetThreadPriority()| immediately call the correct platform function to get the thread ID? So linux/gonk would call |gettid()|. hal::SetThreadPriority() would then issue a runnable on the main thread to perform the actual priority set. At that point the code could look up any pref it needs. Also, this would neatly fit into how we need to solve setting priorities in child processes. Instead of issuing a runnable on the main thread directly it would instead issue an IPC call that then issues a main thread runnable. This would maintain the same API surface as bug 980027, but solve the pref problem and get us half way to solving bug 982325 as well.

(In reply to Ben Kelly [:bkelly] from comment #5) > How about we have |hal::SetThreadPriority()| immediately call the correct > platform function to get the thread ID? So linux/gonk would call |gettid()|. +1, let's not expose the thread ID. > hal::SetThreadPriority() would then issue a runnable on the main thread to > perform the actual priority set. At that point the code could look up any > pref it needs. Wouldn't it be easier for the thread to issue a runnable to get the relevant pref from the main thread and then let the thread call hal::SetThreadPriority() with the proper priority? The latter call would then also dispatch a runnable to the main thread to do the actual priority setting. Basically, this wouldn't be much different than what we do in the ProcessPriorityManager. > Also, this would neatly fit into how we need to solve setting priorities in > child processes. Instead of issuing a runnable on the main thread directly > it would instead issue an IPC call that then issues a main thread runnable. Agreed, good point.

(In reply to Gabriele Svelto [:gsvelto] from comment #6) > Wouldn't it be easier for the thread to issue a runnable to get the relevant > pref from the main thread and then let the thread call > hal::SetThreadPriority() with the proper priority? The latter call would > then also dispatch a runnable to the main thread to do the actual priority > setting. Basically, this wouldn't be much different than what we do in the > ProcessPriorityManager. I guess I was trying to hide as much from the end code calling hal::SetThreadPriority() as possible. It seems to me that the calling code cares about saying "I am doing function X, please set me to the appropriate priority for this platform". Its up to the hal code to Do The Right Thing. Which on gonk may mean reading prefs for RT priority, but on windows might mean something completely different. I think keeping it in the hal layer also will make it easier to add new types of functions in the future. For example, if we run the profiler at elevated priority, then it won't need to worry about a pref step. It can just call hal::SetThreadPriority() and all the infrastructure will be there to handle it correctly. I don't want to bikeshed, though. So I will defer to what you guys think is best. :-)

(In reply to Ben Kelly [:bkelly] from comment #7) > I guess I was trying to hide as much from the end code calling > hal::SetThreadPriority() as possible. It seems to me that the calling code > cares about saying "I am doing function X, please set me to the appropriate > priority for this platform". Its up to the hal code to Do The Right Thing. > Which on gonk may mean reading prefs for RT priority, but on windows might > mean something completely different. > > I think keeping it in the hal layer also will make it easier to add new > types of functions in the future. For example, if we run the profiler at > elevated priority, then it won't need to worry about a pref step. It can > just call hal::SetThreadPriority() and all the infrastructure will be there > to handle it correctly. > > I don't want to bikeshed, though. So I will defer to what you guys think is > best. :-) No, actually what you proposed makes perfect sense, I had partially misunderstood it. In fact it's almost precisely what hal::SetProcessPriority() is doing already (minus delegating to the main thread via a runnable) so modeling hal::SetThreadPriority() against it is the best way to go. To recap we'd have something like this (pseudo-code ahead): hal::SetThreadPriority(aPriority) { int tid = gettid(); nsCOMPtr<nsIRunnable> runnable = new SetThreadPriorityRunnable(aPriority, tid); NS_DispatchToMainThread(runnable); } class SetThreadPriorityRunnable : public nsRunnable { public: SetThreadPriorityRunnable(aPriority, aTid) : mPriority(aPriority) , mTid(aTid) { } NS_IMETHOD Run() { if (!NS_IsMainThread()) { MOZ_CRASH("Should be run on the main thread!"); } else { // Read the scheduler type and arguments for the specified priority level and then call setpriority()/sched_setscheduler() on the tid } } };

Changed the architecture based on what Gabriele said in comment 8. The compositor called hal::SetCurrentThreadPriority. We then do what Gabriele says. The only difference is that the SetThreadPriorityRunnable just calls hal::SetThreadPriority(mTid, mThreadPriority), which reads the prefs and calls the correct setNiceValue / setRealTimeValue. Thanks for the help guys!

Comment on attachment 8413033 [details] [diff] [review] rt_compositor_pref.patch Review of attachment 8413033 [details] [diff] [review]: ----------------------------------------------------------------- Overall I think this is going in the right direction. The only significant change I would make would be to move the runnable into hal, rather than treating it as an external class. Then you don't need to expose the hal::setThreadPriority() method at all. For now this is probably easiest to do by putting the runnable in GonkHal specifically. Also, I think this should be moved back to bug 980027 as it doesn't really split out separately that well. Also, please flag gsvelto for r?. Thanks! ::: dom/ipc/SetThreadPriorityRunnable.h @@ +18,5 @@ > + * 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. > + */ > +class SetThreadPriorityRunnable : public nsRunnable I think this class should live in hal/ and belong to the hal:: namespace. @@ +32,5 @@ > + if (NS_IsMainThread()) { > + hal::SetThreadPriority(mThreadId, mThreadPriority); > + } else { > + MOZ_CRASH("Can only set Thread priorities on main thread\n"); > + } I think this can be written as: NS_ASSERTION(NS_IsMainThread(), "Can only set Thread priorities on main thread"); hal::SetThreadPriority(mThreadId, mThreadPriority); ::: gfx/layers/ipc/CompositorParent.cpp @@ +203,4 @@ > "The compositor thread must be Initialized before instanciating a COmpositorParent."); > MOZ_COUNT_CTOR(CompositorParent); > mCompositorID = 0; > + Whitespace change to this file needed? ::: hal/Hal.cpp @@ +884,1 @@ > } I'm not sure what it means to proxy here if we already require it to be called from the main thread? Overall I don't think this API should be exposed. If the runnable is part of the Hal internals, then we don't need this method exposed. ::: hal/Hal.h @@ +500,5 @@ > /** > + * Given the kernel thread's id and the thread priority, > + * set the current priority. This must be called on the main thread. > + */ > +void SetThreadPriority(int aTid, hal::ThreadPriority aThreadPriority); If the runnable is internal to Hal, then I don't think you need to expose this API.

After talking with Ben, this work should be in 980027, so resolving as dupe.