Closed Bug 822325 Opened 12 years ago Closed 11 years ago

Older apps can evict the most-recently-backgrounded app

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-basecamp:-, b2g18+)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
b2g18 + ---

People

(Reporter: dietrich, Assigned: alan.yenlin.huang)

References

Details

(Whiteboard: [Fugu] [v1.2f-uplift-needed])

Attachments

(1 file, 10 obsolete files)

Implement an LRU pool for background app processes. r=khuey
(deleted), patch
Details | Diff | Splinter Review 
STR:

1. Open Calculator, Gallery, Settings, FM Radio
2. Open the Twitter app
3. Click on a link in a tweet, causing the browser to open
4. Open card view

Expected: Browser and Twitter *at least* are listed

Actual: Browser and the other apps are listed, but Twitter is not

I can reproduce this every time. Unagi 12/16 build.
Requesting blocking in the case that this is a symptom of an underlying platform problem.

Chris, where in the stack is app eviction order determined?
blocking-basecamp: --- → ?
Component: Gaia → Gaia::System
There's no stack, the victim is chosen based on oom_adj class and amount of memory being used.

Android has a hack that adds a "previously viewed app" oom class, i.e. stack of one, but implementing something similar in gecko would be rather complex.
Component: Gaia::System → Gaia
Didn't mean to reset the component.  This is basically a gecko bug, but a workaround would go in the system app.
Component: Gaia → Gaia::System
What would be the risk to use an Android like hack ?
blocking-basecamp: ? → -
tracking-b2g18: --- → +
Flags: needinfo?(jones.chris.g)
We would need to define what "previous app" means and add code to track it and update apps when that changes.  Somewhat nontrivial.  If we get the tracking and updating wrong, we'll mess up the lowmemkiller heuristics.
Flags: needinfo?(jones.chris.g)
> We would need to define what "previous app" means and add code to track it and update apps when that 
> changes.

If Gaia can keep track of the previous app, it can just use the mozapptype parameter we added in bug 808231, and then this is simple from Gecko's perspective.

But I don't think the current behavior blocks.
Not quite, because we don't update that "live".
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Not quite, because we don't update that "live".

Ah, you're right.  It's not so simple.
Summary: more recently opened apps can get evicted before older apps → Older apps can evict the most-recently-backgrounded app
This is a pretty common scenario, for example: start composing email, open browser to check something, go back to finish email.  Fat apps like email (and apparently twitter) tend to get killed when under memory pressure.

We made the right call not to attempt this for v1.0, since it's not trivial, but we should start planning out how to tackle this.
blocking-b2g: --- → leo?
Whiteboard: UX-P?
This would probably be easier if the b2g process managed subprocess priorities in situ, as jlebar proposed in another bug.
Marking blocking+ for 1.1, pending UX evaluation of priority.
blocking-b2g: leo? → leo+
No longer depends on: 844323
(Just FTR, this would be a v.next tech-bug, but we should fix this for leo and are already tracking it, so I'm leaving it off the v.next list.)
Blocks: b2g-v-next
to general component, doesnt' seem like gaia
Component: Gaia::System → General
add Alan and Keven in cc loop
As cjones highlights, the job users have for the task manager is to be able to jump between the last few apps they have been using. Being able to kill processes is a distant second in terms of usefulness. Think of the task switcher as "tabs" for apps.

Given this, what we really want is a task switcher that lists apps in chronological order, by use. The task switcher could allow users to kill running processes, but apps show up independent of whether the process is actually running.

Users want to be able to seamlessly jump between apps they are using without an app mysteriously disappearing. Rather than band-aiding at the Gecko level, we can solve the UX problem by "pausing" apps so we can kill, then resurrect them without affecting the user experience.

Our proposed approach:

* Task manager displays the last few apps used, independent of whether process is running.
* When killing a process
  1. Send a "kill" event to the app, allowing it to save state
  2. Capture the current URL of the app -- this is used to represent the app at that state.
  3. Kill process
* When "thawing" an app
  * Restore the app at the URL that was active when the app was killed
    * This encourages app developers to do the weblike thing and represent state as a URL

Do we expect this feature to be delivered for 1.1? If so, what are we looking at as far as timeline for delivering a complete UX spec?
Whiteboard: UX-P?
> Do we expect this feature to be delivered for 1.1?

I think this is exactly the sort of want-to-have feature that we should not block on.
Assignee: nobody → ahuang
Attached patch proposed fix, v1 (obsolete) (deleted) — — Splinter Review 
This proposed fix uses an LRU pool for background processes. So LRU background process would be the last to kill by LMK.

Hi Dave,
This is a fix as our previous mail discussion. Can you help to review it? Many thanks!
Attachment #727042 - Flags: review?(dhylands)
Attachment #727042 - Flags: review?(dhylands) → review?(justin.lebar+bug)
Comment on attachment 727042 [details] [diff] [review]
proposed fix, v1

Thanks for your patch, Alan.

We need to improve this patch's conformance to Mozilla's coding conventions.
These are cosmetic issues, but they're pretty important for maintaining our
sanity.  Here are some of the changes that you should make here.  In general
these rules should apply to most C++ patches for Gecko.

* Functions start with capital letters.
* Functions that aren't used outside a cpp file are declared static or declared
  inside an anonymous namespace.
* Arguments to functions start with "a".  For example, |void Foo(int aBar)|.
  (This rule does not apply in the JS engine and in a few other places.)
* Lines are wrapped at 80 characters.
* Declare |for| loop variables inside the for() where possible.
* Declare other variables as close to their first use as possible (even in C
  code, but especially in C++).
* Put spaces around arithmetic operators and "=".
* Use nullptr, not NULL.
* Do not mix /* and // inside one comment.

I have other nits on the style and naming, but let's fix these first.

I'm also not convinced that we want a large LRU pool.  This can lead to us
wasting memory that we could otherwise use for background processes.

Consider the case when we have three processes using different amounts of
memory:

  Process A: 50mb
  Process B: 75mb
  Process C: 25mb

Suppose process A was most-recently used, followed by process B, followed by
process C, and suppose our device has a 100mb limit on how much memory it has
available.

If we apply strict LRU here, we can only have process A in memory.  But we
actually have space for processes A and C.  LRU is clearly worse here than the
alternative.

I think the solution here is to make the LRU buffer very small (one or two
apps).  Then most background apps will have the same priority, and we can kill
the biggest ones instead of the least-recently-used ones.

>+// Background LRU pool size, used for ranking background app oom_adj
>+// and out of background process killer.

I don't understand what this comment means.  You might say instead

  // Number of diferent OOM-adjust levels for background processes.  We use
  // these different levels to force the low-memory killer to kill processes in
  // a LRU order.

>+pref("hal.processPriorityManager.gonk.backgroundLruPoolSize", 6);

Please capitalize "LRU" here and elsewhere.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp
>--- a/hal/gonk/GonkHal.cpp
>+++ b/hal/gonk/GonkHal.cpp

>@@ -29,16 +29,17 @@
> 
> #include "android/log.h"
> #include "cutils/properties.h"
> #include "hardware/hardware.h"
> #include "hardware/lights.h"
> #include "hardware_legacy/uevent.h"
> #include "hardware_legacy/vibrator.h"
> #include "hardware_legacy/power.h"
>+#include "private/android_filesystem_config.h"
> 
> #include "base/message_loop.h"
> 
> #include "Hal.h"
> #include "HalImpl.h"
> #include "mozilla/dom/battery/Constants.h"
> #include "mozilla/FileUtils.h"
> #include "mozilla/Monitor.h"

>@@ -1095,29 +1096,144 @@ EnsureKernelLowMemKillerParamsSet()
>         &lowMemNotifyThresholdMB))) {
> 
>     // notify_trigger is in pages.
>     WriteToFile("/sys/module/lowmemorykiller/parameters/notify_trigger",
>       nsPrintfCString("%d", lowMemNotifyThresholdMB * 1024 * 1024 / PAGE_SIZE).get());
>   }
> }
> 
>+static int32_t sBackgroundLruPoolSize = -1;
>+static int *sBackgroundLruPool = NULL;
>+
>+static void
>+MakeBackgroundLruPool()

A better name would have the word "Ensure" instead of "Make", because this
function does not make the pool if it's already been created.

>+{
>+  static bool madeBackgroundLruPool;
>+  if (madeBackgroundLruPool) {
>+    return;
>+  }
>+  madeBackgroundLruPool = true;
>+
>+  /* We set sBackgroundLruPoolSize according to our pref.
>+  // This value is used to set background process LRU pool
>+  // and out of background process killer.
>+  */
>+  if (!NS_SUCCEEDED(Preferences::GetInt(
>+        "hal.processPriorityManager.gonk.backgroundLruPoolSize",
>+        &sBackgroundLruPoolSize))) {
>+    MOZ_CRASH();
>+  }

>+  HAL_LOG(("Making background LRU pool with size(%d)", sBackgroundLruPoolSize));
>+
>+  if (sBackgroundLruPoolSize > 0) {
>+    sBackgroundLruPool = (int *)malloc(sizeof(int)*sBackgroundLruPoolSize);
>+    memset(sBackgroundLruPool, 0, sizeof(int)*sBackgroundLruPoolSize);
>+  }

I think it would be better to use an nsTArray for this.  Then we don't have to
deal with dangerous functions like memset and memmove.  You'd need to change
sBackgroundLruPool to

  StaticAutoPtr<nsTArray<int32_t> > sBackgroundLRUPool;

and then inside the EnsureBackgroundLRUPool function, you'd do

  sBackgroundLRUPool = new nsTArray<int32_t>();
  ClearOnShutdown(&sBackgroundLRUPool);

>+void
>+addIntoBackgroundLruPool(int aPid, int backgroundOomAdj)
>+{
>+  int availableIndex = -1;
>+  int i;
>+  struct stat sb;
>+
>+  /* We use system call stat() to get uid of the target process. 
>+  // This is used to avoid adding Pre-allocated process into pool.
>+  */
>+  if (stat(nsPrintfCString("/proc/%d/stat", aPid).get(), &sb) == -1) {
>+    HAL_LOG(("Cannt access /proc/%d/stat", aPid));
>+    return;
>+  }
>+
>+  if (sb.st_uid == AID_ROOT) {
>+    HAL_LOG(("Skip process pid=%d. We should only add background apps into pool", aPid));
>+    return ;
>+  }

I don't want us to use the UID to decide which is the preallocated process.
This condition will break if we start running the preallocated process as
non-root, if we ever run apps as root (we were for a time; I don't know if we
still do), or if we ever run Gecko as non-root.

Instead, perhaps you can add a static method to ContentParent that gives you
the PID of the current preallocated process, if it exists.

Actually, I think there are race conditions here if we ever reuse a PID.  I'm
sure Gecko is already reaping its processes.  The right thing to do would be to
use the ChildID() of the relevant ContentParent.  That value is unique.  We'd
have to change hal to accept this value instead of the PID.

>+  /* Currently marked this out of background process killer.
>+  // We used 256MB and 512MB unagi kernel and found that there's
>+  // no gain on 256MB device.
>+  */
>+  /*
>+  if (sBackgroundLruPool[sBackgroundLruPoolSize-1] > 0 && availableIndex == -1) {
>+    if (kill(sBackgroundLruPool[sBackgroundLruPoolSize-1], SIGKILL) == 0) {
>+      HAL_LOG(("Out of background process killed process %d", sBackgroundLruPool[sBackgroundLruPoolSize-1]));
>+    }
>+  }
>+  */

We don't check in commented-out code like this into the tree.  If you want to
mention that it's possible to kill processes when we no longer have room in the
background LRU pool, please add that as a comment.

I don't think we can do this unless we also add values corresponding to these
new oom_adj's to

    /sys/module/lowmemorykiller/parameters/adj and
    /sys/module/lowmemorykiller/parameters/minfree.

This work will be a lot easier once I finish bug 844323.  Since that bug is
must-have, while this is nice-to-have, I'm happy to wait for that bug to land
before working on this, if you're OK with that.
Attachment #727042 - Flags: review?(justin.lebar+bug) → review-
(In reply to Justin Lebar [:jlebar] from comment #18)
> I'm also not convinced that we want a large LRU pool.  This can lead to us
> wasting memory that we could otherwise use for background processes.
> 
> Consider the case when we have three processes using different amounts of
> memory:
> 
>   Process A: 50mb
>   Process B: 75mb
>   Process C: 25mb
> 
> Suppose process A was most-recently used, followed by process B, followed by
> process C, and suppose our device has a 100mb limit on how much memory it has
> available.
> 
> If we apply strict LRU here, we can only have process A in memory.  But we
> actually have space for processes A and C.  LRU is clearly worse here than
> the
> alternative.
> 
> I think the solution here is to make the LRU buffer very small (one or two
> apps).  Then most background apps will have the same priority, and we can
> kill
> the biggest ones instead of the least-recently-used ones.

If we use strict LRU but let many background apps having the same priority, we could reach the goal "killing he biggest ones instead of the least-recently-used ones"

I think we could use below approach:
LRU buffer size = 2^(# of background LRU pool levels)-1
Set oom_adj of each background process and maintain LRU buffer as below:

Priority (oom_adj) background  : LRU0
Priority (oom_adj) background+1: LRU1, LRU2
Priority (oom_adj) background+2: LRU3, LRU4, LRU5, LRU6
Priority (oom_adj) background+3: LRU7, LRU8, LRU9, LRU10, LRU11, LRU12, LRU13, LRU14
...
Priority (oom_adj) background+L-1: 2^(# of background LRU pool levels - 1)
(End of buffer)

I think this could still take us away from this bug and reduce the happened rate of above mentioned case.

> I don't want us to use the UID to decide which is the preallocated process.
> This condition will break if we start running the preallocated process as
> non-root, if we ever run apps as root (we were for a time; I don't know if we
> still do), or if we ever run Gecko as non-root.
> 
> Instead, perhaps you can add a static method to ContentParent that gives you
> the PID of the current preallocated process, if it exists.
> 
> Actually, I think there are race conditions here if we ever reuse a PID.  I'm
> sure Gecko is already reaping its processes.  The right thing to do would be
> to
> use the ChildID() of the relevant ContentParent.  That value is unique.  We'd
> have to change hal to accept this value instead of the PID.

I'll work on above once below mentioned bug 844323 has done.

> I don't think we can do this unless we also add values corresponding to these
> new oom_adj's to
> 
>     /sys/module/lowmemorykiller/parameters/adj and
>     /sys/module/lowmemorykiller/parameters/minfree.
> 
> This work will be a lot easier once I finish bug 844323.  Since that bug is
> must-have, while this is nice-to-have, I'm happy to wait for that bug to land
> before working on this, if you're OK with that.
Attached patch WIP, v2 (obsolete) (deleted) — — Splinter Review 
Revised patch according to reviewer's comment 18. This is WIP patch which needs further modification. Waiting for bug 844323.
Attachment #727042 - Attachment is obsolete: true
Alan, that's an interesting idea!  Let's experiment with it and see how it goes.
Please let me (or anyone UX know) if any additional UX is needed here, or if Gordon's approach in comment 15 is what we're going with. Thanks!
Flags: in-moztrap?
It seem I need to apply attachment 742499 [details] [diff] [review] from bug 844323 to make bg -> fg works normally.
Hi Justin,
Intend to know pid and/or childID of preallocated process, this patch add API in PreallocatedProcessManager. Can you help to review this? Many thanks.
Attachment #743514 - Flags: review?(justin.lebar+bug)
Hi Justin,
I implement an LRU pool for background app processes, like comment 16. Can you help to review this patch? Many thanks!
Attachment #728042 - Attachment is obsolete: true
Attachment #743519 - Flags: review?(justin.lebar+bug)
Depends on: 844323
No longer depends on: 856240
(In reply to Alan Huang [:ahuang] from comment #25)
> Hi Justin,
> I implement an LRU pool for background app processes, like comment 16. Can
> you help to review this patch? Many thanks!

It is a typo. I mean comment 19.
Why do we want to limit the number of apps that can run in the background?  This seems unnecessary to me; the OS is perfectly capable of killing bg apps when we're low on memory.  And remember that the limit encoded here will apply to all phones, regardless of how much memory they have.

If you think the hard limit on the number of background apps is a good idea, would you mind proposing the out-of-background killer in a separate bug?
Comment on attachment 743519 [details] [diff] [review]
Implement an LRU pool for background app processes.

A big problem with this patch is that processes which die are never removed from the LRU list.

It might be easier to implement this functionality in the ProcessPriorityManager, since it already keeps track of when processes are created / destroyed.  That way, hal could remain "dumb" and not implement any application logic.

The trick is that hal::SetProcessPriority would now need a pid, a ProcessPriority, and some indication of which LRU level to give to the process, if the process is in the bg.  Maybe we should make a ProcessPriorityWithLRU struct and make hal::SetProcessPriority take that instead of a plain ProcessPriority.

Also, please have a look again at the style comments I gave earlier in this bug.  This patch is a big improvement stylistically, but there are still some long lines, some spaces missing between operators, and some other style issues.

I'm also not wild about doing complex math in here that we have to verify works under all conditions, if we have a way to avoid doing that.

We'll want to write tests for this, now that we have the capability of doing so.  See the tests in dom/browser-element/mochitest/priority.
Attachment #743519 - Flags: review?(justin.lebar+bug) → review-
Attachment #743514 - Flags: review?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #28)
> Comment on attachment 743519 [details] [diff] [review]
> Implement an LRU pool for background app processes.
> 
> A big problem with this patch is that processes which die are never removed
> from the LRU list.
> 
> It might be easier to implement this functionality in the
> ProcessPriorityManager, since it already keeps track of when processes are
> created / destroyed.  That way, hal could remain "dumb" and not implement
> any application logic.

Actually this patch does remove already dead processes. Below code is used to do it.
+        // Send signal 0 to check status of target process.
+        // If the process is already dead, clean up its room.
+        if (kill((*sBackgroundLRUPool)[i], 0) != 0) {
+          (*sBackgroundLRUPool)[i] = 0;

But I think your idea is good. It is better to let ProcessPriorityManager be resposible for the calculation and oom_adj algorithm. I will start to do this in ProcessPriorityManager instead of GonkHal

> 
> The trick is that hal::SetProcessPriority would now need a pid, a
> ProcessPriority, and some indication of which LRU level to give to the
> process, if the process is in the bg.  Maybe we should make a
> ProcessPriorityWithLRU struct and make hal::SetProcessPriority take that
> instead of a plain ProcessPriority.

So you are suggesting that we should change 
hal::SetProcessPriority(int aPid, ProcessPriority aPriority) to hal::SetProcessPriority(int aPid, ProcessPriorityWithLRU aPriorityWithLRU), and define struct ProcessPriorityWithLRU contains ProcessPriority and LRU order? It is indeed much simpler in the part of writing oom_adj. (oomAdj + LRU order)

Also, should we also modify oom_score_adj considering LRU? If we foucus on the order android LMK kill process only, then we don't, right?

> 
> Also, please have a look again at the style comments I gave earlier in this
> bug.  This patch is a big improvement stylistically, but there are still
> some long lines, some spaces missing between operators, and some other style
> issues.

Ok, I found that I missed the newly added lines of Logarithm and HAL_LOG. Thanks for reminding.

> 
> I'm also not wild about doing complex math in here that we have to verify
> works under all conditions, if we have a way to avoid doing that.
> 
> We'll want to write tests for this, now that we have the capability of doing
> so.  See the tests in dom/browser-element/mochitest/priority.
(In reply to Justin Lebar [:jlebar] from comment #27)
> Why do we want to limit the number of apps that can run in the background? 
> This seems unnecessary to me; the OS is perfectly capable of killing bg apps
> when we're low on memory.  And remember that the limit encoded here will
> apply to all phones, regardless of how much memory they have.
> 
> If you think the hard limit on the number of background apps is a good idea,
> would you mind proposing the out-of-background killer in a separate bug?

I think I will propose this in a separate bug. Actually the parameters: *KillUnderMB and backgroundLRUPoolLevels should change according to different phone specs, like RAM size and resolution. OEM could modify b2g.js according to their different phone specs. In OEM's test cases of android-kernel-based phones, the hard limit on the number of background apps indeed helps in getting better power consumption and performance (related to the rate of LMK selecting processes). So I think we can add this in b2g, and keep the decision of turning it on or off open to OEMs.
> Actually this patch does remove already dead processes. Below code is used to do it.

Oh, I see.  You're right.

But what if we've already reaped this process?  Then this has a race condition, right?  I'm pretty sure we already reap our child processes in Gecko.

> So you are suggesting that we should change 
> hal::SetProcessPriority(int aPid, ProcessPriority aPriority) to hal::SetProcessPriority(int aPid, 
> ProcessPriorityWithLRU aPriorityWithLRU), and define struct ProcessPriorityWithLRU contains 
> ProcessPriority and LRU order?

Yes, that's what I was thinking.  But take it with a grain of salt, since I'm not writing the patch; you might have a better idea.

> Also, should we also modify oom_score_adj considering LRU? If we foucus on the order android LMK 
> kill process only, then we don't, right?

As I recall, we only write one of oom_adj or oom_score_adj, and we let the kernel translate to the other.  Whatever we write to /proc should be affected by the LRU logic, I think?
Hi Justin,
I think this is more likely we we want here: let ParticularProcessPriorityManager to add/remove processes into/from LRU pool, and GonkHal only sets process priority with LRU. I think this way is better than previous one. Can you help to give me some comments? Many thanks.
Attachment #743514 - Attachment is obsolete: true
Attachment #743519 - Attachment is obsolete: true
Attachment #746753 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 746753 [details] [diff] [review]
Implement an LRU pool for background app processes. (in PPPM)

Hi, Alan.

I'm sorry, but I've bitrotted you somewhat in bug 870181.

This looks like it's on the right track, but we need to make some changes,
aside from reworking things around the changes made in bug 870181.

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp

>+  static int32_t sBackgroundLRUPoolLevels;
>+  static StaticAutoPtr<nsTArray<ContentParent*> > sBackgroundLRUPool;
>+
>+  static void EnsureBackgroundLRUPool();
>+  static void RemoveFromBackgroundLRUPool(ContentParent* aContentParent);
>+  static void AddIntoBackgroundLRUPool(ContentParent* aContentParent);

These static functions should instead live on ProcessPriorityManager.  Its
purpose is to manage the ParticularProcessPriorityManagers.

>@@ -347,17 +363,18 @@ ProcessPriorityManagerImpl::ProcessPrior
>-  hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER);
>+  hal::SetProcessPriority(getpid(),
>+    ProcessPriorityWithLRU(PROCESS_PRIORITY_MASTER, 0));

A non-zero LRU only makes sense with PROCESS_PRIORITY_BACKGROUND (I think?), so
we could leave off the 0 here.  In fact, we could leave out the constructor
here altogether, if you wanted, and have an implicit conversion from
ProcessPriority to ProcessPriorityWithLRU.  We'd only need the explicit
constructor for BACKGROUND priorities.

>@@ -403,16 +420,27 @@ ProcessPriorityManagerImpl::SetProcessPr
> void
>+ProcessPriorityManagerImpl::SetProcessPriorityWithLRU(
>+    ContentParent* aContentParent,
>+    ProcessPriorityWithLRU aPriorityWithLRU)
>+{
>+  MOZ_ASSERT(aContentParent);
>+  nsRefPtr<ParticularProcessPriorityManager> pppm =
>+    GetParticularProcessPriorityManager(aContentParent);
>+  pppm->SetPriorityWithLRUNow(aPriorityWithLRU);
>+}

This is only called by ParticularProcessPriorityManager, and then this calls
straight back into the PPPM.  So I don't think we need this function.

>+  // We already got a candidate. Shift the list in pool.
>+  for (int i = (availableIndex + ((1 << sBackgroundLRUPoolLevels) - 1))
>+             % ((1 << sBackgroundLRUPoolLevels) - 1); i > 0; i--) {
>+    (*sBackgroundLRUPool)[i] = (*sBackgroundLRUPool)[i - 1];
>+    // Check whether i+1 is power of Two.
>+    // If so, then we need to assign its new process priority LRU.
>+    if (!((i + 1) & i)) {
>+      ProcessPriorityManagerImpl::GetSingleton()->SetProcessPriorityWithLRU(
>+        (*sBackgroundLRUPool)[i],
>+        ProcessPriorityWithLRU(PROCESS_PRIORITY_BACKGROUND,
>+            (int)(log(i + 1.0) / log(2.0))
>+        )
>+      );
>+    }
>+  }

The logic for the LRU pool is much more complicated than I'd like.  While I
have confidence in your ability to do math, I don't have confidence that
someone (maybe me) will not accidentally break this.

It seems to me that this LRU pool is actually its own data structure.  Perhaps
if we encapsulated it as its own class, then we could write tests for it and be
more confident in its correctness.

>@@ -790,21 +927,34 @@ ParticularProcessPriorityManager::SetPri
>+  if (aPriority == PROCESS_PRIORITY_BACKGROUND
>+      && mPriority != PROCESS_PRIORITY_BACKGROUND
>+      && !IsPreallocated()) {
>+      AddIntoBackgroundLRUPool(mContentParent);
>+  }
>+
>+  if (aPriority != PROCESS_PRIORITY_BACKGROUND
>+      && mPriority == PROCESS_PRIORITY_BACKGROUND
>+      && !IsPreallocated()) {
>+      RemoveFromBackgroundLRUPool(mContentParent);
>+  }

Operators go at the end of the line when we break a long line.

>   LOGP("Changing priority from %s to %s.",
>        ProcessPriorityToString(mPriority),
>        ProcessPriorityToString(aPriority));
>   mPriority = aPriority;
>-  hal::SetProcessPriority(Pid(), mPriority);
>+
>+  SetPriorityWithLRUNow(ProcessPriorityWithLRU(mPriority, 0));

Why is this 0 and not the LRU value?

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>--- a/hal/Hal.cpp
>+++ b/hal/Hal.cpp

>@@ -1021,16 +1022,33 @@ OomAdjOfOomScoreAdj(int aOomScoreAdj)
>+static int
>+OomScoreAdjOfOomAdj(int aAdj)
>+{
>+  // Convert OOM adjustment from the domain of /proc/<pid>/oom_adj
>+  // to the domain of /proc/<pid>/oom_score_adj.
>+
>+  int aOomScoreAdj;
>+
>+  if (aAdj < 0) {
>+    aOomScoreAdj = ceil(((float)OOM_SCORE_ADJ_MIN / OOM_DISABLE) * aAdj);
>+  } else {
>+    aOomScoreAdj = ceil(((float)OOM_SCORE_ADJ_MAX / OOM_ADJUST_MAX) * aAdj);
>+  }
>+
>+  return aOomScoreAdj;
>+}

I'm not certain that this is correct.  I recall looking at the kernel code and
noticing that the function the kernel uses to convert from oom_score_adj to
oom_adj is not an inverse of the function that the kernel uses to convert from
oom_adj to oom_score_adj.

You can test this yourself on a phone by manually setting a process's oom_adj
and reading its oom_score_adj.

>@@ -1182,31 +1200,31 @@ SetNiceForPid(int aPid, int aNice)
>-SetProcessPriority(int aPid, ProcessPriority aPriority)
>+SetProcessPriority(int aPid, ProcessPriorityWithLRU aPriorityWithLRU)
>-    int clampedOomScoreAdj = clamped<int>(oomScoreAdj, OOM_SCORE_ADJ_MIN,
>-                                                       OOM_SCORE_ADJ_MAX);
>+    int clampedOomScoreAdj =
>+      clamped<int>(oomScoreAdj + OomScoreAdjOfOomAdj(aPriorityWithLRU.mLRU),
>+                   OOM_SCORE_ADJ_MIN, OOM_SCORE_ADJ_MAX);

I guess the idea here is that we should add the smallest value to the
oom_score_adj such that we get a different oom_adj?  It's not clear to me that
this actually works, because of the weirdness about converting to and from
oom_adj.  We should check to make sure.

Also, we should probably be asserting here that aPriorityWithLRU.mPriority is
BACKGROUND if mLRU is non-zero.  Perhaps the PriorityWithLRU class can check
this.

I think we must modify EnsureKernelLowMemKillerParamsSet() so that every
oom_adj that we can give to a process is written in
lowmemorykiller/parameters/adj and lowmemorykiller/parameters/minfree.  (I say
"I think" because I haven't checked the lowmemorykiller code to see what it
does when an oom_adj isn't listed here.  You may want to check the kernel
sources to see; they're in the B2G checkout.)

If we do have to write each of the oom_adj's to this file, then that may be a
good time to ensure that each of the LRU levels gets a different oom_adj.

This patch will need tests.  We can work on the tests in parallel with reviews
on the code itself.  The existing tests for this code are in
dom/browser-element/mochitest/priority.
Attachment #746753 - Flags: feedback?(justin.lebar+bug) → feedback+
(In reply to Justin Lebar [:jlebar] from comment #33)
> Comment on attachment 746753 [details] [diff] [review]
> Implement an LRU pool for background app processes. (in PPPM)

Hi, Justin.

> 
> Hi, Alan.
> 
> I'm sorry, but I've bitrotted you somewhat in bug 870181.
> 
> This looks like it's on the right track, but we need to make some changes,
> aside from reworking things around the changes made in bug 870181.

Ok, I will look at bug 870181 and see how should I modify the patch.

> 
> >diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
> >--- a/dom/ipc/ProcessPriorityManager.cpp
> >+++ b/dom/ipc/ProcessPriorityManager.cpp
> 
> >+  static int32_t sBackgroundLRUPoolLevels;
> >+  static StaticAutoPtr<nsTArray<ContentParent*> > sBackgroundLRUPool;
> >+
> >+  static void EnsureBackgroundLRUPool();
> >+  static void RemoveFromBackgroundLRUPool(ContentParent* aContentParent);
> >+  static void AddIntoBackgroundLRUPool(ContentParent* aContentParent);
> 
> These static functions should instead live on ProcessPriorityManager.  Its
> purpose is to manage the ParticularProcessPriorityManagers.

Ok, this is reasonable. I will put them in ProcessPriorityManager.

> 
> >@@ -347,17 +363,18 @@ ProcessPriorityManagerImpl::ProcessPrior
> >-  hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER);
> >+  hal::SetProcessPriority(getpid(),
> >+    ProcessPriorityWithLRU(PROCESS_PRIORITY_MASTER, 0));
> 
> A non-zero LRU only makes sense with PROCESS_PRIORITY_BACKGROUND (I think?),
> so
> we could leave off the 0 here.  In fact, we could leave out the constructor
> here altogether, if you wanted, and have an implicit conversion from
> ProcessPriority to ProcessPriorityWithLRU.  We'd only need the explicit
> constructor for BACKGROUND priorities.

To make the structures in hal and ProcessPriorityMamager simple, I now want to use ProcessPriority and LRU in seperate auguments instead of using ProcessPriorityWithLRU. I think just define 
ProcessPriorityManagerImpl::SetProcessPriority(ContentParent* aContentParent,
                          ProcessPriority aPriority,
                          uint32_t aLRU = 0) and
hal::SetProcessPriority(int aPid, hal::ProcessPriority aPriority, uint32_t aLRU = 0)
could still get what we want here, and makes more things much simpler. Those don't want to set LRU coud just call SetProcessPriority as before.

> 
> >@@ -403,16 +420,27 @@ ProcessPriorityManagerImpl::SetProcessPr
> > void
> >+ProcessPriorityManagerImpl::SetProcessPriorityWithLRU(
> >+    ContentParent* aContentParent,
> >+    ProcessPriorityWithLRU aPriorityWithLRU)
> >+{
> >+  MOZ_ASSERT(aContentParent);
> >+  nsRefPtr<ParticularProcessPriorityManager> pppm =
> >+    GetParticularProcessPriorityManager(aContentParent);
> >+  pppm->SetPriorityWithLRUNow(aPriorityWithLRU);
> >+}
> 
> This is only called by ParticularProcessPriorityManager, and then this calls
> straight back into the PPPM.  So I don't think we need this function.

After moved the static functions to ProcessPriorityManager and re-define ProcessPriorityManagerImpl::SetProcessPriority, it is fine to remove this. I will do it.

> 
> >+  // We already got a candidate. Shift the list in pool.
> >+  for (int i = (availableIndex + ((1 << sBackgroundLRUPoolLevels) - 1))
> >+             % ((1 << sBackgroundLRUPoolLevels) - 1); i > 0; i--) {
> >+    (*sBackgroundLRUPool)[i] = (*sBackgroundLRUPool)[i - 1];
> >+    // Check whether i+1 is power of Two.
> >+    // If so, then we need to assign its new process priority LRU.
> >+    if (!((i + 1) & i)) {
> >+      ProcessPriorityManagerImpl::GetSingleton()->SetProcessPriorityWithLRU(
> >+        (*sBackgroundLRUPool)[i],
> >+        ProcessPriorityWithLRU(PROCESS_PRIORITY_BACKGROUND,
> >+            (int)(log(i + 1.0) / log(2.0))
> >+        )
> >+      );
> >+    }
> >+  }
> 
> The logic for the LRU pool is much more complicated than I'd like.  While I
> have confidence in your ability to do math, I don't have confidence that
> someone (maybe me) will not accidentally break this.
> 
> It seems to me that this LRU pool is actually its own data structure. 
> Perhaps
> if we encapsulated it as its own class, then we could write tests for it and
> be
> more confident in its correctness.

I agree with your concern and I think your suggestion is good. So, I will encapsulate the LRU pool and all its calculations to a new class in ProcessPriorityManager.cpp. 

> 
> >@@ -790,21 +927,34 @@ ParticularProcessPriorityManager::SetPri
> >+  if (aPriority == PROCESS_PRIORITY_BACKGROUND
> >+      && mPriority != PROCESS_PRIORITY_BACKGROUND
> >+      && !IsPreallocated()) {
> >+      AddIntoBackgroundLRUPool(mContentParent);
> >+  }
> >+
> >+  if (aPriority != PROCESS_PRIORITY_BACKGROUND
> >+      && mPriority == PROCESS_PRIORITY_BACKGROUND
> >+      && !IsPreallocated()) {
> >+      RemoveFromBackgroundLRUPool(mContentParent);
> >+  }
> 
> Operators go at the end of the line when we break a long line.

Got it, thanks!

> 
> >   LOGP("Changing priority from %s to %s.",
> >        ProcessPriorityToString(mPriority),
> >        ProcessPriorityToString(aPriority));
> >   mPriority = aPriority;
> >-  hal::SetProcessPriority(Pid(), mPriority);
> >+
> >+  SetPriorityWithLRUNow(ProcessPriorityWithLRU(mPriority, 0));
> 
> Why is this 0 and not the LRU value?

No matter it is a newly added BACKGROUND process or process with other priority, none needs LRU value here. I will revise the code here without using ProcessPriorityWithLRU.

> 
> >diff --git a/hal/Hal.cpp b/hal/Hal.cpp
> >--- a/hal/Hal.cpp
> >+++ b/hal/Hal.cpp
> 
> >@@ -1021,16 +1022,33 @@ OomAdjOfOomScoreAdj(int aOomScoreAdj)
> >+static int
> >+OomScoreAdjOfOomAdj(int aAdj)
> >+{
> >+  // Convert OOM adjustment from the domain of /proc/<pid>/oom_adj
> >+  // to the domain of /proc/<pid>/oom_score_adj.
> >+
> >+  int aOomScoreAdj;
> >+
> >+  if (aAdj < 0) {
> >+    aOomScoreAdj = ceil(((float)OOM_SCORE_ADJ_MIN / OOM_DISABLE) * aAdj);
> >+  } else {
> >+    aOomScoreAdj = ceil(((float)OOM_SCORE_ADJ_MAX / OOM_ADJUST_MAX) * aAdj);
> >+  }
> >+
> >+  return aOomScoreAdj;
> >+}
> 
> I'm not certain that this is correct.  I recall looking at the kernel code
> and
> noticing that the function the kernel uses to convert from oom_score_adj to
> oom_adj is not an inverse of the function that the kernel uses to convert
> from
> oom_adj to oom_score_adj.
> 
> You can test this yourself on a phone by manually setting a process's oom_adj
> and reading its oom_score_adj.

Converting from oom_score_adj to oom_adj is not an inverse of converting from oom_adj to oom_score_adj. In kernel code,
oom_score_adj -> oom_adj is (OOM_ADJUST_MAX    / OOM_SCORE_ADJ_MAX) * oom_score_adj;
and 
oom_adj -> oom_score_adj is (OOM_SCORE_ADJ_MAX / -OOM_DISABLE)      * oom_adj

But if we use above function, we won't get what we want now. This is because this part kernel code uses interger operation (floor) instead of rounding or ceiling. For example in unagi:
if we write oom_adj = 7, I will get oom_score_adj = floor(1000 / 17 * 7) = 411. But I could only get oom_adj = 7 if I write oom_score_adj = 467. That's why I use the inverse function (+ceiling) of OomAdjOfOomScoreAdj as OomScoreAdjOfOomAdj.

I think this will be still good in later kernel version, which uses oom_score_adj as its reference for lowmemorykiller.

> 
> >@@ -1182,31 +1200,31 @@ SetNiceForPid(int aPid, int aNice)
> >-SetProcessPriority(int aPid, ProcessPriority aPriority)
> >+SetProcessPriority(int aPid, ProcessPriorityWithLRU aPriorityWithLRU)
> >-    int clampedOomScoreAdj = clamped<int>(oomScoreAdj, OOM_SCORE_ADJ_MIN,
> >-                                                       OOM_SCORE_ADJ_MAX);
> >+    int clampedOomScoreAdj =
> >+      clamped<int>(oomScoreAdj + OomScoreAdjOfOomAdj(aPriorityWithLRU.mLRU),
> >+                   OOM_SCORE_ADJ_MIN, OOM_SCORE_ADJ_MAX);
> 
> I guess the idea here is that we should add the smallest value to the
> oom_score_adj such that we get a different oom_adj?  It's not clear to me
> that
> this actually works, because of the weirdness about converting to and from
> oom_adj.  We should check to make sure.
> 
> Also, we should probably be asserting here that aPriorityWithLRU.mPriority is
> BACKGROUND if mLRU is non-zero.  Perhaps the PriorityWithLRU class can check
> this.
> 
> I think we must modify EnsureKernelLowMemKillerParamsSet() so that every
> oom_adj that we can give to a process is written in
> lowmemorykiller/parameters/adj and lowmemorykiller/parameters/minfree.  (I
> say
> "I think" because I haven't checked the lowmemorykiller code to see what it
> does when an oom_adj isn't listed here.  You may want to check the kernel
> sources to see; they're in the B2G checkout.)
> 
> If we do have to write each of the oom_adj's to this file, then that may be a
> good time to ensure that each of the LRU levels gets a different oom_adj.
> 

Yes, I want to add the smallest value to the oom_score_adj so we could get a different oom_adj. We cannot write all oom_adj we can give to a process in lowmemorykiller/parameters/adj and lowmemorykiller/parameters/minfree, because the data structure in lowmemorykiller.c only accept 6 pairs. We already use 6 priorities and no more left. We can write more, but those won't take effects.

I did test on unagi and checked ICS 4.0.4/JB MR1.1's lowmemorykiller.c, the algorithms in lowmem_shrink() could both work if process got an oom_adj isn't listed in /sys/module/lowmemorykiller/parameters/adj.

And thanks for the suggestion, I will add assertion if aLRU is greater than 0 but aPriority is not PROCESS_PRIORITY_BACKGROUND in hal::SetProcessPriority at Hal.cpp.

> This patch will need tests.  We can work on the tests in parallel with
> reviews
> on the code itself.  The existing tests for this code are in
> dom/browser-element/mochitest/priority.

Sure, I did tests without using mochitest before. I only used "b2g-ps --oom" for checking. I will also write tests, too.
WIP. I still need to encapsulate the LRU pool.
Attachment #746753 - Attachment is obsolete: true
> To make the structures in hal and ProcessPriorityMamager simple, I now want
> to use ProcessPriority and LRU in seperate auguments instead of using
> ProcessPriorityWithLRU. I think just define 
>
> ProcessPriorityManagerImpl::SetProcessPriority(ContentParent* aContentParent,
>                           ProcessPriority aPriority,
>                           uint32_t aLRU = 0) and
> hal::SetProcessPriority(int aPid, hal::ProcessPriority aPriority, uint32_t aLRU = 0)
>
> could still get what we want here, and makes more things much simpler. Those
> [that] don't want to set LRU coud just call SetProcessPriority as before.

Okay.  I'm happy to reserve judgement until I look at the patch!

> That's why I use the inverse function (+ceiling) of OomAdjOfOomScoreAdj as
> OomScoreAdjOfOomAdj.

Okay.  The problem is that OomAdjOfOomScoreAdj follows the kernel's
conventions, while OomScoreAdjOfOomAdj does not.  That's confusing.

Can you add a comment to this function indicating what it does?  We should also
change its name.

Perhaps a better thing to do would be to add a constant which is equal to
OomScoreAdjOfOomAdj(1), or to add a function which rounds an oom_score_adj up
to the next oom_adj.

> We already use 6 priorities and no more left. We can write more, but those
> won't take effects.

I see.  And I see now that the kernel applies the policy from the last
(minfree, adj) pair for these high-oom_adj processes, which is exactly what we
want.  Thanks for checking.
We still need test case for it
Attachment #749202 - Attachment is obsolete: true
At this point, too much risk to block. We'll look at an approval nomination instead of block once this is completed.
blocking-b2g: leo+ → -
Flags: in-moztrap?
Below are oom_adj and oom_score_adj info, without and with this patch. Usage and Homescreen are launched by default. I launched apps in below order:
Camera, Gallery, FM Radio, Settings, Marketplace, Test Sensors, Device Storage, Membuster, Test receiver, Test Receiver#2, Test Container, UI tests, Template, Share Receiver, Test Agent,  Test receiver#1.

Without this patch:
> $ adb shell b2g-ps --oom
> APPLICATION       OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g                  0        149         0          root      107   1     172724 77924 ffffffff 400b9330 S /system/b2g/b2g
> Usage                6        456         400        app_344   344   107   65728  24456 ffffffff 40032330 S /system/b2g/plugin-container
> Homescreen           2        199         134        app_371   371   107   79940  28268 ffffffff 40071330 S /system/b2g/plugin-container
> Camera               6        459         400        app_429   429   107   70592  25552 ffffffff 4001d330 S /system/b2g/plugin-container
> Gallery              6        456         400        app_482   482   107   68608  24372 ffffffff 400c4330 S /system/b2g/plugin-container
> FM Radio             6        457         400        app_498   498   107   67536  24832 ffffffff 4009a330 S /system/b2g/plugin-container
> Settings             6        459         400        app_507   507   107   70656  25844 ffffffff 400966ec R /system/b2g/plugin-container
> Marketplace          6        453         400        app_509   509   107   66336  23160 ffffffff 40055330 S /system/b2g/plugin-container
> Test Sensors         6        449         400        app_533   533   107   65284  22556 ffffffff 400f2330 S /system/b2g/plugin-container
> Device Storage       6        453         400        app_542   542   107   65340  23324 ffffffff 40fbc2a8 R /system/b2g/plugin-container
> Membuster            6        452         400        app_553   553   107   65280  22612 ffffffff 400cb330 S /system/b2g/plugin-container
> Test receiver (      6        452         400        app_563   563   107   65284  22884 ffffffff 405fc690 R /system/b2g/plugin-container
> Test Receiver#2      6        451         400        app_573   573   107   65212  21900 ffffffff 412c84a2 R /system/b2g/plugin-container
> Test Container       6        453         400        app_584   584   107   65212  22840 ffffffff 4136d404 R /system/b2g/plugin-container
> UI tests             6        451         400        app_596   596   107   65212  21924 ffffffff 401166ec R /system/b2g/plugin-container
> Template             6        451         400        app_608   608   107   65212  22364 ffffffff 40084330 S /system/b2g/plugin-container
> Share Receiver       6        450         400        app_625   625   107   64252  21860 ffffffff 412e8f7c R /system/b2g/plugin-container
> Test Agent           6        444         400        app_640   640   107   60152  19208 ffffffff 4130c1e4 R /system/b2g/plugin-container
> Test receiver#1      6        450         400        app_649   649   107   65212  22004 ffffffff 4127404a R /system/b2g/plugin-container
> (Preallocated a      6        403         400        root      674   107   54960  14116 ffffffff 40f4618a R /system/b2g/plugin-container

With this patch:
> $ adb shell b2g-ps --oom
> APPLICATION       OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g                  0        135         0          root      108   1     159984 71644 ffffffff 400e5330 S /system/b2g/b2g
> Homescreen           2        202         134        app_397   397   108   81568  29508 ffffffff 400d0330 S /system/b2g/plugin-container
> Usage                10       722         667        app_423   423   108   64108  23832 ffffffff 4007c330 S /system/b2g/plugin-container
> Camera               10       722         667        app_464   464   108   68984  23876 ffffffff 400ce330 S /system/b2g/plugin-container
> Gallery              9        653         600        app_493   493   108   66744  23024 ffffffff 400b9330 S /system/b2g/plugin-container
> FM Radio             9        653         600        app_509   509   108   65720  22932 ffffffff 40073330 S /system/b2g/plugin-container
> Settings             9        659         600        app_526   526   108   69868  25752 ffffffff 400cd330 S /system/b2g/plugin-container
> Marketplace          9        651         600        app_527   527   108   63664  22452 ffffffff 40082330 S /system/b2g/plugin-container
> Test Sensors         9        650         600        app_559   559   108   64684  21748 ffffffff 4009c330 S /system/b2g/plugin-container
> Device Storage       9        653         600        app_574   574   108   64704  23156 ffffffff 400dc330 S /system/b2g/plugin-container
> Membuster            9        649         600        app_575   575   108   63656  21376 ffffffff 400a2330 S /system/b2g/plugin-container
> Test receiver (      9        649         600        app_600   600   108   63660  21232 ffffffff 40092330 S /system/b2g/plugin-container
> Test Receiver#2      8        584         534        app_620   620   108   64688  21600 ffffffff 400cb330 S /system/b2g/plugin-container
> Test Container       8        585         534        app_635   635   108   65652  22176 ffffffff 4012a330 S /system/b2g/plugin-container
> UI tests             8        586         534        app_650   650   108   64820  22520 ffffffff 4010b330 S /system/b2g/plugin-container
> Template             8        583         534        app_664   664   108   64680  21228 ffffffff 400b4330 S /system/b2g/plugin-container
> Share Receiver       7        518         467        app_679   679   108   64680  22040 ffffffff 4012a330 S /system/b2g/plugin-container
> Test Agent           7        519         467        app_680   680   108   64680  22788 ffffffff 400d7330 S /system/b2g/plugin-container
> Test receiver#1      6        452         400        app_710   710   108   64688  22648 ffffffff 400b8330 S /system/b2g/plugin-container
> (Preallocated a      6        411         400        root      725   108   58472  17568 ffffffff 412a84e0 R /system/b2g/plugin-container

Then launch some app in the order "Marketplace, Test Sensors, Device Storage, Membuster, Test receiver, Test Receiver#2, Test Container, UI tests" again. Suppose they will get smaller oom_adj and oom_score_adj now.

> $ adb shell b2g-ps --oom
> APPLICATION       OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g                  0        136         0          root      108   1     154808 72268 ffffffff 400e5330 S /system/b2g/b2g
> Homescreen           2        202         134        app_397   397   108   78208  29504 ffffffff 400d0330 S /system/b2g/plugin-container
> Usage                10       722         667        app_423   423   108   64108  23848 ffffffff 4007c330 S /system/b2g/plugin-container
> Camera               10       722         667        app_464   464   108   66936  23868 ffffffff 400ce330 S /system/b2g/plugin-container
> Gallery              9        653         600        app_493   493   108   64696  23016 ffffffff 400b9330 S /system/b2g/plugin-container
> FM Radio             9        653         600        app_509   509   108   64696  22928 ffffffff 40073330 S /system/b2g/plugin-container
> Settings             9        660         600        app_526   526   108   67820  26080 ffffffff 400cd330 S /system/b2g/plugin-container
> Marketplace          9        651         600        app_527   527   108   63664  22448 ffffffff 40082330 S /system/b2g/plugin-container
> Test Sensors         8        584         534        app_559   559   108   63660  21728 ffffffff 4009c330 S /system/b2g/plugin-container
> Device Storage       8        587         534        app_574   574   108   64704  23148 ffffffff 400dc330 S /system/b2g/plugin-container
> Membuster            8        583         534        app_575   575   108   62632  21364 ffffffff 400a2330 S /system/b2g/plugin-container
> Test receiver (      8        583         534        app_600   600   108   61612  21248 ffffffff 40092330 S /system/b2g/plugin-container
> Test Receiver#2      7        516         467        app_620   620   108   62640  21576 ffffffff 400cb330 S /system/b2g/plugin-container
> Test Container       7        518         467        app_635   635   108   63604  22168 ffffffff 4012a330 S /system/b2g/plugin-container
> UI tests             6        452         400        app_650   650   108   63796  22512 ffffffff 4010b330 S /system/b2g/plugin-container
> Template             9        649         600        app_664   664   108   62632  21220 ffffffff 400b4330 S /system/b2g/plugin-container
> Share Receiver       9        651         600        app_679   679   108   62632  22032 ffffffff 4012a330 S /system/b2g/plugin-container
> Test Agent           9        652         600        app_680   680   108   62632  22780 ffffffff 400d7330 S /system/b2g/plugin-container
> Test receiver#1      9        649         600        app_710   710   108   62640  21592 ffffffff 400b8330 S /system/b2g/plugin-container
> (Preallocated a      6        416         400        root      725   108   60532  19960 ffffffff 400ea330 S /system/b2g/plugin-container


Kill 'Test Receiver#2' and 'Test receiver' from card view.
> $ adb shell b2g-ps --oom
> APPLICATION       OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g                  0        137         0          root      108   1     153720 72312 ffffffff 400e5330 S /system/b2g/b2g
> Homescreen           2        202         134        app_397   397   108   78112  29504 ffffffff 400d0330 S /system/b2g/plugin-container
> Usage                10       722         667        app_423   423   108   64108  23864 ffffffff 4007c330 S /system/b2g/plugin-container
> Camera               10       722         667        app_464   464   108   66936  23876 ffffffff 400ce330 S /system/b2g/plugin-container
> Gallery              9        653         600        app_493   493   108   64696  23024 ffffffff 400b9330 S /system/b2g/plugin-container
> FM Radio             9        653         600        app_509   509   108   64696  22936 ffffffff 40073330 S /system/b2g/plugin-container
> Settings             9        661         600        app_526   526   108   67820  26384 ffffffff 400cd330 S /system/b2g/plugin-container
> Marketplace          9        651         600        app_527   527   108   62640  22456 ffffffff 40082330 S /system/b2g/plugin-container
> Test Sensors         8        584         534        app_559   559   108   63660  21736 ffffffff 4009c330 R /system/b2g/plugin-container
> Device Storage       8        587         534        app_574   574   108   63680  23156 ffffffff 400dc330 S /system/b2g/plugin-container
> Membuster            8        583         534        app_575   575   108   62632  21376 ffffffff 400a2330 S /system/b2g/plugin-container
> Test Container       7        518         467        app_635   635   108   63604  22180 ffffffff 4012a330 S /system/b2g/plugin-container
> UI tests             6        452         400        app_650   650   108   62772  22520 ffffffff 4010b330 S /system/b2g/plugin-container
> Template             9        649         600        app_664   664   108   62632  21228 ffffffff 400b4330 S /system/b2g/plugin-container
> Share Receiver       9        651         600        app_679   679   108   62632  22040 ffffffff 4012a330 S /system/b2g/plugin-container
> Test Agent           9        652         600        app_680   680   108   62632  22788 ffffffff 400d7330 S /system/b2g/plugin-container
> Test receiver#1      9        650         600        app_710   710   108   62640  21600 ffffffff 400b8330 S /system/b2g/plugin-container
> (Preallocated a      6        416         400        root      725   108   60532  19968 ffffffff 400ea330 S /system/b2g/plugin-container

Then launch 'Test Receiver#2' and 'Test receiver' again. Suppose oom_adj and oom_score_adj of apps which have LRU larger than those two before shouldn't change.
> $ adb shell b2g-ps --oom
> APPLICATION       OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g                  0        140         0          root      108   1     159800 73784 ffffffff 41a77b9e R /system/b2g/b2g
> Homescreen           2        202         134        app_397   397   108   79168  29536 ffffffff 400d0330 S /system/b2g/plugin-container
> Usage                10       722         667        app_423   423   108   64108  23872 ffffffff 4007b6ec R /system/b2g/plugin-container
> Camera               10       722         667        app_464   464   108   67960  23880 ffffffff 400ce330 S /system/b2g/plugin-container
> Gallery              9        653         600        app_493   493   108   65720  23028 ffffffff 400b9330 S /system/b2g/plugin-container
> FM Radio             9        653         600        app_509   509   108   64696  22936 ffffffff 40073330 S /system/b2g/plugin-container
> Settings             9        661         600        app_526   526   108   68844  26392 ffffffff 40092f76 R /system/b2g/plugin-container
> Marketplace          9        651         600        app_527   527   108   62640  22456 ffffffff 40082330 S /system/b2g/plugin-container
> Test Sensors         8        584         534        app_559   559   108   63660  21736 ffffffff 4009c330 R /system/b2g/plugin-container
> Device Storage       8        587         534        app_574   574   108   63680  23156 ffffffff 400dc330 S /system/b2g/plugin-container
> Membuster            8        583         534        app_575   575   108   62632  21376 ffffffff 400a2330 S /system/b2g/plugin-container
> Test Container       8        585         534        app_635   635   108   63604  22180 ffffffff 4012a330 S /system/b2g/plugin-container
> UI tests             7        519         467        app_650   650   108   62772  22520 ffffffff 4010b330 S /system/b2g/plugin-container
> Template             9        649         600        app_664   664   108   62632  21228 ffffffff 400b4330 S /system/b2g/plugin-container
> Share Receiver       9        651         600        app_679   679   108   62632  22040 ffffffff 4012a330 S /system/b2g/plugin-container
> Test Agent           9        652         600        app_680   680   108   62632  22788 ffffffff 400d7330 S /system/b2g/plugin-container
> Test receiver#1      9        650         600        app_710   710   108   62640  21600 ffffffff 400b8330 S /system/b2g/plugin-container
> Test Receiver#2      7        516         467        app_725   725   108   63664  21416 ffffffff 400ea330 S /system/b2g/plugin-container
> Test receiver (      6        452         400        app_1505  1505  108   63660  22620 ffffffff 40037330 S /system/b2g/plugin-container
> (Preallocated a      6        418         400        root      1524  108   62580  21112 ffffffff 40082330 S /system/b2g/plugin-container

I also wrote a test case for this.
TEST-PASS | unknown test url | Got new process, id=1
TEST-PASS | unknown test url | Expected priority of childID 1 to change to FOREGROUND
TEST-PASS | unknown test url | Expected priority of childID 1 to change to BACKGROUND
TEST-PASS | unknown test url | Expected backgroundLRU 1 of childID 1 to change to 1
Attachment #750373 - Attachment is obsolete: true
Attachment #773839 - Flags: review?(justin.lebar+bug)
I'm at a work week, so I probably won't be able to have a look at this until Monday or so.
Blocks: 899451
leo+ requested per bug 899451.
blocking-b2g: - → leo?
Given https://bugzilla.mozilla.org/show_bug.cgi?id=900320#c4 and this isn't a regression, so non-blocking at this time.
blocking-b2g: leo? → -
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Unblocking on bug 899451, because I unduped this from bug 900320.
No longer blocks: 899451
Comment on attachment 773839 [details] [diff] [review]
Implement an LRU pool for background app processes.

My last day at Mozilla is Friday, and I don't think I'm going to have time to get to this before them.  Kyle will need to figure out who's going to own this code.
Attachment #773839 - Flags: review?(justin.lebar+bug) → review?(khuey)
Just to set expectations I am unlikely to get to this before next week.

Justin and I are sorry this patch has waited so long.
Hello Kyle,

Will you review this recently, or would you help me find someone who could help us? Thanks.
Blocks: 913893
Status: NEW → ASSIGNED
Flags: needinfo?(khuey)
Blocks: 914728
Comment on attachment 773839 [details] [diff] [review]
Implement an LRU pool for background app processes.

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

This looks pretty good.  I r-d it for some weird stuff with preferences, but I didn't see much else that was scary.

::: b2g/app/b2g.js
@@ +579,5 @@
>  pref("dom.ipc.processPriorityManager.temporaryPriorityLockMS", 5000);
>  
> +//Number of different background levels for background processes.  We use
> +//these different levels to force the low-memory killer to kill processes in
> +//a LRU order.

nit: put a space between // and the text

::: dom/ipc/ProcessPriorityManager.cpp
@@ +304,5 @@
>    nsCOMPtr<nsITimer> mResetPriorityTimer;
>  };
>  
> +class BackgroundProcessLRUPool MOZ_FINAL
> +  : public nsISupports

This doesn't need to inherit from nsISupports.

@@ +310,5 @@
> +public:
> +  static void StaticInit();
> +  static BackgroundProcessLRUPool* Singleton();
> +
> +  NS_DECL_ISUPPORTS

So you can get rid of this.

@@ +328,5 @@
> +private:
> +
> +  static bool sInitialized;
> +  static bool sPrefListenersRegistered;
> +  static StaticRefPtr<BackgroundProcessLRUPool> sSingleton;

And just change this to StaticAutoPtr.

@@ +335,5 @@
> +
> +  int32_t mLRUPoolLevels;
> +  uint32_t mLRUPoolSize;
> +  uint32_t mLRUPoolAvailableIndex;
> +  nsAutoPtr<nsTArray<ContentParent*> > mLRUPool;

I don't see any reason you need the nsAutoPtr here.  Just do nsTArray<ContentParent*> directly.  That'll also make the code less ugly (you won't need to do (*mLRUPool)[i] everywhere).

@@ +337,5 @@
> +  uint32_t mLRUPoolSize;
> +  uint32_t mLRUPoolAvailableIndex;
> +  nsAutoPtr<nsTArray<ContentParent*> > mLRUPool;
> +
> +  uint32_t CalculateLRULevel(uint32_t aBackgroudLRUPoolIndex);

*Background

@@ +339,5 @@
> +  nsAutoPtr<nsTArray<ContentParent*> > mLRUPool;
> +
> +  uint32_t CalculateLRULevel(uint32_t aBackgroudLRUPoolIndex);
> +
> +  NS_IMETHODIMP UpdateAvailableIndexInLRUPool(

s/NS_IMETHODIMP/nsresult/.

NS_IMETHODIMP is used when defining (not declaring) methods that were declared with NS_IMETHOD, like the methods on nsI* interfaces.

@@ +348,5 @@
> +
> +  void EnsureLRUPool();
> +
> +  BackgroundProcessLRUPool();
> +  ~BackgroundProcessLRUPool() {}

No need to specify a destructor if you're not going to put anything in it.

@@ +1210,5 @@
> +/* static */ StaticRefPtr<BackgroundProcessLRUPool>
> +  BackgroundProcessLRUPool::sSingleton;
> +
> +NS_IMPL_ISUPPORTS1(BackgroundProcessLRUPool,
> +                   nsISupports);

You won't need this anymore.

@@ +1218,5 @@
> +                                              void* aClosure)
> +{
> +  StaticInit();
> +  return 0;
> +}

I don't understand why this exists at all.  You only set up this preference observer if getting the preference at the start fails, but that should basically never happen.  I think you can get rid of this.

@@ +1257,5 @@
> +  // Priority background+2: LRU3, LRU4, LRU5, LRU6
> +  // Priority background+3: LRU7, LRU8, LRU9, LRU10, LRU11, LRU12, LRU13, LRU14
> +  // ...
> +  // Priority background+L-1: 2^(number of background LRU pool levels - 1)
> +  //(End of buffer)

nit: space after //

@@ +1280,5 @@
> +    if (!sPrefListenersRegistered) {
> +      sPrefListenersRegistered = true;
> +      Preferences::RegisterCallback(PrefChangedCallback,
> +        "dom.ipc.processPriorityManager.backgroundLRUPoolLevels");
> +    }

Then once you get rid of this, EnsureLRUPool cannot fail, so you can go back to the constructor and get rid of the if (!sInitialized) bit.

@@ +1288,5 @@
> +  if (mLRUPoolLevels <= 0) {
> +    MOZ_CRASH();
> +  }
> +
> +  //LRU pool size = 2^(number of background LRU pool levels)-1

nit: space between // and text

@@ +1289,5 @@
> +    MOZ_CRASH();
> +  }
> +
> +  //LRU pool size = 2^(number of background LRU pool levels)-1
> +  mLRUPoolSize = (uint32_t)((1 << mLRUPoolLevels) - 1);

Please add an assertion that mLRUPoolLevels is not too big.

MOZ_ASSERT(mLRUPoolLevels <= 31), I believe.

@@ +1295,5 @@
> +
> +  LOG("Making background LRU pool with size(%d)", mLRUPoolSize);
> +
> +  mLRUPool = new nsTArray<ContentParent*>();
> +  mLRUPool->InsertElementsAt(0, mLRUPoolSize, (ContentParent*)nullptr);

Actually you may want something smaller for the maximum value of mLRUPoolLevels because if someone tries 31 this is going to OOM ;-)

Also I doubt you need the ContentParent cast.

@@ +1299,5 @@
> +  mLRUPool->InsertElementsAt(0, mLRUPoolSize, (ContentParent*)nullptr);
> +
> +  sInitialized = true;
> +
> +}

nit: remove the \n at the end of this function

@@ +1322,5 @@
> +    }
> +  }
> +}
> +
> +NS_IMETHODIMP

nsresult

@@ +1332,5 @@
> +  // We have to make sure the index in LRUPool doesn't point to any
> +  // ContentParent.
> +  if (aTargetIndex >= 0 && aTargetIndex < mLRUPoolSize) {
> +    if (aTargetIndex < mLRUPoolAvailableIndex &&
> +        (*mLRUPool)[aTargetIndex] == nullptr) {

Why not fold these two if ()s together?

@@ +1341,5 @@
> +
> +  // When we didn't specify any legal aTargetIndex, then we just check
> +  // whether current mLRUPoolAvailableIndex points to any ContentParent or not.
> +  if (mLRUPoolAvailableIndex >= 0 && mLRUPoolAvailableIndex < mLRUPoolSize) {
> +    if (!((*mLRUPool)[mLRUPoolAvailableIndex])) {

Same here.

@@ +1357,5 @@
> +  for (uint32_t i = 0; i < mLRUPoolSize; i++) {
> +    if ((*mLRUPool)[i]) {
> +      if ((*mLRUPool)[i]->ChildID() == aContentParent->ChildID()) {
> +        LOG("ChildID(%llu) already in LRU pool", aContentParent->ChildID());
> +        return NS_ERROR_UNEXPECTED;

Does this actually happen?  If not it would be nice to MOZ_ASSERT it instead of returning NS_ERROR_UNEXPECTED.

@@ +1370,5 @@
> +
> +  // If the LRUPool is already full, then we should set mLRUPoolAvailableIndex
> +  // to mLRUPoolSize-1. Here uses the mod operator to do it.
> +  mLRUPoolAvailableIndex =
> +    (mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize;

Either the comment or the code or wrong, or my jetlag is too severe to make sense of it

(mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize is just going to equal mLRUPoolAvailableIndex, not mLRUPoolSize - 1, right?  What's going on here?

@@ +1381,5 @@
> +{
> +  for (uint32_t i = mLRUPoolAvailableIndex; i > 0; i--) {
> +    (*mLRUPool)[i] = (*mLRUPool)[i - 1];
> +    // Check whether i+1 is power of Two.
> +    // If so, then we need to assign its new process priority LRU.

I would add "because it crossed a LRU group boundary" or something to that effect so it's clear why these are special.
Attachment #773839 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #48)
> Comment on attachment 773839 [details] [diff] [review]
> @@ +1218,5 @@
> > +                                              void* aClosure)
> > +{
> > +  StaticInit();
> > +  return 0;
> > +}
> 
> I don't understand why this exists at all.  You only set up this preference
> observer if getting the preference at the start fails, but that should
> basically never happen.  I think you can get rid of this.
> 
> @@ +1280,5 @@
> > +    if (!sPrefListenersRegistered) {
> > +      sPrefListenersRegistered = true;
> > +      Preferences::RegisterCallback(PrefChangedCallback,
> > +        "dom.ipc.processPriorityManager.backgroundLRUPoolLevels");
> > +    }
> 
> Then once you get rid of this, EnsureLRUPool cannot fail, so you can go back
> to the constructor and get rid of the if (!sInitialized) bit.
> 

I was wondering the case the someone set preference of "dom.ipc.processPriorityManager.backgroundLRUPoolLevels" again. But you are right, this shouldn't happen in any cases. I will remove these out.

> @@ +1289,5 @@
> > +    MOZ_CRASH();
> > +  }
> > +
> > +  //LRU pool size = 2^(number of background LRU pool levels)-1
> > +  mLRUPoolSize = (uint32_t)((1 << mLRUPoolLevels) - 1);
> 
> Please add an assertion that mLRUPoolLevels is not too big.
> 
> MOZ_ASSERT(mLRUPoolLevels <= 31), I believe.
> 
> @@ +1295,5 @@
> > +
> > +  LOG("Making background LRU pool with size(%d)", mLRUPoolSize);
> > +
> > +  mLRUPool = new nsTArray<ContentParent*>();
> > +  mLRUPool->InsertElementsAt(0, mLRUPoolSize, (ContentParent*)nullptr);
> 
> Actually you may want something smaller for the maximum value of
> mLRUPoolLevels because if someone tries 31 this is going to OOM ;-)

Okay, but this should be MOZ_ASSERT(mLRUPoolLevels <= 6). In b2g.js, Our PROCESS_PRIORITY_BACKGROUND's oom_score_adj is 667, which leads its oom_adj is 10. In this case, we can only have mLRUPoolLevels <= 6 because maximum oom_adj is 15 in GonkHal.cpp

> 
> Also I doubt you need the ContentParent cast.

We need this cast, otherwise we will get a build break :(

> @@ +1357,5 @@
> > +  for (uint32_t i = 0; i < mLRUPoolSize; i++) {
> > +    if ((*mLRUPool)[i]) {
> > +      if ((*mLRUPool)[i]->ChildID() == aContentParent->ChildID()) {
> > +        LOG("ChildID(%llu) already in LRU pool", aContentParent->ChildID());
> > +        return NS_ERROR_UNEXPECTED;
> 
> Does this actually happen?  If not it would be nice to MOZ_ASSERT it instead
> of returning NS_ERROR_UNEXPECTED.

I hope this won't happen but I think I have to make sure there won't be somewhere else set a processes' priority from background to background. But I think you are right, I should add MOZ_ASSERT here.

> 
> @@ +1370,5 @@
> > +
> > +  // If the LRUPool is already full, then we should set mLRUPoolAvailableIndex
> > +  // to mLRUPoolSize-1. Here uses the mod operator to do it.
> > +  mLRUPoolAvailableIndex =
> > +    (mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize;
> 
> Either the comment or the code or wrong, or my jetlag is too severe to make
> sense of it
> 
> (mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize is just going to
> equal mLRUPoolAvailableIndex, not mLRUPoolSize - 1, right?  What's going on
> here?

After the for-loop, the mLRUPoolAvailableIndex equals -1 if the the pool is full.
so, (mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize will equal mLRUPoolAvailableIndex in normal cases (the pool still have room), and equal mLRUPoolSize - 1 in the special case (there's no room for it).

I will modify the comments into this:
>  // If the LRUPool is already full, mLRUPoolAvailableIndex is still -1 after
>  // above loop finished. We should set mLRUPoolAvailableIndex
>  // to mLRUPoolSize - 1 in this case. Here uses the mod operator to do it:
>  // New mLRUPoolAvailableIndex either equals old mLRUPoolAvailableIndex, or
>  // mLRUPoolSize - 1 if old mLRUPoolAvailableIndex is -1.
>  mLRUPoolAvailableIndex =
>    (mLRUPoolAvailableIndex + mLRUPoolSize) % mLRUPoolSize;

  

  



        Attachment #773839 -
        Attachment is obsolete: true

        Attachment #809807 -
        Flags: review?(khuey)

    
    

    
  
Alive,

Does this patch fix 8892371

  

  


Flags: needinfo?(alive)

    
    

    
  
Transfer NI

  

  


Flags: needinfo?(alive)

    
    

    
  
(In reply to Preeti Raghunath(:Preeti) from comment #51)
> Alive,
> 
> Does this patch fix 8892371

No, this can solve only one caller and one callee case.

  

  


blocking-b2g: - → 1.3?

    
  
Blocks: 892371

    
    

    
  
Comment on attachment 809807 [details] [diff] [review]
Implement an LRU pool for background app processes.

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

r=me

::: dom/browser-element/mochitest/priority/test_BackgroundLRU.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +Test that calling setVisible('false') on an iframe causes its visibility to
> +change.

Fix the comment please.

::: dom/ipc/ProcessPriorityManager.cpp
@@ +953,5 @@
>  void
>  ParticularProcessPriorityManager::SetPriorityNow(ProcessPriority aPriority,
> +                                                 ProcessCPUPriority aCPUPriority,
> +                                                 uint32_t aBackgroundLRU
> +                                                 )

nit: move the ')' up onto the previous line

@@ +1183,5 @@
>           mCachedPriority >= PROCESS_PRIORITY_FOREGROUND;
>  }
>  
> +/* static */ StaticAutoPtr<BackgroundProcessLRUPool>
> +  BackgroundProcessLRUPool::sSingleton;

nit: no indentation here.

@@ +1189,5 @@
> +/* static */ BackgroundProcessLRUPool*
> +BackgroundProcessLRUPool::Singleton()
> +{
> +  if (!sSingleton) {
> +    //StaticInit();

Please remove commented out code.

@@ +1234,5 @@
> +  }
> +
> +  // LRU pool size = 2 ^ (number of background LRU pool levels) - 1
> +  mLRUPoolSize = (uint32_t)((1 << mLRUPoolLevels) - 1);
> +  MOZ_ASSERT(mLRUPoolLevels <= 6);

Please add your explanation for why this is 6 in a comment here.

@@ +1274,5 @@
> +  // We have to make sure the index in LRUPool doesn't point to any
> +  // ContentParent.
> +  if (aTargetIndex >= 0 && aTargetIndex < mLRUPoolSize &&
> +      aTargetIndex < mLRUPoolAvailableIndex &&
> +    mLRUPool[aTargetIndex] == nullptr) {

please indent the last line of the if condition properly, and just do !mLRUPool[aTargetIndex]

  

  



        Attachment #809807 -
        Flags: review?(khuey) → review+

    
    

    
  


  
Revise patch according to reviewer's comments.

Modifying ProcessPriorityManager will not only effect b2g. I use try command "try: -b do -p all -u all -t none" for testing.
https://tbpl.mozilla.org/?tree=Try&rev=2c36770c3062
Waiting for test result now.

  

  



        Attachment #809807 -
        Attachment is obsolete: true

    
    

    
  
Unfortunately the patch did not build on try due to -Werror.  You can use --enable-warnings-as-errors to get that locally.

  

  



    
    

    
  
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #56)
> Unfortunately the patch did not build on try due to -Werror.  You can use
> --enable-warnings-as-errors to get that locally.

Yep, that's true. I should be careful and build with --enable-warnings-as-errors locally. I fixed the signed/unsigned integer comparison and made another patch.

  

  



    
    

    
  


  
This patch passed my local b2g and browser build with "--enable-warnings-as-errors". 

Now it's on try server:
https://tbpl.mozilla.org/?tree=Try&rev=aa9605b1ebd9

  

  



        Attachment #811787 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/bca0c6bde0ef

  

  


Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
  
Depends on: 922919

    
  
Blocks: 922919
No longer depends on: 922919

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
  
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED

    
    

    
  
not a committed feature for 1.3, so clearing nom

  

  


blocking-b2g: 1.3? → ---
Whiteboard: [Fugu] [v1.2f-uplift-needed]

    
    

    
  
Do you have any performance number to prove the patch?

  

  


Flags: needinfo?(ahuang)

    
    

    
  
Sorry, no, since our purpose to do this is to deliver better user experience instead of improving performance.

  

  


Flags: needinfo?(ahuang)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: