Idea: adjust OOM scores to favor Chrome tabs over Android stuff |
||||||||
Issue descriptionThis 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.
,
Jun 2 2017
> 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.
,
Jun 2 2017
CL on the way https://codereview.chromium.org/2922653003/
,
Jun 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32715a644e2054e559a66fbc5d75fc3a75f7a607 commit 32715a644e2054e559a66fbc5d75fc3a75f7a607 Author: cylee <cylee@chromium.org> Date: Fri Jun 02 15:26:32 2017 TabManager: Kill background tabs before android (background) apps. BUG= chromium:728865 TEST=unit tests Review-Url: https://codereview.chromium.org/2922653003 Cr-Commit-Position: refs/heads/master@{#476663} [modify] https://crrev.com/32715a644e2054e559a66fbc5d75fc3a75f7a607/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc [modify] https://crrev.com/32715a644e2054e559a66fbc5d75fc3a75f7a607/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h [modify] https://crrev.com/32715a644e2054e559a66fbc5d75fc3a75f7a607/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc
,
Jun 2 2017
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.
,
Jun 2 2017
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.
,
Jun 5 2017
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?
,
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.
,
Jun 7 2017
,
Jun 7 2017
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
,
Jun 7 2017
This should be coordinated with b/62353695 (Reduce ARC container RAM usage when not running an app).
,
Jun 7 2017
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.
,
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.
,
Jun 8 2017
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.
,
Jun 8 2017
Yeah M60 mergeback is on the way: https://chromium-review.googlesource.com/c/527979
,
Jun 8 2017
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
,
Jun 8 2017
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.
,
Jun 8 2017
,
Jan 22 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bccheng@chromium.org
, Jun 2 2017