Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22911017:
[Android] Make TRIM_MEMORY_COMPLETE check >= rather than == (Closed)
|
Created:
7 years, 4 months ago by rmcilroy Modified:
7 years, 3 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, bulach Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Make TRIM_MEMORY_COMPLETE check >= rather than ==
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223727
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment. #
Total comments: 2
Patch Set 3 : Add specific callback for ui hidden notifications #Patch Set 4 : Back to patch set 2, and refine documentation. #Patch Set 5 : #Messages
Total messages: 15 (0 generated)
As requested in comment to http://crrev.com/18908002
As requested in comment to http://crrev.com/18908002
Thanks! +bulach FYI https://codereview.chromium.org/22911017/diff/1/base/android/java/src/org/chr... File base/android/java/src/org/chromium/base/MemoryPressureListener.java (right): https://codereview.chromium.org/22911017/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/MemoryPressureListener.java:49: level == ComponentCallbacks2.TRIM_MEMORY_RUNNING_CRITICAL) { there's still a == here. Intentional?
https://codereview.chromium.org/22911017/diff/1/base/android/java/src/org/chr... File base/android/java/src/org/chromium/base/MemoryPressureListener.java (right): https://codereview.chromium.org/22911017/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/MemoryPressureListener.java:49: level == ComponentCallbacks2.TRIM_MEMORY_RUNNING_CRITICAL) { On 2013/08/19 16:28:42, joth wrote: > there's still a == here. Intentional? Actually this is intentional (we don't want to send a message for TRIM_MEMORY_UI_HIDDEN, which is 20, but do want to send one for TRIM_MEMORY_RUNNING_CRITICAL, which is 15, and for greater than TRIM_MEMORY_BACKGROUND, which is 40). I've added a comment as such.
sorry! I forgot to follow up on our comments on this one. basically the change lgtm although it does highlight something else we may want to address in future... https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureListener.java (right): https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureListener.java:51: // to the renderer process every time the user switches tabs. this is very chrome-specific comment. normally we try and design the code in base/ to be application agnostic. I'm not sure how to avoid it other than add more MemoryPressureLevelList.MEMORY_PRESSURE_xxx constants to better reflect these levels, OR change MemoryPressureListener to only report the 'background' levels and have Android specific code listen for the foreground notifications where relevant.
Thanks Joth. Bulach: do you have any suggestions regarding Joth's suggestion on base/ being application agnostic? I can't think of a very good solution at the moment.
https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/MemoryPressureListener.java (right): https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... base/android/java/src/org/chromium/base/MemoryPressureListener.java:15: * native. how about we add something like this: ...into native for levels that are considered actionable. ...should an app-level really want other levels, it can have its own notification mechanism to call directly the native code passing whatever mapping it wants.. wdyt?
On 2013/09/04 18:57:41, bulach wrote: > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > File base/android/java/src/org/chromium/base/MemoryPressureListener.java > (right): > > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > base/android/java/src/org/chromium/base/MemoryPressureListener.java:15: * > native. > how about we add something like this: > ...into native for levels that are considered actionable. > > > ...should an app-level really want other levels, it can have its own > notification mechanism to call directly the native code passing whatever mapping > it wants.. wdyt? Sorry for the delay, I've been busy with other things. How about this? I've added a new callback / notify for UiHidden, with the default being to ignore this callback. I think it makes sense to have this as a separate callback since the UI being hidden seems to me to be quite a different situation from the other memory pressure callbacks. wdyt?
On 2013/09/16 11:31:40, rmcilroy wrote: > On 2013/09/04 18:57:41, bulach wrote: > > > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > > File base/android/java/src/org/chromium/base/MemoryPressureListener.java > > (right): > > > > > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > > base/android/java/src/org/chromium/base/MemoryPressureListener.java:15: * > > native. > > how about we add something like this: > > ...into native for levels that are considered actionable. > > > > > > ...should an app-level really want other levels, it can have its own > > notification mechanism to call directly the native code passing whatever > mapping > > it wants.. wdyt? > > Sorry for the delay, I've been busy with other things. How about this? I've > added a new callback / notify for UiHidden, with the default being to ignore > this callback. I think it makes sense to have this as a separate callback since > the UI being hidden seems to me to be quite a different situation from the other > memory pressure callbacks. wdyt? Sometimes you want policy keyed off both - dropping rendering caches rarely makes sense when in the foreground unless we're really about to be killed (and even then, it's probably just delaying inevitable and a cost in jank/UX). When in background you may want to wait until a moderate pressure (and/or time of being in background) before dropping rendering caches (to avoid unnecessary jank on a short task away and back again). subscribers can use this API to achieve that, but means they may thrash a bit if both calls tend to occur in quick succession.
On 2013/09/16 23:04:23, joth wrote: > On 2013/09/16 11:31:40, rmcilroy wrote: > > On 2013/09/04 18:57:41, bulach wrote: > > > > > > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > > > File base/android/java/src/org/chromium/base/MemoryPressureListener.java > > > (right): > > > > > > > > > https://codereview.chromium.org/22911017/diff/6001/base/android/java/src/org/... > > > base/android/java/src/org/chromium/base/MemoryPressureListener.java:15: * > > > native. > > > how about we add something like this: > > > ...into native for levels that are considered actionable. > > > > > > > > > ...should an app-level really want other levels, it can have its own > > > notification mechanism to call directly the native code passing whatever > > mapping > > > it wants.. wdyt? > > > > Sorry for the delay, I've been busy with other things. How about this? I've > > added a new callback / notify for UiHidden, with the default being to ignore > > this callback. I think it makes sense to have this as a separate callback > since > > the UI being hidden seems to me to be quite a different situation from the > other > > memory pressure callbacks. wdyt? > > Sometimes you want policy keyed off both - dropping rendering caches rarely > makes sense when in the foreground unless we're really about to be killed (and > even then, it's probably just delaying inevitable and a cost in jank/UX). When > in background you may want to wait until a moderate pressure (and/or time of > being in background) before dropping rendering caches (to avoid unnecessary jank > on a short task away and back again). subscribers can use this API to achieve > that, but means they may thrash a bit if both calls tend to occur in quick > succession. Yes this makes sense, I think the added UiHidden callback was a mistake and have reverted it. It turns out I was misunderstanding Marcus's comment - he meant the documentation in the header should be changed to signal that we only forward actionable memory pressure signals onto native, and any other levels (e.g., UIHidden) should be forwarded on by a notifier in the app level if required. I've updated the comment to specify this and to make it be less specific to chrome.
lgtm as we chatted, any app that have more knowledge or want something more specifically can then listen to the extra signals at the app level and go to native as it wishes... thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911017/20001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911017/20001
Message was sent while issue was closed.
Change committed as 223727 |
