Closed Bug 1059619 Opened 10 years ago Closed 7 years ago

Remove CPUWakeLock check when calculating priority in ComputePriority()

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: dliang, Unassigned)

References

Details

In ComputerPriority(), it will return PROCESS_PRIORITY_FOREGROUND_HIGH when CPUWakeLock is held and app has "critical" type. There are something need to discuss as following: 1. Is it good to calculate priority by CPUWakeLock? Generally, CPUWakeLock is just avoid system enter suspend and let app finish its jobs normally. I don't think it's good factor to calculate priority. 2. In multi-apps launch, it's possible to cause priority manager report HIGHER priority which is not we expected. For example, in bug 1056493, launch Dialer then Contact, it caused AppType is Critical by Dialer and CPUWakeLock was held by Contact, so the priority became PROCESS_PRIORITY_FOREGROUND_HIGH till condition is broken and other application cannot get enough CPU resource due to priority is wrong. http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#972
Hi Gabriele, would you mind leave some comments here?
Flags: needinfo?(gsvelto)
Depends on: 1056493
So, the "critical" app type plus a CPU wake-lock was especially designed to cover two scenarios: - An application absolutely needs to start ahead of other applications. This is the case when the dialer/callscreen has not been launched and there's an incoming call. Using the highest priority level to launch the app guarantees. See bug 836654 for more information on this. - An application needs to stay alive for a certain period of time. This is the scenario where for example the SMS app needs to store a message in the database and post a notification. Or the dialer needs to record a missed call in the dialer and post a notification. Etc... In general this are all valid scenarios. If applications are misusing the CPU wake-lock then we should fix the applications, not the priority manager. Now I see that in bug 1056493 this was removed but then backed out. This is very likely to cause startup regressions in the scenarios I've described above. Please when modifying the process priority manager at least needinfo me as I'm one of the few people left in Mozilla who worked on it in the beginning and knows almost all of its ins and outs.
Flags: needinfo?(gsvelto)
Scratch the final part of my previous comment, I actually got it wrong since the gecko patch in bug 1056493 didn't land. I've confused it with the patch in bug 874353.
(In reply to Gabriele Svelto [:gsvelto] from comment #2) > So, the "critical" app type plus a CPU wake-lock was especially designed to > cover two scenarios: > My concern is why do we need "critical" plus "CPUWakeLock" if we want to increase the priority of application. Could we increase priority just reference "critical" flag? By the definition of wake lock, it just used to avoid system go to suspend and application can make sure it can get CPU resource till its jobs is finished. Ideally, all applications or modules should hold their own wake lock if they have jobs to do. CPUWakeLock might be not a good factor to increase application priority. > - An application absolutely needs to start ahead of other applications. This > is the case when the dialer/callscreen has not been launched and there's an > incoming call. Using the highest priority level to launch the app > guarantees. See bug 836654 for more information on this. > > - An application needs to stay alive for a certain period of time. This is > the scenario where for example the SMS app needs to store a message in the > database and post a notification. Or the dialer needs to record a missed > call in the dialer and post a notification. Etc... > > In general this are all valid scenarios. If applications are misusing the > CPU wake-lock then we should fix the applications, not the priority manager. > Agreed, maybe my above example is not good. > Now I see that in bug 1056493 this was removed but then backed out. This is > very likely to cause startup regressions in the scenarios I've described > above. Please when modifying the process priority manager at least needinfo > me as I'm one of the few people left in Mozilla who worked on it in the > beginning and knows almost all of its ins and outs.
Flags: needinfo?(gsvelto)
(In reply to Danny Liang [:dliang] from comment #4) > My concern is why do we need "critical" plus "CPUWakeLock" if we want to > increase the priority of application. Could we increase priority just > reference "critical" flag? No because the critical flag is always set for certain applications (e.g. communications). > By the definition of wake lock, it just used to avoid system go to suspend > and application can make sure it can get CPU resource till its jobs is > finished. Ideally, all applications or modules should hold their own wake > lock if they have jobs to do. CPUWakeLock might be not a good factor to > increase application priority. Yes, the cpu wakelock alone is not a good indication hence our use of the 'critical' mozapptype. The rationale is the following: - If a regular application is holding the cpu wakelock and is in the foreground nothing happens - If a regular application is holding the cpu wakelock and is in the background then its priority is boosted above the level of other background applications (it gets the BACKGROUND_PERCEIVABLE priority level instead of BACKGROUND) - If a critical application is holding the cpu wakelock then it gets a higher priority than all other foreground applications (FOREGROUND_HIGH vs FOREGROUND). This happens even if the application is in the background and is used to speed up launching critical applications (communications for incoming calls and clock for alarms) - If a critical application is not holding the cpu wakelock the nothing happens to its priority
Flags: needinfo?(gsvelto)
I've updated the documentation in https://developer.mozilla.org/en-US/Firefox_OS/Platform/Architecture to better reflect how the priority levels are adjusted.
BTW you just made me realize we have a glaring bug regarding the dialer. communications/* apps are considered critical because we used them for receiving calls but they shouldn't be anymore. callscreen should be flagged critical but isn't. I'm going to file a bug.
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.