Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15995014:
Adds MemoryPressureListener. (Closed)
|
Created:
7 years, 6 months ago by bulach Modified:
7 years, 6 months ago CC:
chromium-reviews, mnaganov (inactive) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds MemoryPressureListener.
Some platforms, like android, send clear signals when
under memory pressure.
Create a centralized place so that:
- Underlying platform can advise the rest of the system of
its memory status.
- Individual subsystems can then listen to such notifications
and free their memory as per their own policies.
BUG=246125
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205796
Patch Set 1 #Patch Set 2 : Moved to base/, callback-based #Patch Set 3 : Hooks up with MemoryPurger #
Total comments: 20
Patch Set 4 : Joth's comments #
Total comments: 18
Patch Set 5 : More comments #
Total comments: 22
Patch Set 6 : #Patch Set 7 : MemoryPressureListener #
Total comments: 4
Patch Set 8 : Nits #
Total comments: 4
Patch Set 9 : nits #Messages
Total messages: 22 (0 generated)
following discussions on the linked bug, ptal.
https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureHandler.java (right): https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:11: remove \n https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:12: public class MemoryPressureHandler { private? Comment this is an internal impl. detail of the cc side? https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:13: public static void onMemoryPressure(int memoryPressureType) { Ah I see. So, we can make this self-contained, no public function needed, by doing appContext.registerComponentCallbacks(an instance of CompontentCallbacks2) GetApplicationContext() called on native side and passed over. https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureList.template (right): https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureList.template:9: public static final int name = value; indent++ https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... File base/memory/memory_pressure_handler.h (right): https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... base/memory/memory_pressure_handler.h:17: #define BASE_MEMORY_PRESSURE_HANDLER_H_ nit: don't like the Handler name as it doesn't handle anything itself. MemoryPressureNotifier? (Generally things that wrap an ObserverList get called Notifier?) https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... File base/memory/memory_pressure_handler_list.h (right): https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... base/memory/memory_pressure_handler_list.h:13: DEFINE_MEMORY_PRESSURE_TYPE(MEMORY_PRESSURE_MODERATE, 1) these need some descriptions. Tom's suggestions of time it takes to recreate in the bug is nice starting point - although that depends on the foreground/background state too of the 'thing' being dropped, and it may seem confusing to talk about that here too. (It leads to the question of should this carry for/background state too, but that's domain specific, not a global state, so we don't want it tied in here) https://codereview.chromium.org/15995014/diff/5001/chrome/android/testshell/j... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/15995014/diff/5001/chrome/android/testshell/j... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:160: MemoryPressureHandler.onMemoryPressure(level); with my self-contained idea, this would not be needed.. https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.cc (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.cc:90: MemoryPurger::PurgeAll(); this seems completely wrong use of MemoryPurger::PurgeAll() - we're not just about to hibernate to disk. We maybe still in foreground, and this will drop caches that will need re-creating on the very next frame. I think the individual subsystems that the MemoryPurger pokes each need to be migrated to be MemoryPressureListener aware and apply the fine grained policy appropriate to that domain. eventually, MemoryPurger::PurgeAll() would just be a synonym for broadcasting a MemoryPressureListener(CRITICAL) state. (maybe we have a dedicated CRITICAL_SUSPEND state for it) https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.h (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.h:32: static void RegisterMemoryPressureListener(); if this is actually useful it should be in cross-platform code.
thanks joth! comments addressed, and a couple of questions below, ptal. https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureHandler.java (right): https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:11: On 2013/06/05 16:51:23, joth wrote: > remove \n Done. https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:12: public class MemoryPressureHandler { On 2013/06/05 16:51:23, joth wrote: > private? Comment this is an internal impl. detail of the cc side? Done. https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:13: public static void onMemoryPressure(int memoryPressureType) { On 2013/06/05 16:51:23, joth wrote: > Ah I see. > > So, we can make this self-contained, no public function needed, by doing > appContext.registerComponentCallbacks(an instance of CompontentCallbacks2) > > GetApplicationContext() called on native side and passed over. > a-ha! good idea, I didn't know about this registration, certainly simplify things. done. https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureList.template (right): https://codereview.chromium.org/15995014/diff/5001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureList.template:9: public static final int name = value; On 2013/06/05 16:51:23, joth wrote: > indent++ Done. https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... File base/memory/memory_pressure_handler.h (right): https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... base/memory/memory_pressure_handler.h:17: #define BASE_MEMORY_PRESSURE_HANDLER_H_ On 2013/06/05 16:51:23, joth wrote: > nit: don't like the Handler name as it doesn't handle anything itself. > > MemoryPressureNotifier? > (Generally things that wrap an ObserverList get called Notifier?) Done. https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... File base/memory/memory_pressure_handler_list.h (right): https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... base/memory/memory_pressure_handler_list.h:13: DEFINE_MEMORY_PRESSURE_TYPE(MEMORY_PRESSURE_MODERATE, 1) On 2013/06/05 16:51:23, joth wrote: > these need some descriptions. > Tom's suggestions of time it takes to recreate in the bug is nice starting point > - although that depends on the foreground/background state too of the 'thing' > being dropped, and it may seem confusing to talk about that here too. (It leads > to the question of should this carry for/background state too, but that's domain > specific, not a global state, so we don't want it tied in here) yeah, I'm ambivalent of these levels... on one hand, it seems that having at least _two_ levels is better than none, so individual modules can fine tune their decision. otoh, it makes this more contentious... :) I made it just two levels, with comments, let me know if you think it's enough. background / foreground should be conveyed in some other mechanism, and each module should query that to decide what to flush. https://codereview.chromium.org/15995014/diff/5001/chrome/android/testshell/j... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/15995014/diff/5001/chrome/android/testshell/j... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:160: MemoryPressureHandler.onMemoryPressure(level); On 2013/06/05 16:51:23, joth wrote: > with my self-contained idea, this would not be needed.. indeed, removed. https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.cc (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.cc:90: MemoryPurger::PurgeAll(); On 2013/06/05 16:51:23, joth wrote: > this seems completely wrong use of MemoryPurger::PurgeAll() - we're not just > about to hibernate to disk. We maybe still in foreground, and this will drop > caches that will need re-creating on the very next frame. > > I think the individual subsystems that the MemoryPurger pokes each need to be > migrated to be MemoryPressureListener aware and apply the fine grained policy > appropriate to that domain. > > eventually, MemoryPurger::PurgeAll() would just be a synonym for broadcasting a > MemoryPressureListener(CRITICAL) state. (maybe we have a dedicated > CRITICAL_SUSPEND state for it) should I separate this for the time being then? https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.h (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.h:32: static void RegisterMemoryPressureListener(); On 2013/06/05 16:51:23, joth wrote: > if this is actually useful it should be in cross-platform code. the registration itself? this file itself is cross-platform.. for the time being, no other platform would send the signal yet, so I think this will be done as a separate patch.
https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.cc (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.cc:90: MemoryPurger::PurgeAll(); On 2013/06/05 19:02:11, bulach wrote: > On 2013/06/05 16:51:23, joth wrote: > > this seems completely wrong use of MemoryPurger::PurgeAll() - we're not just > > about to hibernate to disk. We maybe still in foreground, and this will drop > > caches that will need re-creating on the very next frame. > > > > I think the individual subsystems that the MemoryPurger pokes each need to be > > migrated to be MemoryPressureListener aware and apply the fine grained policy > > appropriate to that domain. > > > > eventually, MemoryPurger::PurgeAll() would just be a synonym for broadcasting > a > > MemoryPressureListener(CRITICAL) state. (maybe we have a dedicated > > CRITICAL_SUSPEND state for it) > > should I separate this for the time being then? Yes. Given pkasting's general skeptism about MemoryPurger::PurgeAll() ever working, and my suspension this will often do the wrong thing even if the purger otherwise were working correctly, I'd drop this part of the CL altogether. https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... File base/android/memory_pressure_handler_android.h (right): https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... base/android/memory_pressure_handler_android.h:6: #define CHROME_BROWSER_ANDROID_MEMORY_PRESSURE_HANDLER_ANDROID_H_ is this guy still a 'handler' ? https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... base/android/memory_pressure_handler_android.h:18: static void RegisterSystemCallback(JNIEnv* env); rather than require embedder to call this, we could do it from within the cross platform MemoryPressureNotifier's constructor. (naive approach: just have it call some static PlatInit() method that each platform implements as they like). This maybe racy if we don't setup Application Context right away on startup, but I think we do. Anyway, I don't have strong pref eitherway on this. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... File base/memory/memory_pressure_handler_list.h (right): https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_handler_list.h:4: fix this guy's filename also nit: it's more a level than a type. memory_pressure_notifier_level_list.h ? https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... File base/memory/memory_pressure_notifier.h (right): https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:53: }; indent
https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... File base/android/java/src/org/chromium/base/MemoryPressureHandler.java (right): https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:42: level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) { oh, hmmm, yeah we may find it very useful to have this global background-ness connected through too for GL resource release. I totally forgot this info was included here. We haven't a firm idea on the policy details there yet though, so maybe we come back and iterate more on. (Worse case we just register our own ComponentCallbacks2 instance for that I guess) https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... File base/android/memory_pressure_handler_android.h (right): https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... base/android/memory_pressure_handler_android.h:18: static void RegisterSystemCallback(JNIEnv* env); On 2013/06/05 19:28:58, joth wrote: > rather than require embedder to call this, we could do it from within the cross > platform MemoryPressureNotifier's constructor. (naive approach: just have it > call some static PlatInit() method that each platform implements as they like). > This maybe racy if we don't setup Application Context right away on startup, but > I think we do. > > Anyway, I don't have strong pref eitherway on this. but nead caution if we do this as we (probably) want to be sure the java notifier gets hooked up on the UI thread. Can easily do that with ThreadUtils.runOnUiThread() I guess. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... File base/memory/memory_pressure_notifier.h (right): https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:31: // the listener. I'd like to add some note that if this is the same thread as the system is broadcasting the memory pressure event on, then it is guaranteed you're called synchronously within that broadcast and hence you should not do long-running garbage collection work. But conversely, if there's something you want to be sure is released before we return coontrol back to system code, this is the place to do it. (The latter is very important for webview, assuming we use this to implement GL resource release). https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:41: // new MemoryPressureNotifier::Listener(base::Bind(&OnMemoryPressure)); oh right! I forgot the pattern used in ActivityStatus - the fact this is a wrapper around ObserverList is totally hidden internal detail, and you just use this RAII object to register & deregister. (i.e. there's no virtual interface you subclass or whatever) yeah so handler / observer / notifier in the outer class name is kind of spurious, as the outer class is never used by the event consumer. Just MemoryPressure::Listener seems just uine This seems a kind of uncommon pattern though, so others may have a stronger pref this is done some other way. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:75: void HandleMemoryPressure(MemoryPressureType memory_pressure_type); nit: Handle -> Notify
thanks joth! all addressed, and one question below: https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... File chrome/browser/memory_purger.cc (right): https://codereview.chromium.org/15995014/diff/5001/chrome/browser/memory_purg... chrome/browser/memory_purger.cc:90: MemoryPurger::PurgeAll(); On 2013/06/05 19:28:58, joth wrote: > On 2013/06/05 19:02:11, bulach wrote: > > On 2013/06/05 16:51:23, joth wrote: > > > this seems completely wrong use of MemoryPurger::PurgeAll() - we're not just > > > about to hibernate to disk. We maybe still in foreground, and this will drop > > > caches that will need re-creating on the very next frame. > > > > > > I think the individual subsystems that the MemoryPurger pokes each need to > be > > > migrated to be MemoryPressureListener aware and apply the fine grained > policy > > > appropriate to that domain. > > > > > > eventually, MemoryPurger::PurgeAll() would just be a synonym for > broadcasting > > a > > > MemoryPressureListener(CRITICAL) state. (maybe we have a dedicated > > > CRITICAL_SUSPEND state for it) > > > > should I separate this for the time being then? > > Yes. Given pkasting's general skeptism about MemoryPurger::PurgeAll() ever > working, and my suspension this will often do the wrong thing even if the purger > otherwise were working correctly, I'd drop this part of the CL altogether. > sgtm.. removed this altogether from the latest patchset. I also chatted with mnaganov, we'll be adding the various "MemoryPressure::Listener" in separate patches as soon as this lands. https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... File base/android/java/src/org/chromium/base/MemoryPressureHandler.java (right): https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:42: level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) { On 2013/06/05 19:49:48, joth wrote: > oh, hmmm, yeah we may find it very useful to have this global background-ness > connected through too for GL resource release. I totally forgot this info was > included here. > > We haven't a firm idea on the policy details there yet though, so maybe we come > back and iterate more on. (Worse case we just register our own > ComponentCallbacks2 instance for that I guess) should this perhaps expose the native call? that is, other modules could plug their listeners in their java code however they want? (of course, I mean, expose a static java wrapper function here, not the native directly..) https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... File base/android/memory_pressure_handler_android.h (right): https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... base/android/memory_pressure_handler_android.h:6: #define CHROME_BROWSER_ANDROID_MEMORY_PRESSURE_HANDLER_ANDROID_H_ On 2013/06/05 19:28:58, joth wrote: > is this guy still a 'handler' ? renamed to memory_pressure_android (and fixed the include header..) https://codereview.chromium.org/15995014/diff/19001/base/android/memory_press... base/android/memory_pressure_handler_android.h:18: static void RegisterSystemCallback(JNIEnv* env); On 2013/06/05 19:49:48, joth wrote: > On 2013/06/05 19:28:58, joth wrote: > > rather than require embedder to call this, we could do it from within the > cross > > platform MemoryPressureNotifier's constructor. (naive approach: just have it > > call some static PlatInit() method that each platform implements as they > like). > > This maybe racy if we don't setup Application Context right away on startup, > but > > I think we do. > > > > Anyway, I don't have strong pref eitherway on this. > > but nead caution if we do this as we (probably) want to be sure the java > notifier gets hooked up on the UI thread. Can easily do that with > ThreadUtils.runOnUiThread() I guess. I'd rather keep it simple for the time being.. once / if CrOS hook up their low-memory signals, we can certain review this in a way to accommodate the platform-specific bits.. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... File base/memory/memory_pressure_handler_list.h (right): https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_handler_list.h:4: On 2013/06/05 19:28:58, joth wrote: > fix this guy's filename > also nit: it's more a level than a type. > > memory_pressure_notifier_level_list.h ? yep, agreed. done. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... File base/memory/memory_pressure_notifier.h (right): https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:31: // the listener. On 2013/06/05 19:49:48, joth wrote: > I'd like to add some note that if this is the same thread as the system is > broadcasting the memory pressure event on, then it is guaranteed you're called > synchronously within that broadcast and hence you should not do long-running > garbage collection work. But conversely, if there's something you want to be > sure is released before we return coontrol back to system code, this is the > place to do it. (The latter is very important for webview, assuming we use this > to implement GL resource release). > added almost verbatim. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:41: // new MemoryPressureNotifier::Listener(base::Bind(&OnMemoryPressure)); On 2013/06/05 19:49:48, joth wrote: > oh right! I forgot the pattern used in ActivityStatus - the fact this is a > wrapper around ObserverList is totally hidden internal detail, and you just use > this RAII object to register & deregister. (i.e. there's no virtual interface > you subclass or whatever) > > > yeah so handler / observer / notifier in the outer class name is kind of > spurious, as the outer class is never used by the event consumer. Just > MemoryPressure::Listener seems just uine > > This seems a kind of uncommon pattern though, so others may have a stronger pref > this is done some other way. since this is based on the existing "ActivityStatus" (which doesn't have a handler / notifier / verb suffix), kept as MemoryPressure for consistency. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:53: }; On 2013/06/05 19:28:58, joth wrote: > indent Done. https://codereview.chromium.org/15995014/diff/19001/base/memory/memory_pressu... base/memory/memory_pressure_notifier.h:75: void HandleMemoryPressure(MemoryPressureType memory_pressure_type); On 2013/06/05 19:49:48, joth wrote: > nit: Handle -> Notify Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure_level_list.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:29: DEFINE_MEMORY_PRESSURE_LEVEL(MEMORY_PRESSURE_SURRENDER_OR_DIE, 2) btw: I don't want this to be funny or anything, but on all the discussions, including with android members, it seems people don't realize how "critical" the lack of swap actually is.. I fear that "MEMORY_PRESSURE_CRITICAL" may not be given the attention it deserves.. so wdyt about replacing "critical" with something a bit more 'frightening'?
lgtm to avoid more timezone back and forth, although I do think we'll need to have a few concrete users of it setup before we really know how these levels are going to pan out. https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... File base/android/java/src/org/chromium/base/MemoryPressureHandler.java (right): https://codereview.chromium.org/15995014/diff/19001/base/android/java/src/org... base/android/java/src/org/chromium/base/MemoryPressureHandler.java:42: level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) { On 2013/06/06 09:28:30, bulach wrote: > On 2013/06/05 19:49:48, joth wrote: > > oh, hmmm, yeah we may find it very useful to have this global background-ness > > connected through too for GL resource release. I totally forgot this info was > > included here. > > > > We haven't a firm idea on the policy details there yet though, so maybe we > come > > back and iterate more on. (Worse case we just register our own > > ComponentCallbacks2 instance for that I guess) > > should this perhaps expose the native call? that is, other modules could plug > their listeners in their java code however they want? (of course, I mean, expose > a static java wrapper function here, not the native directly..) Just to avoid having two different ComponentCallbacks2 classes instantiated? not sure it's worth it - we can always add a public API in future when needed. The question here was more if we should add more of the levels to MemoryPressureList https://codereview.chromium.org/15995014/diff/27001/base/android/java/src/org... File base/android/java/src/org/chromium/base/MemoryPressure.java (right): https://codereview.chromium.org/15995014/diff/27001/base/android/java/src/org... base/android/java/src/org/chromium/base/MemoryPressure.java:42: level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) { I dunno. This will map UI_HIDDEN => MODERATE, but TRIM_MEMORY_RUNNING_MODERATE => CRITICAL. The former is a great time to drop e.g. large bitmap allocations, but the latter isn't (if they're still in use on screen). How's this going to hang together? https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure_level_list.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:29: DEFINE_MEMORY_PRESSURE_LEVEL(MEMORY_PRESSURE_SURRENDER_OR_DIE, 2) On 2013/06/06 09:28:30, bulach wrote: > btw: I don't want this to be funny or anything, but on all the discussions, > including with android members, it seems people don't realize how "critical" the > lack of swap actually is.. > I fear that "MEMORY_PRESSURE_CRITICAL" may not be given the attention it > deserves.. > > so wdyt about replacing "critical" with something a bit more 'frightening'? This gets into brinkmanship. IF the system _really_ is that low, it will kill apps before even sending them this signal, so it's dangerous if this level will give people too much belief they don't need to release X at all until this signal is received. equally if X is needed to draw the very next frame to screen, dropping it is silly. So whatever scary wording is put here, it needs to be tempered with the domain specific estimate on how soon we're going to need to recreate it again anyway.
I did not review Android code. Some nits below are because "cold start" is a well-known phrase but "starting from cold" is something I've never heard. LGTM https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:14: // restarted from cold. This paragraph has grammar problems and is confusing anyway when compared against the level list; how about just "Refer to memory_pressure_level_list.h for information about what sorts of signals can be sent under what conditions." https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:28: // function that takes an MemoryPressureLevel parameter. To stop listening, Nit: an -> a https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:36: // it returns control back to system code, this is the place to do it. Nit: it returns control back -> control is returned https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:39: // potentially kill the app, and then later it'll have to be restarted from Nit: it'll have to be restarted from cold -> Chrome will have to be cold-started https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:56: // delete my_listener Nit: semicolon https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:58: class BASE_EXPORT MemoryPressure { Nit: I'd call this class "MemoryPressureHandler", as "memory pressure" is a condition, not an object. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:73: friend class MemoryPressure; I see no reason to make Notify() private and make the outer class a friend. Just make this public. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure_level_list.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:18: // have to be re-created, plus the cost of starting up from cold. Nit: starting up from cold -> a cold start https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:29: DEFINE_MEMORY_PRESSURE_LEVEL(MEMORY_PRESSURE_SURRENDER_OR_DIE, 2) I think this SURRENDER_OR_DIE case adds nothing to the CRITICAL case and I would remove it entirely.
thanks Peter and Joth! I addressed all the comments except naming as "handler", as you seem to have conflicting opinions. please let me know how strongly you feel about it and I'll make at least one of you happy ;) https://codereview.chromium.org/15995014/diff/27001/base/android/java/src/org... File base/android/java/src/org/chromium/base/MemoryPressure.java (right): https://codereview.chromium.org/15995014/diff/27001/base/android/java/src/org... base/android/java/src/org/chromium/base/MemoryPressure.java:42: level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) { On 2013/06/06 22:36:46, joth wrote: > I dunno. This will map UI_HIDDEN => MODERATE, but TRIM_MEMORY_RUNNING_MODERATE > => CRITICAL. > > The former is a great time to drop e.g. large bitmap allocations, but the latter > isn't (if they're still in use on screen). > How's this going to hang together? > agreed. I called out the critical level here (ComponentCallbacks2.TRIM_MEMORY_COMPLETE), and then everything else will be mapped to moderate. note that onLowMemory() is also critical. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:14: // restarted from cold. On 2013/06/07 01:00:43, Peter Kasting wrote: > This paragraph has grammar problems and is confusing anyway when compared > against the level list; how about just "Refer to memory_pressure_level_list.h > for information about what sorts of signals can be sent under what conditions." Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:28: // function that takes an MemoryPressureLevel parameter. To stop listening, On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: an -> a Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:36: // it returns control back to system code, this is the place to do it. On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: it returns control back -> control is returned Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:39: // potentially kill the app, and then later it'll have to be restarted from On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: it'll have to be restarted from cold -> Chrome will have to be cold-started Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:56: // delete my_listener On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: semicolon Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:58: class BASE_EXPORT MemoryPressure { On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: I'd call this class "MemoryPressureHandler", as "memory pressure" is a > condition, not an object. please see joth's nit, it was a handler before: https://codereview.chromium.org/15995014/diff/5001/base/memory/memory_pressur... I'm happy to go either way but I can't make you both happy :) please let me know who feels stronger about this. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure.h:73: friend class MemoryPressure; On 2013/06/07 01:00:43, Peter Kasting wrote: > I see no reason to make Notify() private and make the outer class a friend. > Just make this public. Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... File base/memory/memory_pressure_level_list.h (right): https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:18: // have to be re-created, plus the cost of starting up from cold. On 2013/06/07 01:00:43, Peter Kasting wrote: > Nit: starting up from cold -> a cold start Done. https://codereview.chromium.org/15995014/diff/27001/base/memory/memory_pressu... base/memory/memory_pressure_level_list.h:29: DEFINE_MEMORY_PRESSURE_LEVEL(MEMORY_PRESSURE_SURRENDER_OR_DIE, 2) On 2013/06/07 01:00:43, Peter Kasting wrote: > I think this SURRENDER_OR_DIE case adds nothing to the CRITICAL case and I would > remove it entirely. Done.
How about MemoryPressureService?
On 7 June 2013 13:52, <pkasting@chromium.org> wrote: > How about MemoryPressureService? > > Yeah that would work. hmmm Pondering more... I think the conceptual problem I have with MemoryPressureX is, for the most part, it's little more than a static namespace-like class (discouraged by the style guide). The "interesting" bit for the user is 100% inside the inner MemoryPressureX::Listener class. Combined with the fact that folks seem to now discourage use of nested public class declarations at all, I wonder if we can flip this around and strip the outer class away: make that outer calss into some detail in anonymous namespace in the .cc, not part of the public API. In fact, don't think it even needs a class declaration at all. e.g. typedef base::Callback<void(MemoryPressureLevel)> MemoryPressureCallback; // This is an RAII class yada yada. You will get callbacks as long as the object instance exists.... class MemoryPressureListener { public: explicit MemoryPressureListener(const MemoryPressureCallback& memory_pressure_callback); ~MemoryPressureListener(); // Intended for use by the platform specific implementation. static void NotifyAllListeners(MemoryPressureLevel memory_pressure_level); private: void Notify(MemoryPressureLevel memory_pressure_level); }; // .cc namespace { LazyInstance::Leaky<ObserverListThreadSafe<MemoryPressureListener> > g_observers; } MemoryPressureListener::NotifyAllListeners(MemoryPressureLevel memory_pressure_level) { // note how Notify() can now be private as we only access it from within the same class :-) g_observers->Notify(bind(&MemoryPressureListener::Notify() ...); } MemoryPressureListener::MemoryPressureListener() { g_observers->AddObserver(this); } ... Sorry for the back and forth... I should have spent more time on this way back when ActivityStatus landed, as that could really use the same simplification.
I don't have any preference w.r.t. all that.
thanks both! yeah, making the observer list internal surely simplifies this, ptal. after this lands, I'll make sure ActivityStatus uses the same pattern.
lgtm Yes, that is much simpler API now. Thanks! https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... base/memory/memory_pressure_listener.cc:36: &MemoryPressureListener::Notify, memory_pressure_level); nit: g_observers.Get().Notify(&MemoryPressureListener::Notify, memory_pressure_level); https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:36: // potentially kill the app, and then later Chrome will have to be cold-started. ubernit: we normally try and avoid talking about specific apps, e.g. Chrome, in the lower layers like base.
thanks joth! all addressed. +jar for OWNERS in base, ptal. https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... base/memory/memory_pressure_listener.cc:36: &MemoryPressureListener::Notify, memory_pressure_level); On 2013/06/10 18:53:22, joth wrote: > nit: > > g_observers.Get().Notify(&MemoryPressureListener::Notify, > memory_pressure_level); Done. https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/15995014/diff/46001/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:36: // potentially kill the app, and then later Chrome will have to be cold-started. On 2013/06/10 18:53:22, joth wrote: > ubernit: we normally try and avoid talking about specific apps, e.g. Chrome, in > the lower layers like base. Done.
Patch set 8; base/memory/memory* LGTM. See nits below. https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:20: #include "base/observer_list_threadsafe.h" nit: Do we need all these in the header? Do we need singleton here? ref_counted? https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:60: }; nit: personal: #undef DEFINE_MEMORY_PRESSURE_LEVEL after inclusion.
jochen: OWNERS for chrome/browser jar: thanks! addressed remaining nits. joth: the bots were red because of a misname between java and c++.. I renamed the _android files to be Listener too. https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:20: #include "base/observer_list_threadsafe.h" On 2013/06/11 00:20:42, jar wrote: > nit: Do we need all these in the header? Do we need singleton here? > ref_counted? ops, leftover from previous patchset. removed. https://codereview.chromium.org/15995014/diff/49002/base/memory/memory_pressu... base/memory/memory_pressure_listener.h:60: }; On 2013/06/11 00:20:42, jar wrote: > nit: personal: #undef DEFINE_MEMORY_PRESSURE_LEVEL after inclusion. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/15995014/60001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/15995014/60001
Message was sent while issue was closed.
Change committed as 205796 |
