New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 728865 link

Starred by 3 users

Issue metadata

Status: Archived
Owner:
Closed: Jun 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Idea: adjust OOM scores to favor Chrome tabs over Android stuff

Project Member Reported by diand...@chromium.org, Jun 1 2017

Issue description

This is an idea for a short term "fix" to some of our problems dealing with low memory.

The idea here is, until we get our tab discarder and/or OOM killer into good shape, we should prioritize OOM killing of Chrome background tabs over killing Android stuff.

Why?  Because killing a Chrome tab will actually free up a lot of memory as opposed to killing a dinky little Android background process.

---

Specifically: the OOM killer works much worse than it used to before Android because we used to target a Chrome process to kill.  ...and almost always killing a Chrome process would free SIGNIFICANT memory.

Now, we kill a dinky Android process and it doesn't do much.  If we kill enough of them we might be able to get somewhere, but it takes a long time.

---

Looking at a sample OOM kill on a kevin (memory pressure created by "balloon"), I see the following processes with a non -1000 OOM score:

<6>[  434.856591] [ pid ]   uid  tgid total_vm      rss nr_ptes nr_pmds swapents oom_score_adj name

<6>[  434.856877] [ 3476]  1000  3476    72387     9158     109       4     3197           300 chrome
<6>[  434.856886] [ 3624] 665368  3624   237778        0     106       4     3435           817 d.process.media
<6>[  434.856889] [ 3647] 665382  3647   236490        0      96       4     3041           650 hromium.arc.ime
<6>[  434.856896] [ 3739] 656360  3739   239527        0     100       4     3070           741 arc.applauncher
<6>[  434.856899] [ 3760] 665372  3760   275937     2522     182       4     6974           417 .gms.persistent
<6>[  434.856902] [ 3825] 665388  3825   240929        0     107       4     3329           665 rk.clouddpc.arc
<6>[  434.856905] [ 3836] 665372  3836   240556        0     108       4     3268           787 e.process.gapps
<6>[  434.856908] [ 3883] 665372  3883   312231      620     219       4     6654           772 gle.android.gms
<6>[  434.856911] [ 4063] 665362  4063   236299        0      99       4     3163           757 c.intent_helper
<6>[  434.856914] [ 4104] 665365  4104   235762        0     101       4     3036           954 viders.calendar
<6>[  434.856917] [ 4124] 665362  4124   236225        0      92       4     2968           726 arc.file_system
<6>[  434.856920] [ 4141] 665383  4141   236469        0      92       4     3004           711 hromium.arc.tts
<6>[  434.856923] [ 4178] 665392  4178   248077        0     121       4     8465           696 gle.android.tts
<6>[  434.856927] [ 4222] 665366  4222   237202        0     105       4     3352           802 d.process.acore
<6>[  434.856930] [ 4262] 665360  4262   236731        0      95       4     2983           680 crash_collector
<6>[  434.856933] [ 4274] 665372  4274   269743      384     167       4     4672           909 id.gms.unstable
<6>[  434.856936] [ 4331] 656361  4331   236299        0      99       4     3029           924 m.android.phone
<6>[  434.856938] [ 4398] 665372  4398   263267        0     143       4     4134           970 .android.gms.ui
<6>[  434.856942] [ 4465] 665372  4465   264094        0     147       4     4546           939 ndroid.gms:snet
<6>[  434.856945] [ 4967] 665372  4967   264845        0     149       4     4319           893 android.gms:car
<6>[  434.856948] [ 5018] 665367  5018   235098        0      93       4     2947           878 id.defcontainer
<6>[  434.856950] [ 5068] 665374  5068   235139        0      92       4     2937           848 android.musicfx
<6>[  434.856953] [ 5097] 656360  5097   235133        0      97       4     2943           863 ndroid.keychain
<6>[  434.856956] [ 5117] 665375  5117   262996    16862     164       4     7857           300 android.vending
<6>[  434.856959] [ 5132] 665389  5132   237581        0      97       4     3003           833 apps.cloudprint
<6>[  434.856962] [ 5206]  1000  5206   131227    23094     223       4    11880           533 chrome

When we pick an OOM victim, we end up picking:

<3>[  434.857036] Out of memory: Kill process 4398 (.android.gms.ui) score 971 or sacrifice child
<3>[  434.857142] Killed process 4398 (.android.gms.ui) total-vm:1053068kB, anon-rss:0kB, file-rss:0kB

Then we end up picking:

<3>[  437.829731] Out of memory: Kill process 4104 (viders.calendar) score 955 or sacrifice child
<3>[  437.842958] Killed process 4104 (viders.calendar) total-vm:943048kB, anon-rss:0kB, file-rss:0kB


These are just not terribly useful tasks to pick.  Although they may be "unimportant" and thus find to kill, killing them really doesn't provide any real benefit and doesn't help us free up memory.

It would be _much_ better if we instead chose to kill process 5206.  If we had picked that process to kill, we would have freed up 23094 pages (AKA 92MB of memory).

---

This should be a very easy thing to do and could possibly even land in M-59.  teravest@ points to AdjustOomPrioritiesImpl() in Chrome as the place to change.

---

Note that it's OK if the Chrome tab discarder doesn't change.  It could freely try to kill some Android processes.  ...but if the tab discarder fails, it seems like we should point the OOM killer at Chrome tabs before Android processes.




 
Owner: cylee@chromium.org
Cheng-Yu, could you get this implemented so that we can give it a try ASAP?
> This should be a very easy thing to do and could possibly even land in M-59.
> teravest@ points to AdjustOomPrioritiesImpl() in Chrome as the place to change.

AdjustOomPrioritiesImpl() sorts the process list based on an enum called ProcessType:
https://chromium.googlesource.com/chromium/src.git/+/fedf65a53b4a7f5f88bde8f71fe1ebdfb80c3696/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h#33

So I think what we have to do here is to swap BACKGROUND_TAB = 4 and BACKGROUND_APP = 5 (with some unit test fixes.)  Either way, it shouldn't be complicated/risky.  Merging to M59/60 will require some manual conflict resolution because of https://codereview.chromium.org/2898033002 (Chrome refactoring with some file moves), though.

Labels: M-60 M-59
Owner: igo@chromium.org
As I understand it, I think we are hoping for M-59 for this.  What is the overall plan for validating that this will help as much as we think it will?  When do we want to try to get this merged back to M-59 and M-60?

Over to Igo to help test, coordinate, and decide.
On the same note, are we sure we even want to kill those "dinky processes"?

1. what are they?
2. do they automatically restart after killed?
3. does android kill them?
4. if they are part of one app, do they go away when the app itself is killed?  Or should they be killed together with the app main process?

Shouldn't we try to emulate Android's behavior with respect to these processes?  I suspect they get assigned OOM scores because they are forked from some app.

Igo: looks like this fell through the cracks.  Seems like it would be worth getting in R59 though, no?  ...or are we banking completely on Ben's changes to avoid OOMs?

Comment 8 by igo@chromium.org, Jun 5 2017

Let's test it out with the new build before merging. And we can chat about it today in the group. Thanks.

Comment 9 by igo@chromium.org, Jun 7 2017

Labels: Merge-Request-60
Project Member

Comment 10 by sheriffbot@chromium.org, Jun 7 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This should be coordinated with b/62353695 (Reduce ARC container RAM usage when not running an app).
To better explain #11:

we might want not to kill android background processes at all.

1. they are small, so reclaiming them is not very helpful
2. they might get restarted anyway
3. if we kill them successfully, we're changing the UX (no more Android notifications or background updates or whatnot)
4. if we really want to kill them, we might as well consider shutting down Android entirely and dealing with the consequences, which is discussed in the b/ issue.

Comment 13 by cylee@google.com, Jun 8 2017

#13 
yusukes@ has fixed some of those issues(if not all). For example, he've already marked "persistent" for those arc specific important processes (means it won't be killed by tab discarder anymore).  Also tab discarder doesn't kill the same process in 1 minute if it is respawned.

There doesn't seem to be a handy way deciding whether a process is "tiny system process" or a "normal android app". Maybe we'll need a whitelist for the former. But here "background" process may include those "normal android app". So we still want to kill "background" processes in the sense.
Owner: cylee@chromium.org
cylee: please do the merge to R60.

--

Note that this bug is about OOM scores, not general userspace kill order.  I've filed bug #731194 which aims to take this one step further and also points out more about the theoretical difference between OOM scores and userspace kill oder.

--

igo: still on your plate to figure out R59.

Comment 15 by cylee@google.com, Jun 8 2017

Yeah M60 mergeback is on the way: https://chromium-review.googlesource.com/c/527979
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 8 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b2635e877a3b3f3c9c4a4e59ca59f1cf155db89

commit 6b2635e877a3b3f3c9c4a4e59ca59f1cf155db89
Author: Cheng-Yu Lee <cylee@chromium.org>
Date: Thu Jun 08 19:44:35 2017

TabManager: Kill background tabs before android (background) apps.

BUG= chromium:728865 
TEST=unit tests

(cherry picked from 32715a644e2054e559a66fbc5d75fc3a75f7a607)

Review-Url: https://codereview.chromium.org/2922653003
Cr-Original-Commit-Position: refs/heads/master@{#476663}
Change-Id: Ifed1901150207c27edab1a1ce10c3d7b2e3f036d
Reviewed-on: https://chromium-review.googlesource.com/527979
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#262}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/6b2635e877a3b3f3c9c4a4e59ca59f1cf155db89/chrome/browser/memory/tab_manager_delegate_chromeos.cc
[modify] https://crrev.com/6b2635e877a3b3f3c9c4a4e59ca59f1cf155db89/chrome/browser/memory/tab_manager_delegate_chromeos.h
[modify] https://crrev.com/6b2635e877a3b3f3c9c4a4e59ca59f1cf155db89/chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc

Comment 17 by igo@chromium.org, Jun 8 2017

Labels: -M-59
Re: c14  - No let's not push to 59 right now. We haves fixes in with dramatic improvements. I'd like to isolate where we are at and test this in 60.

Removing the 59 label.
Status: Fixed (was: Untriaged)
OK, then this is fixed.  

Bug #731194 for future work.

Comment 19 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment