Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2340293003:
Add MemoryMonitorAndroid (Closed)
DescriptionAdd MemoryMonitorAndroid
Use ActivityManager.getMemoryInfo() to implement MemoryMonitor
for android.
Design doc:
https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9e4/edit#heading=h.z8w4k8xdqow
NO_DEPENDENCY_CHECKS=true
BUG=617492
Committed: https://crrev.com/66264142bb289b0975682b0423685467cb2475f9
Cr-Commit-Position: refs/heads/master@{#421128}
Patch Set 1 #
Total comments: 4
Patch Set 2 : typo #
Total comments: 6
Patch Set 3 : DCHECK #
Total comments: 12
Patch Set 4 : Use native callback instead of returning MemoryInfo #
Total comments: 3
Patch Set 5 : rebase #
Messages
Total messages: 38 (16 generated)
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: + chrisha@chromium.org, haraken@chromium.org
Chris, does this make sense to you? I checked how long does ActivityManager.getMemoryInfo() take, and it took ~2ms including JNI overhead. I think it would be acceptable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(I'm clearly not eligible to review this CL though.) https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... content/browser/memory/memory_monitor_android.cc:67: // Implementation of factory function defined in memory_monitor.h. a factory function https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... File content/browser/memory/memory_monitor_android.h (right): https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... content/browser/memory/memory_monitor_android.h:40: // Initializes fields IDs of ActivityManager.MemoryInfo field IDs
https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... content/browser/memory/memory_monitor_android.cc:67: // Implementation of factory function defined in memory_monitor.h. On 2016/09/16 06:49:26, haraken wrote: > > a factory function Done. https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... File content/browser/memory/memory_monitor_android.h (right): https://codereview.chromium.org/2340293003/diff/1/content/browser/memory/memo... content/browser/memory/memory_monitor_android.h:40: // Initializes fields IDs of ActivityManager.MemoryInfo On 2016/09/16 06:49:26, haraken wrote: > > field IDs Done.
Looks nice and simple to me. Would be great to have an appropriate Android person chime in to ensure we're doing things right. Also need a Java reviewer. https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:32: return 0; When can this happen? If this is at all common, then we need to rethink this approach. https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android_unittest.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android_unittest.cc:29: } A test of the actual GetFreeMemoryUntilCriticalMb logic as well?
bashi@chromium.org changed reviewers: + agrieve@chromium.org, boliu@chromium.org, sky@chromium.org
Chris, thank you for review! boliu@, agrieve@: Could you review android part? sky@: Could you review content/test/BUILD.gn? https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:32: return 0; On 2016/09/26 17:56:48, chrisha (slow) wrote: > When can this happen? If this is at all common, then we need to rethink this > approach. IIUC, it shouldn't happen. Changed to DCHECK(). https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android_unittest.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android_unittest.cc:29: } On 2016/09/26 17:56:48, chrisha (slow) wrote: > A test of the actual GetFreeMemoryUntilCriticalMb logic as well? In this test, GetFreeMemoryUntilCriticalMb() returns actual available memory on the device on which this test is running and we can't specify an expected value (it may be zero or even negative). One option would be using a mock function (like other MemoryMonitor tests) but what we want to test here is that checking whether JNI bindings and Android APIs are working, so I didn't add a test of GetFreeMemoryUntilCriticalMb.
LGTM
https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:58: avail_mem_id_ = env->GetFieldID(clazz.obj(), "availMem", "J"); we go to great lengths to not hand write jni code. I'm not entirely sure if jni generation works for sdk classes, and if there's an existing example already how about returning long[], and convert with jni_array.h? or if this is called frequently and causing gc is not idea, could just have java callback into native with the 4 parameters I know neither is ideal, but still better than turning sdk mis-use into runtime errors imo
https://codereview.chromium.org/2340293003/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2340293003/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:1114: "../browser/memory/memory_monitor_linux_unittest.cc", different CL?
Androidy parts lgtm https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.h (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: drop the (c) https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.h:21: long avail_mem; nit: might be better to type these as either jlong, or int64_t
https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:58: avail_mem_id_ = env->GetFieldID(clazz.obj(), "availMem", "J"); On 2016/09/27 00:19:11, boliu wrote: > we go to great lengths to not hand write jni code. I'm not entirely sure if jni > generation works for sdk classes, and if there's an existing example already > > how about returning long[], and convert with jni_array.h? > > or if this is called frequently and causing gc is not idea, could just have java > callback into native with the 4 parameters > > I know neither is ideal, but still better than turning sdk mis-use into runtime > errors imo You can generate wrappers for sdk classes, but I don't think the generator has a good answer for field access. This seems fine to me, as it even has tests.
https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:58: avail_mem_id_ = env->GetFieldID(clazz.obj(), "availMem", "J"); On 2016/09/27 00:35:05, agrieve wrote: > On 2016/09/27 00:19:11, boliu wrote: > > we go to great lengths to not hand write jni code. I'm not entirely sure if > jni > > generation works for sdk classes, and if there's an existing example already > > > > how about returning long[], and convert with jni_array.h? > > > > or if this is called frequently and causing gc is not idea, could just have > java > > callback into native with the 4 parameters > > > > I know neither is ideal, but still better than turning sdk mis-use into > runtime > > errors imo > > You can generate wrappers for sdk classes, but I don't think the generator has a > good answer for field access. This seems fine to me, as it even has tests. Yeah, ok. There really is nothing great here :/ Could you save these fields, so they are not looked up every call? https://codereview.chromium.org/2340293003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java (right): https://codereview.chromium.org/2340293003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:22: ActivityManager.MemoryInfo info = new ActivityManager.MemoryInfo(); actually, to avoid gc churn, can you save this object instead of allocating in every call? is there any sense how frequently this will be called?
https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:58: avail_mem_id_ = env->GetFieldID(clazz.obj(), "availMem", "J"); Thank you for suggestions. PS#4 uses native callback instead of GetFieldID. I slightly prefer this approach but I'm happy to go back to use GetFieldID. What do you think? > Could you save these fields, so they are not looked up every call? The previous CL stores these fields at constructor so this lookups happens only once. https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... File content/browser/memory/memory_monitor_android.h (right): https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/09/27 00:32:25, agrieve wrote: > nit: drop the (c) Done. https://codereview.chromium.org/2340293003/diff/40001/content/browser/memory/... content/browser/memory/memory_monitor_android.h:21: long avail_mem; On 2016/09/27 00:32:25, agrieve wrote: > nit: might be better to type these as either jlong, or int64_t Done. https://codereview.chromium.org/2340293003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java (right): https://codereview.chromium.org/2340293003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:22: ActivityManager.MemoryInfo info = new ActivityManager.MemoryInfo(); On 2016/09/27 03:00:23, boliu wrote: > actually, to avoid gc churn, can you save this object instead of allocating in > every call? is there any sense how frequently this will be called? Done. It's still unclear how much we call this function, but a delta between calls is at least greater than seconds. https://codereview.chromium.org/2340293003/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2340293003/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:1114: "../browser/memory/memory_monitor_linux_unittest.cc", On 2016/09/27 00:19:45, boliu wrote: > different CL? Removed (I failed to re-base correctly). https://codereview.chromium.org/2340293003/diff/60001/content/browser/memory/... File content/browser/memory/memory_monitor_android.cc (right): https://codereview.chromium.org/2340293003/diff/60001/content/browser/memory/... content/browser/memory/memory_monitor_android.cc:35: return (info.avail_mem - info.threshold) >> kMBShift; agrieve@ gave me great advice not to call GetMemoryInfo() every time. I'll try to reduce GetMemoryInfo() calls in a follow-up CL.
lgtm
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2340293003/#ps60001 (title: "Use native callback instead of returning MemoryInfo")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2340293003 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Description was changed from ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... BUG=617492 ========== to ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... NO_DEPENDENCY_CHECKS=true BUG=617492 ==========
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2340293003/diff/60001/content/test/BUILD.gn File content/test/BUILD.gn (left): https://codereview.chromium.org/2340293003/diff/60001/content/test/BUILD.gn#o... content/test/BUILD.gn:1114: "../browser/memory/memory_monitor_linux_unittest.cc", is this correct? now patch shows removing this file, which also not be part of this CL, probably?
https://codereview.chromium.org/2340293003/diff/60001/content/test/BUILD.gn File content/test/BUILD.gn (left): https://codereview.chromium.org/2340293003/diff/60001/content/test/BUILD.gn#o... content/test/BUILD.gn:1114: "../browser/memory/memory_monitor_linux_unittest.cc", On 2016/09/27 05:17:14, boliu wrote: > is this correct? now patch shows removing this file, which also not be part of > this CL, probably? Ah, no. Thanks for pointing this out. I'm really bad at rebasing CLs :(
The CQ bit was unchecked by bashi@chromium.org
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, agrieve@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2340293003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... NO_DEPENDENCY_CHECKS=true BUG=617492 ========== to ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... NO_DEPENDENCY_CHECKS=true BUG=617492 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... NO_DEPENDENCY_CHECKS=true BUG=617492 ========== to ========== Add MemoryMonitorAndroid Use ActivityManager.getMemoryInfo() to implement MemoryMonitor for android. Design doc: https://docs.google.com/document/d/1xHBD29Pi26cwXd7byJd7DhvdhJ8EzODP_uqUUI7J9... NO_DEPENDENCY_CHECKS=true BUG=617492 Committed: https://crrev.com/66264142bb289b0975682b0423685467cb2475f9 Cr-Commit-Position: refs/heads/master@{#421128} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/66264142bb289b0975682b0423685467cb2475f9 Cr-Commit-Position: refs/heads/master@{#421128}
Message was sent while issue was closed.
Given agrieve's comments in the design doc, I'd prefer we move to using a single call to getMemoryInfo to get the threshold, and then simply use base::GetSystemMemoryInfo (which reads from /proc/meminfo). That also allows us to use the same mocks for testing as the other monitors, and unifies the various platforms. https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android_unittest.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android_unittest.cc:29: } On 2016/09/26 23:30:19, bashi1 wrote: > On 2016/09/26 17:56:48, chrisha (slow) wrote: > > A test of the actual GetFreeMemoryUntilCriticalMb logic as well? > > In this test, GetFreeMemoryUntilCriticalMb() returns actual available memory on > the device on which this test is running and we can't specify an expected value > (it may be zero or even negative). > > One option would be using a mock function (like other MemoryMonitor tests) but > what we want to test here is that checking whether JNI bindings and Android APIs > are working, so I didn't add a test of GetFreeMemoryUntilCriticalMb. I'd prefer we use a mock and test the actual logic as well, like the other monitors.
Message was sent while issue was closed.
As commented on the design doc, I'd prefer to defer such kind of optimization until it's really needed. And I'm not sure using base::GetSystemMemoryInfo helps unifying the logic because on Android we will end up calculating different numbers. https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... File content/browser/memory/memory_monitor_android_unittest.cc (right): https://codereview.chromium.org/2340293003/diff/20001/content/browser/memory/... content/browser/memory/memory_monitor_android_unittest.cc:29: } On 2016/09/27 15:47:16, chrisha (slow) wrote: > On 2016/09/26 23:30:19, bashi1 wrote: > > On 2016/09/26 17:56:48, chrisha (slow) wrote: > > > A test of the actual GetFreeMemoryUntilCriticalMb logic as well? > > > > In this test, GetFreeMemoryUntilCriticalMb() returns actual available memory > on > > the device on which this test is running and we can't specify an expected > value > > (it may be zero or even negative). > > > > One option would be using a mock function (like other MemoryMonitor tests) but > > what we want to test here is that checking whether JNI bindings and Android > APIs > > are working, so I didn't add a test of GetFreeMemoryUntilCriticalMb. > > I'd prefer we use a mock and test the actual logic as well, like the other > monitors. Working on this. https://codereview.chromium.org/2382623002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
