Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259743002:
Add MemoryStateListenerAndroid (Closed)
DescriptionAdd MemoryStateListenerAndroid
This class registers a callback which is invoked when the OS
dispatches OnTrimMemory(). ChildMemoryCoordinatorImplAndroid
sends calls |delegate()->OnTrimMemoryImmediately()| when
the level of memory pressure is critical.
BUG=617492
Patch Set 1 #Patch Set 2 : Remove dep #Patch Set 3 : Fix dependency and update test #
Total comments: 2
Patch Set 4 : Comments & export #
Total comments: 16
Patch Set 5 : Remove check #Patch Set 6 : native #Messages
Total messages: 37 (13 generated)
Description was changed from ========== WIP: Add MemoryStateListener for android BUG=617492 ========== to ========== Add MemoryStateListenerAndroid This class registers a callback which is invoked when the OS dispatches OnTrimMemory(). ChildMemoryCoordinatorImplAndroid sends calls |delegate()->OnTrimMemoryImmediately()| when the level of memory pressure is critical. BUG=617492 ==========
Patchset #2 (id:20001) has been deleted
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL The CL lacks tests (maybe it's difficult to have tests) but I'd like to have quick feedback if this looks good to you. This is the Java part of https://codereview.chromium.org/2260433006/.
This looks good to me but I'm not familiar with JNI enough to review this CL. Chris?
Unfortunately, neither am I. lgtm in principle, however. Maybe Andrew Grieve can pipe in here?
haraken@chromium.org changed reviewers: + agrieve@chromium.org
+agrieve
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bashi@chromium.org changed reviewers: + jochen@chromium.org, michaelbai@chromium.org, nyquist@chromium.org
Adding OWNERs review michaelbai@: Could you review content/app/android ? nyquist@: Could you review chrome/android/BUILD.gn ? jochen@: Could you review components/BUILD.gn and content/ ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
will this component be shared with ios? otherwise, it should be part of content/ (see crbug.com/624590)
On 2016/08/29 09:26:29, jochen wrote: > will this component be shared with ios? otherwise, it should be part of content/ > (see crbug.com/624590) Yes if the original plan is unchanged. But I'm not sure we really want to make this available on iOS. Separating memory coordinator from content/ has been the source of complexity of memory coordinator. chrisha@, haraken@: WDYT?
https://codereview.chromium.org/2259743002/diff/60001/components/memory_coord... File components/memory_coordinator/common/BUILD.gn (right): https://codereview.chromium.org/2259743002/diff/60001/components/memory_coord... components/memory_coordinator/common/BUILD.gn:35: } Since all those files is Android specific, It is better to move them to //components/memory_coordinator/android/BUILD.gn.
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
> will this component be shared with ios? otherwise, it should be part of content/ > (see crbug.com/624590) > Yes if the original plan is unchanged. But I'm not sure we really want to make > this available on iOS. Separating memory coordinator from content/ has been the > source of complexity of memory coordinator. > > chrisha@, haraken@: WDYT? Chatted with chrisha@ and haraken@. Though it's still unclear how well memory coordinator would work on iOS but our current plan is to support iOS as well. I'd like to keep this in components/ for now. In the future we may move this into content/ if it doesn't work well on iOS. https://codereview.chromium.org/2259743002/diff/60001/components/memory_coord... File components/memory_coordinator/common/BUILD.gn (right): https://codereview.chromium.org/2259743002/diff/60001/components/memory_coord... components/memory_coordinator/common/BUILD.gn:35: } On 2016/08/29 16:43:23, michaelbai wrote: > Since all those files is Android specific, It is better to move them to > //components/memory_coordinator/android/BUILD.gn. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
so typically, a layered component looks like this: https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... I have a hard time matching the current structure to this - the code you've added so far is independent of content/ I'd propose to move the code into content until it actually is certain that we want ios support and what it should look like on a general note, there are no DEPS files in the component that would keep somebody from including browser/ files from child/
looks fine overall, but will defer to others about content vs components https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: @MainDex Are you sure this is required to be in the main dex? It's not obvious to me why it would be. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:52: if (sCallbacks == null) { nit: the native side of this already DCHECKs this condition. Probably don't need this if check as well. If you want, you could change it to use an assert (but should likewise assert that registerCallback is called only once) https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:60: private static void simulateOnTrimMemory(Context context, int level) { This looks to be used only by tests, but having it be a JNI method means it will end up in the final Chrome.apk. It's simple enough that I think it'd be better to just have the C++ call Application.onTrimMemory() directly via JNI rather than go through this wrapper method. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl_android.cc (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl_android.cc:17: MemoryStateListenerAndroid::Start(this); No action required - but just wanted to see if at some point you had plans to make this also query for the existing memory state. E.g. rather than only trimming when thresholds change, maybe we'd want allocations to be limited while under memory pressure?
https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl_android.cc (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl_android.cc:17: MemoryStateListenerAndroid::Start(this); On 2016/08/30 18:23:44, agrieve wrote: > No action required - but just wanted to see if at some point you had plans to > make this also query for the existing memory state. E.g. rather than only > trimming when thresholds change, maybe we'd want allocations to be limited while > under memory pressure? Getting the state transitions would indeed be beneficial. They are available in Android? I was under the (mistaken?) impression that you only got point notifications about pressure events, and never knew when it was ending. Is there a signal we can subscribe to that tells us the global state of memory pressure on the system? ie. not just notifications about my process being in the middle of the kill list, etc, but global notifications like "I'm going to start killing processes, as memory is full".
On 2016/08/30 12:16:44, jochen wrote: > so typically, a layered component looks like this: > https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... > > I have a hard time matching the current structure to this - the code you've > added so far is independent of content/ > > I'd propose to move the code into content until it actually is certain that we > want ios support and what it should look like > > on a general note, there are no DEPS files in the component that would keep > somebody from including browser/ files from child/ IIUC, you are suggesting that 1) move memory coordinator into content/ 2) otherwise follow the structure mentioned in https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... ? I'd prefer to 1 -- On iOS I think memory coordinator would be much less useful given that we don't use blink, v8 and content. I was trying to find out what components in iOS we can make them as clients of memory coordinator but I couldn't find candidates so far. If chrisha@ and haraken@ are fine with moving this into content/, I'll try to that, but I'd like to separate components vs content discussion from this CL at this point.
https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: @MainDex On 2016/08/30 18:23:44, agrieve wrote: > Are you sure this is required to be in the main dex? It's not obvious to me why > it would be. The comment in MainDex.java says that this annotation means it's used by renderer process, and this will be used in renderer. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:52: if (sCallbacks == null) { On 2016/08/30 18:23:44, agrieve wrote: > nit: the native side of this already DCHECKs this condition. Probably don't need > this if check as well. If you want, you could change it to use an assert (but > should likewise assert that registerCallback is called only once) Removed. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:60: private static void simulateOnTrimMemory(Context context, int level) { On 2016/08/30 18:23:44, agrieve wrote: > This looks to be used only by tests, but having it be a JNI method means it will > end up in the final Chrome.apk. > > It's simple enough that I think it'd be better to just have the C++ call > Application.onTrimMemory() directly via JNI rather than go through this wrapper > method. Could you elaborate how I can implement this in native code? I codesearched but couldn't find much examples which I can refer to (it seems that it's uncommon to invoke Java APIs from native code in chromium). https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl_android.cc (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl_android.cc:17: MemoryStateListenerAndroid::Start(this); On 2016/08/30 18:38:38, chrisha (slow) wrote: > On 2016/08/30 18:23:44, agrieve wrote: > > No action required - but just wanted to see if at some point you had plans to > > make this also query for the existing memory state. E.g. rather than only > > trimming when thresholds change, maybe we'd want allocations to be limited > while > > under memory pressure? > > Getting the state transitions would indeed be beneficial. They are available in > Android? I was under the (mistaken?) impression that you only got point > notifications about pressure events, and never knew when it was ending. > > Is there a signal we can subscribe to that tells us the global state of memory > pressure on the system? ie. not just notifications about my process being in the > middle of the kill list, etc, but global notifications like "I'm going to start > killing processes, as memory is full". If the Android system provides features which we can use to query for existing memory state, yes, we would want to use it. But for now I'd want to focus on OnTrimMemory().
On 2016/09/01 00:52:54, bashi1 wrote: > On 2016/08/30 12:16:44, jochen wrote: > > so typically, a layered component looks like this: > > > https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... > > > > I have a hard time matching the current structure to this - the code you've > > added so far is independent of content/ > > > > I'd propose to move the code into content until it actually is certain that we > > want ios support and what it should look like > > > > on a general note, there are no DEPS files in the component that would keep > > somebody from including browser/ files from child/ > > IIUC, you are suggesting that > 1) move memory coordinator into content/ > 2) otherwise follow the structure mentioned in > https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... > > ? > > I'd prefer to 1 -- On iOS I think memory coordinator would be much less useful > given that we don't use blink, v8 and content. I was trying to find out what > components in iOS we can make them as clients of memory coordinator but I > couldn't find candidates so far. > > If chrisha@ and haraken@ are fine with moving this into content/, I'll try to > that, but I'd like to separate components vs content discussion from this CL at > this point. +1 to moving MC to content/. Also +1 to do that in a separate CL.
https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: @MainDex On 2016/09/01 00:54:41, bashi1 wrote: > On 2016/08/30 18:23:44, agrieve wrote: > > Are you sure this is required to be in the main dex? It's not obvious to me > why > > it would be. > > The comment in MainDex.java says that this annotation means it's used by > renderer process, and > this will be used in renderer. Ah okay. Why have this live in the renderer process rather than the browser process? I'd figure subscribing in the browser process would make for easier coordination. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:60: private static void simulateOnTrimMemory(Context context, int level) { On 2016/09/01 00:54:40, bashi1 wrote: > On 2016/08/30 18:23:44, agrieve wrote: > > This looks to be used only by tests, but having it be a JNI method means it > will > > end up in the final Chrome.apk. > > > > It's simple enough that I think it'd be better to just have the C++ call > > Application.onTrimMemory() directly via JNI rather than go through this > wrapper > > method. > > Could you elaborate how I can implement this in native code? I codesearched but > couldn't find much examples which I can refer to (it seems that it's uncommon to > invoke Java APIs from native code in chromium). Look for generate_jar_jni() GN template https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl_android.cc (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl_android.cc:17: MemoryStateListenerAndroid::Start(this); On 2016/09/01 00:54:41, bashi1 wrote: > On 2016/08/30 18:38:38, chrisha (slow) wrote: > > On 2016/08/30 18:23:44, agrieve wrote: > > > No action required - but just wanted to see if at some point you had plans > to > > > make this also query for the existing memory state. E.g. rather than only > > > trimming when thresholds change, maybe we'd want allocations to be limited > > while > > > under memory pressure? > > > > Getting the state transitions would indeed be beneficial. They are available > in > > Android? I was under the (mistaken?) impression that you only got point > > notifications about pressure events, and never knew when it was ending. > > > > Is there a signal we can subscribe to that tells us the global state of memory > > pressure on the system? ie. not just notifications about my process being in > the > > middle of the kill list, etc, but global notifications like "I'm going to > start > > killing processes, as memory is full". You can pretty much assume memory is always full on Android :P. > > If the Android system provides features which we can use to query for existing > memory state, yes, we would want to use it. But for now I'd want to focus on > OnTrimMemory(). The trim levels just map to how many background processes Android is able to keep alive with the RAM available on the system. You can't know when they are starting to be killed, but the first trim level (moderate) is called when there are fewer than 13 background processes (so called fairly early.) I thought onTrimMemory would be called when memory recovers, but just tested and looks like it doesn't. Looks like the only way to know when there's no longer memory pressure is to poll. ActivityManager.getMyMemoryState() can poll for the current state. The docs say it contains the last value of onTrimMemory(), but I tested querying it from Application.onCreate(), and it correctly returned an existing pressure level on my device running N. I also tested calling it after memory pressure recovered, and it showed the lower trim level rather than the last value sent to onTrimMemory(). https://developer.android.com/reference/android/app/ActivityManager.html#getM... Anyways, not sure any of this is useful. Certainly not actionable in the current patch (if ever).
https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: @MainDex On 2016/09/01 02:23:08, agrieve wrote: > On 2016/09/01 00:54:41, bashi1 wrote: > > On 2016/08/30 18:23:44, agrieve wrote: > > > Are you sure this is required to be in the main dex? It's not obvious to me > > why > > > it would be. > > > > The comment in MainDex.java says that this annotation means it's used by > > renderer process, and > > this will be used in renderer. > > Ah okay. Why have this live in the renderer process rather than the browser > process? I'd figure subscribing in the browser process would make for easier > coordination. Here is a doc which describes the motivation of this class. https://docs.google.com/document/d/1X-RA2y5NP8P1uZ9-q9cxKxzcjqWBz0mS2M4rIsexk... My understanding is followings (but it's probably wrong): - browser and renderers are separate processes - The OS dispatches memory state notifications per process. Browser cannot receive memory state for renderers If browser can register ComponentCallbacks2 for renderers, we don't need to use this in renderers. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:60: private static void simulateOnTrimMemory(Context context, int level) { On 2016/09/01 02:23:08, agrieve wrote: > On 2016/09/01 00:54:40, bashi1 wrote: > > On 2016/08/30 18:23:44, agrieve wrote: > > > This looks to be used only by tests, but having it be a JNI method means it > > will > > > end up in the final Chrome.apk. > > > > > > It's simple enough that I think it'd be better to just have the C++ call > > > Application.onTrimMemory() directly via JNI rather than go through this > > wrapper > > > method. > > > > Could you elaborate how I can implement this in native code? I codesearched > but > > couldn't find much examples which I can refer to (it seems that it's uncommon > to > > invoke Java APIs from native code in chromium). > > Look for generate_jar_jni() GN template Thanks! Done. https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl_android.cc (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl_android.cc:17: MemoryStateListenerAndroid::Start(this); On 2016/09/01 02:23:08, agrieve wrote: > On 2016/09/01 00:54:41, bashi1 wrote: > > On 2016/08/30 18:38:38, chrisha (slow) wrote: > > > On 2016/08/30 18:23:44, agrieve wrote: > > > > No action required - but just wanted to see if at some point you had plans > > to > > > > make this also query for the existing memory state. E.g. rather than only > > > > trimming when thresholds change, maybe we'd want allocations to be limited > > > while > > > > under memory pressure? > > > > > > Getting the state transitions would indeed be beneficial. They are available > > in > > > Android? I was under the (mistaken?) impression that you only got point > > > notifications about pressure events, and never knew when it was ending. > > > > > > Is there a signal we can subscribe to that tells us the global state of > memory > > > pressure on the system? ie. not just notifications about my process being in > > the > > > middle of the kill list, etc, but global notifications like "I'm going to > > start > > > killing processes, as memory is full". > You can pretty much assume memory is always full on Android :P. > > > > > > If the Android system provides features which we can use to query for existing > > memory state, yes, we would want to use it. But for now I'd want to focus on > > OnTrimMemory(). > > The trim levels just map to how many background processes Android is able to > keep alive with the RAM available on the system. You can't know when they are > starting to be killed, but the first trim level (moderate) is called when there > are fewer than 13 background processes (so called fairly early.) > > I thought onTrimMemory would be called when memory recovers, but just tested and > looks like it doesn't. Looks like the only way to know when there's no longer > memory pressure is to poll. > > ActivityManager.getMyMemoryState() can poll for the current state. > > The docs say it contains the last value of onTrimMemory(), but I tested querying > it from Application.onCreate(), and it correctly returned an existing pressure > level on my device running N. I also tested calling it after memory pressure > recovered, and it showed the lower trim level rather than the last value sent to > onTrimMemory(). > > https://developer.android.com/reference/android/app/ActivityManager.html#getM... > > Anyways, not sure any of this is useful. Certainly not actionable in the current > patch (if ever). This information is really helpful. Thanks!
offline discussion revealed that net/ is supposed to depend on this, so we can't move the code either to content/ nor components/, but need to find a new place. I suggested base/
On 2016/09/01 12:07:47, jochen wrote: > offline discussion revealed that net/ is supposed to depend on this, so we can't > move the code either to content/ nor components/, but need to find a new place. > I suggested base/ Could we invert the net dependency with an interface or middle-man? net/ generally tries not to have dependencies I think.
https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... File components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java (right): https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: @MainDex On 2016/09/01 03:28:20, bashi1 wrote: > On 2016/09/01 02:23:08, agrieve wrote: > > On 2016/09/01 00:54:41, bashi1 wrote: > > > On 2016/08/30 18:23:44, agrieve wrote: > > > > Are you sure this is required to be in the main dex? It's not obvious to > me > > > why > > > > it would be. > > > > > > The comment in MainDex.java says that this annotation means it's used by > > > renderer process, and > > > this will be used in renderer. > > > > Ah okay. Why have this live in the renderer process rather than the browser > > process? I'd figure subscribing in the browser process would make for easier > > coordination. > > Here is a doc which describes the motivation of this class. > https://docs.google.com/document/d/1X-RA2y5NP8P1uZ9-q9cxKxzcjqWBz0mS2M4rIsexk... > > My understanding is followings (but it's probably wrong): > - browser and renderers are separate processes > - The OS dispatches memory state notifications per process. Browser cannot > receive memory state for renderers > > If browser can register ComponentCallbacks2 for renderers, we don't need to use > this in renderers. The testing I've done around memory is written up here: https://github.com/agrieve/AndroidMultiProcessPlayground and in internal doc: https://goto.google.com/tdiresearch I believe we shouldn't need to have renderers forward memory pressure signals to the browser. When Chrome is in the foreground, it will receive the same onTrimMemory() as all other foreground processes. onTrimMemory() signals sent to background processes are not as useful since they don't correspond to the number of background processes running, but instead show how close to the bottom of the LRU it is (and may never be called). Background processes are always killable, so should already be doing everything they can to free up memory.
On 2016/09/01 14:10:35, agrieve wrote: > On 2016/09/01 12:07:47, jochen wrote: > > offline discussion revealed that net/ is supposed to depend on this, so we > can't > > move the code either to content/ nor components/, but need to find a new > place. > > I suggested base/ > > Could we invert the net dependency with an interface or middle-man? net/ > generally tries not to have dependencies I think. There's a single small interface that we could move to base/ that would be all the net/ would have to know about. (MemoryCoordinatorClient, analogous to MemoryPressureListener).
On 2016/09/01 14:26:47, agrieve wrote: > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > File > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java > (right): > > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: > @MainDex > On 2016/09/01 03:28:20, bashi1 wrote: > > On 2016/09/01 02:23:08, agrieve wrote: > > > On 2016/09/01 00:54:41, bashi1 wrote: > > > > On 2016/08/30 18:23:44, agrieve wrote: > > > > > Are you sure this is required to be in the main dex? It's not obvious to > > me > > > > why > > > > > it would be. > > > > > > > > The comment in MainDex.java says that this annotation means it's used by > > > > renderer process, and > > > > this will be used in renderer. > > > > > > Ah okay. Why have this live in the renderer process rather than the browser > > > process? I'd figure subscribing in the browser process would make for easier > > > coordination. > > > > Here is a doc which describes the motivation of this class. > > > https://docs.google.com/document/d/1X-RA2y5NP8P1uZ9-q9cxKxzcjqWBz0mS2M4rIsexk... > > > > My understanding is followings (but it's probably wrong): > > - browser and renderers are separate processes > > - The OS dispatches memory state notifications per process. Browser cannot > > receive memory state for renderers > > > > If browser can register ComponentCallbacks2 for renderers, we don't need to > use > > this in renderers. > > The testing I've done around memory is written up here: > https://github.com/agrieve/AndroidMultiProcessPlayground > and in internal doc: https://goto.google.com/tdiresearch > > I believe we shouldn't need to have renderers forward memory pressure signals to > the browser. When Chrome is in the foreground, it will receive the same > onTrimMemory() as all other foreground processes. onTrimMemory() signals sent to > background processes are not as useful since they don't correspond to the number > of background processes running, but instead show how close to the bottom of the > LRU it is (and may never be called). Background processes are always killable, > so should already be doing everything they can to free up memory. Okay, interesting. Then having the listener be in the browser is probably sufficient. We also likely want to poll (at a very low frequency, say 5-10s?) in order to see the state transitions in the other direction.
On 2016/09/01 18:28:19, chrisha (slow) wrote: > On 2016/09/01 14:26:47, agrieve wrote: > > > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > > File > > > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java > > (right): > > > > > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > > > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: > > @MainDex > > On 2016/09/01 03:28:20, bashi1 wrote: > > > On 2016/09/01 02:23:08, agrieve wrote: > > > > On 2016/09/01 00:54:41, bashi1 wrote: > > > > > On 2016/08/30 18:23:44, agrieve wrote: > > > > > > Are you sure this is required to be in the main dex? It's not obvious > to > > > me > > > > > why > > > > > > it would be. > > > > > > > > > > The comment in MainDex.java says that this annotation means it's used by > > > > > renderer process, and > > > > > this will be used in renderer. > > > > > > > > Ah okay. Why have this live in the renderer process rather than the > browser > > > > process? I'd figure subscribing in the browser process would make for > easier > > > > coordination. > > > > > > Here is a doc which describes the motivation of this class. > > > > > > https://docs.google.com/document/d/1X-RA2y5NP8P1uZ9-q9cxKxzcjqWBz0mS2M4rIsexk... > > > > > > My understanding is followings (but it's probably wrong): > > > - browser and renderers are separate processes > > > - The OS dispatches memory state notifications per process. Browser cannot > > > receive memory state for renderers > > > > > > If browser can register ComponentCallbacks2 for renderers, we don't need to > > use > > > this in renderers. > > > > The testing I've done around memory is written up here: > > https://github.com/agrieve/AndroidMultiProcessPlayground > > and in internal doc: https://goto.google.com/tdiresearch > > > > I believe we shouldn't need to have renderers forward memory pressure signals > to > > the browser. When Chrome is in the foreground, it will receive the same > > onTrimMemory() as all other foreground processes. onTrimMemory() signals sent > to > > background processes are not as useful since they don't correspond to the > number > > of background processes running, but instead show how close to the bottom of > the > > LRU it is (and may never be called). Background processes are always killable, > > so should already be doing everything they can to free up memory. > > Okay, interesting. Then having the listener be in the browser is probably > sufficient. We also likely want to poll (at a very low frequency, say 5-10s?) in > order to see the state transitions in the other direction. Now I understand that forwarding singals wouldn't make sense (that makes mc implement simpler :). I think we still need a listener in renderer to take immediate action (e.g. v8 GC) when onTrimMemory() is called. This is how the current MemoryPressureListener::OnSyncMemoryPressure() is working in renderers (more context can be found [1] and [2]). So what do you think about listening onTrimMemory() just for RenderThreadImpl::onTrimMemoryImmediately() ? [1] https://bugs.chromium.org/p/chromium/issues/detail?id=590975 [2] https://docs.google.com/document/d/1X2HglPS9zIOWAzmIdp7ghh0AGQsVa__sJbE_wim3R...
On 2016/09/02 00:49:25, bashi1 wrote: > On 2016/09/01 18:28:19, chrisha (slow) wrote: > > On 2016/09/01 14:26:47, agrieve wrote: > > > > > > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > > > File > > > > > > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2259743002/diff/80001/components/memory_coord... > > > > > > components/memory_coordinator/android/java/src/org/chromium/components/memory_coordinator/MemoryStateListener.java:21: > > > @MainDex > > > On 2016/09/01 03:28:20, bashi1 wrote: > > > > On 2016/09/01 02:23:08, agrieve wrote: > > > > > On 2016/09/01 00:54:41, bashi1 wrote: > > > > > > On 2016/08/30 18:23:44, agrieve wrote: > > > > > > > Are you sure this is required to be in the main dex? It's not > obvious > > to > > > > me > > > > > > why > > > > > > > it would be. > > > > > > > > > > > > The comment in MainDex.java says that this annotation means it's used > by > > > > > > renderer process, and > > > > > > this will be used in renderer. > > > > > > > > > > Ah okay. Why have this live in the renderer process rather than the > > browser > > > > > process? I'd figure subscribing in the browser process would make for > > easier > > > > > coordination. > > > > > > > > Here is a doc which describes the motivation of this class. > > > > > > > > > > https://docs.google.com/document/d/1X-RA2y5NP8P1uZ9-q9cxKxzcjqWBz0mS2M4rIsexk... > > > > > > > > My understanding is followings (but it's probably wrong): > > > > - browser and renderers are separate processes > > > > - The OS dispatches memory state notifications per process. Browser cannot > > > > receive memory state for renderers > > > > > > > > If browser can register ComponentCallbacks2 for renderers, we don't need > to > > > use > > > > this in renderers. > > > > > > The testing I've done around memory is written up here: > > > https://github.com/agrieve/AndroidMultiProcessPlayground > > > and in internal doc: https://goto.google.com/tdiresearch > > > > > > I believe we shouldn't need to have renderers forward memory pressure > signals > > to > > > the browser. When Chrome is in the foreground, it will receive the same > > > onTrimMemory() as all other foreground processes. onTrimMemory() signals > sent > > to > > > background processes are not as useful since they don't correspond to the > > number > > > of background processes running, but instead show how close to the bottom of > > the > > > LRU it is (and may never be called). Background processes are always > killable, > > > so should already be doing everything they can to free up memory. > > > > Okay, interesting. Then having the listener be in the browser is probably > > sufficient. We also likely want to poll (at a very low frequency, say 5-10s?) > in > > order to see the state transitions in the other direction. > > Now I understand that forwarding singals wouldn't make sense (that makes mc > implement simpler :). I think we still need a listener in renderer to take > immediate action (e.g. v8 GC) when onTrimMemory() is called. This is how the > current MemoryPressureListener::OnSyncMemoryPressure() is working in renderers > (more context can be found [1] and [2]). So what do you think about listening > onTrimMemory() just for RenderThreadImpl::onTrimMemoryImmediately() ? > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=590975 > [2] > https://docs.google.com/document/d/1X2HglPS9zIOWAzmIdp7ghh0AGQsVa__sJbE_wim3R... If it simplifies the code, then that sgtm. But afaik, there's no logic in Android that takes action after onTrimMemory() is dispatched (e.g. if you slept for 1 second then cleared some memory, it would have the same effect), so there's no Android-wise need to free memory synchronously. |
