jlebar and I had a difference of opinion over this heuristic, but my experience running the memory usage tests has convinced me that this is a good idea. What I've found is that my mental model of launching apps is kind of like "back/forward" navigation in a web browser. Launching the app is "forward", returning to the homescreen is "back". In this model, my mind expects the homescreen to stay running behind the foreground app and going "back" takes me to the state I was previously in. When the homescreen is killed early, this model is violated and it's rather jarring. This is fudging the definition of a "perceivable" app a bit, but I think it's justified. I'd prefer to do this in the platform with heuristics like apps with app-launcher permissions get protected as perceivable when going to the background.

Doing this bug as written would violate the memory acceptance criteria as written. > MANDATORY: No perceivable application is killed due to memory pressure while a > background application is alive. Since we agree that the homescreen isn't actually perceivable, making it perceivable as a hack makes it impossible to meet this criterion. Instead, I'd suggest killing the homescreen after any true bg apps, but before any true perceivable apps. This further increases the apparent proliferation of memory classes, but since apps don't explicitly request to be put into a specific class, I don't think this is a big deal.

I don't disagree a priori. What do you propose for this new class?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > I don't disagree a priori. What do you propose for this new class? You mean, what name? "Homescreen"? "HighPriorityNonPerceivable"? :shrug:

Prior art is http://androidxref.com/4.0.4/xref/frameworks/base/services/java/com/android/server/am/ProcessList.java#47 Indeed, "home" is special cased. So whatev.

basecamp- The exprience is not so bad that we'll block on it for v1.

I tend to disagree. This bug causes the homescreen to perma-crash when any new app is loaded. So the user can get into situations where every app->homescreen transition relaunches the homescreen from scratch. Follow the steps at [1] to see what I mean. I would like UX to chime in. https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:_Every_app_is_successfully_launched_into_the_foreground

I'll just fix this; we don't need to waste our time arguing about the blocking status.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > I tend to disagree. This bug causes the homescreen to perma-crash when any > new app is loaded. So the user can get into situations where every > app->homescreen transition relaunches the homescreen from scratch. Follow > the steps at [1] to see what I mean. > > I would like UX to chime in. > > https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0: > _Every_app_is_successfully_launched_into_the_foreground You nailed it in your first comment. As the hub of the system, the home screen will be the most frequently-accessed component in FFOS, and as such we should focus our efforts on making it load instantly. I cannot speak to the implementation proposals discussed above, but from a product quality standpoint it would be tremendous if we can make the home app load instantly, reliably. For that reason, I strongly support blocking+ on this, and am also marking this UX-P1. Thanks guys :)

Triage team still feels that this isn't a blocker.

This seems to work. cjones, the main thing I'm unsure of here is whether we're SendSetAppType()'ing at the right time. I'm also unsure that I've chosen good settings for the oom killer, but I figure we can adjust these later.

Comment on attachment 693160 [details] [diff] [review] Patch, v1 We don't have SR in this component (I'll file a bug on that). Jonas, can you please SR the following API change? I'm adding an attribute "mozapptype" to iframes. It's ignored unless the iframe corresponds to an app. If the iframe corresponds to an app and mozapptype is "homescreen", we will be less likely to kill that iframe's process when we encounter low memory.

The Gaia change can land before or after the Gecko change.

Comment on attachment 693160 [details] [diff] [review] Patch, v1 >diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js >+pref("hal.processPriorityManager.gonk.backgroundHomescreenOomScoreAdjust", 200); Are you sure this value puts us on an integer oom_adj value? >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp > TabParent::Show(const nsIntSize& size) > { >+ nsAutoString appType; >+ GetAppType(appType); >+ if (!appType.IsEmpty()) { >+ unused << SendSetAppType(appType); We should send this along with the PBrowser constructor. We can pass it down from nsFrameConstructor and keep the IContent* hackery centralized. > void >+TabParent::GetAppType(nsAString& aAppType) We wouldn't need this if we send the info from FrameConstructor. This patch makes me vom in my mouth a bit, but otherwise looks OK ;).

> We should send this along with the PBrowser constructor. I tend to dislike adding things to that because it's unnecessarily complicated in the window.open case. We end up writing all the code twice (because it has to work parent --> child and child --> parent). Is there some other init method I can use?

Also, I guess you mean s/nsFrameConstructor/nsFrameLoader/?

Yes, I meant FrameLoader. There's not any other init method. If you can't send it though the constructor at least only send it once.

Comment on attachment 693160 [details] [diff] [review] Patch, v1 Review of attachment 693160 [details] [diff] [review]: ----------------------------------------------------------------- sr=me

> Are you sure this value puts us on an integer oom_adj value? Unless the kernel is doing something weird, it turns out that none of our oom_score_adj's correspond to integer oom_adj's. $ adb shell # echo 3 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 176 # echo 4 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 235 So 200 is not an integer oom_adj. But neither is 400, which we currently use for bg processes: # echo 6 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 352 # echo 7 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 411 and neither is 67, which we use for fg processes: # echo 1 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 58 # echo 2 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj 117 I'll use 411 in this patch and file a separate bug to fix the rest of these. I think we need to fix GonkHal.cpp::OomAdjOfOomScoreAdj, too. > Yes, I meant FrameLoader. Doing it in nsFrameLoader is /way/ simpler. Thanks for the suggestion.

> I'll use 411 in this patch and file a separate bug to fix the rest of these. erm, I mean 176.

Interdiff is $ interdiff <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=693160) <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=693478) if you want it.

> I'll use [176] in this patch and file a separate bug to fix the rest of these. Filed bug 822709.

> I'll use [176] in this patch and file a separate bug to fix the rest of these. Turns out the kernel is doing something weird, and our code (which it turns out is based on the kernel code) is right. It turns out that the oom_adj --> oom_score_adj and oom_score_adj --> oom_adj functions are not inverses of each other. The first one multiplies by 15 and divides by 1000, while the second multiplies by 1000 and divides by 17. The formulas are oom_score_adj = (oom_adj * 1000) / 17 oom_adj = (oom_score_adj * 15) / 1000 so an oom_score_adj of 200 corresponds perfectly to an oom_adj of 3. Just not the other way around. I'll fix the patch locally.

BTW, you can test this patch by running https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:_Every_app_is_successfully_launched_into_the_foreground . If by around step 18 you don't want to throw the phone out the window, your patches have helped ;).

Comment on attachment 693478 [details] [diff] [review] Patch, v2 [Approval Request Comment] Affects only B2G, no string changes.

> If by around step 18 you don't want to throw the phone out the window, your patches have helped ;). Homescreen didn't crash once.

Comment on attachment 693164 [details] Gaia pull request Tim, even though this isn't a blocker it's a UX-P1 (and a simple fix). If you're too busy to look at this because it's not a blocker, please let us know and we can find another reviewer. Thanks!

Given that we're actively working on this bug and it is highly desired I think we should remove any roadblocks to finishing this work. I'm flipping the blocking flag to basecamp+. We can renom, if necessary, if this bug takes significantly more time to land or has a bad bounce.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a56fc48ecfd https://hg.mozilla.org/releases/mozilla-b2g18/rev/bad2a6a97372