870181 - Downgrade processes' CPU priorities when we receive a call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

We've determined that it's helpful for some of the testcases in bug 847592 if we lower other processes' CPU priorities when we receive a call.  I have patches to do this.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add an additional LowCPUPriority argument to hal::SetProcessPriority.

This patch also makes it an error to call hal::SetProcessPriority from a child process.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Lower CPU priority for non-high priority processes when there's an incoming call.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Test that we downgrade processes' CPU priorities when appropriate.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 747241 [details] [diff] [review]
Part 1: Add an additional LowCPUPriority argument to hal::SetProcessPriority.

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

Ok.

::: hal/Hal.cpp
@@ +877,5 @@
> +  switch (aPriority) {
> +  case PROCESS_PRIORITY_MASTER:
> +    if (aCPUPriority == PROCESS_CPU_PRIORITY_NORMAL) {
> +      return "MASTER:CPU_NORMAL";
> +    } else if (aCPUPriority == PROCESS_CPU_PRIORITY_LOW) {

Nit: Lots of else-after-return here.

::: hal/Hal.h
@@ +473,5 @@
> + * Set the priority of the given process.  A process's priority is a two-tuple
> + * consisting of a hal::ProcessPriority value and a hal::ProcessCPUPriority
> + * value.
> + *
> + * Two processes with the same PrrocessCPUPriority value don't necessarily have

Nit: "PrrocessCPUPriority" typo

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 747242 [details] [diff] [review]
Part 2: Lower CPU priority for non-high priority processes when there's an incoming call.

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

This looks fine but if there's an easier/better way to handle the following I'd prefer it...

::: dom/ipc/ProcessPriorityManager.cpp
@@ +51,5 @@
>  // priority manager.
>  //
>  // (Wow, our logging story is a huge mess.)
>  
> +#define ENABLE_LOGGING 1

Revert this before checkin.

@@ +955,5 @@
> +    ProcessPriorityToString(mPriority, mCPUPriority));
> +
> +  if (oldPriority != mPriority) {
> +    ProcessPriorityManagerImpl::GetSingleton()->
> +      NotifyProcessPriorityChanged(this, oldPriority);

So, the way I read this, we're going to have the following call chain (assuming all the right conditions are met):

  1. Call NotifyProcessPriorityChanged
  2. NotifyProcessPriorityChanged collects all the ParticularProcessPriorityManagers and for each:
    3. Call ResetCPUPriorityNow
    4. Call SetPriorityNow (the single arg overload)
    5. Call ComputeCPUPriority
    6. Call OtherProcessHasHighPriority
    7. OtherProcessHasHighPriority collects all ParticularProcessPriorityManagers again

That's a lot of hash enumeration every time a process starts or stops. Shouldn't there be a better way to do this?

Comment 6

[Justin Lebar (not reading bugmail)]

> That's a lot of hash enumeration every time a process starts or stops. Shouldn't there be a better 
> way to do this?

I feel like this is a premature optimization; a process starting or stopping is an expensive event.  At most we can have what, twenty apps running?  So we're talking 400 iterations of a loop.

But I'll see if I can improve this so it's not O(n^2) without adding complex invariants.

Comment 7

[Justin Lebar (not reading bugmail)]

I added a hashtable for the high-priority processes.  It's better; you were right.  Thanks, bent.  :)

https://hg.mozilla.org/projects/birch/rev/2c50a1950fd8
https://hg.mozilla.org/projects/birch/rev/6c53b7ecb2ee
https://hg.mozilla.org/projects/birch/rev/1fb1b2e7660f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c50a1950fd8
https://hg.mozilla.org/mozilla-central/rev/6c53b7ecb2ee
https://hg.mozilla.org/mozilla-central/rev/1fb1b2e7660f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/31244fd39afc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6aba0ca00f62
https://hg.mozilla.org/releases/mozilla-b2g18/rev/75f8fd838132

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a53ff6157699
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/68fdd8a0a9e1
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a566b3cf2687

Comment 10

[Deepa Subramanian]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 11

[Deepa Subramanian]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?