Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10113009:
Set Android/V8 memory limits from dalvik.vm.heapsize. (Closed)
|
Created:
8 years, 8 months ago by ulan Modified:
8 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, klobag.chromium, Vyacheslav Egorov (Chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSet Android/V8 memory limits from dalvik.vm.heapsize.
Fix memory usage computation for Android.
BUG=b/6182964
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139038
Patch Set 1 : #
Total comments: 21
Patch Set 2 : Address comments #
Total comments: 10
Patch Set 3 : Move DalvikHeapSize to webkitplatformsupport, do not set V8 heap limits. #Patch Set 4 : Make heap size parsing more robust. #
Total comments: 4
Patch Set 5 : Extract DalvikHeapSize #
Total comments: 4
Patch Set 6 : Move DalvikHeapSize to base/android and make it LazyInstance. #Patch Set 7 : Remove redundant includes #
Total comments: 6
Patch Set 8 : Address comments, tweak memory limits. #
Total comments: 2
Patch Set 9 : Add comment about highUsageDeltaMB. #
Total comments: 2
Patch Set 10 : Use static variable instead of LazyInstance #
Total comments: 4
Patch Set 11 : Address comments, rebase #
Messages
Total messages: 27 (0 generated)
James, I added you to reviewers because you are an owner of src/webkit. Feel free to rubber-stamp after Tony and Erik review if you are not familiar with Android or V8. Tony already reviewed similar CL for clank tree, but we decided that it would be better to land it upstream. Erik, could you please review change in webkit_glue.cc where we set V8 space limits?
LGTM (but I'm not an owner) Just a few stylistic nits. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... File webkit/glue/dalvik_heap_size_android.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.cc:14: Just one blank line between methods. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... File webkit/glue/dalvik_heap_size_android.h (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:11: #include "base/string_number_conversions.h" The two includes above should move to the .cc file. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:13: namespace webkit_glue{ nit: space before { https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:21: ~DalvikHeapSize(); These should both be private since it is only supposed to be used as a singleton. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:23: int HeapSizeMB(); Mark this const. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:26: No need for this linebreak https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:29: int ParseHeapSize(const std::string& str); const https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... File webkit/glue/webkit_glue.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:85: void SetJavaScriptFlags(const std::string& str) { This method's contract seems to be to set the flags in str. In fact, render_process_impl.cc may call it multiple times, so it seems silly to re-set the same flags for android inside this method each time. Can you instead add the heap size related flags in render_process_impl.cc instead of here? Or if it is important to update the test callsites, maybe there is some other way?
LGTM https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... File webkit/glue/webkit_glue.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:89: // Set V8 heap size to 6/8 of the device heap size. Why not 3/4? https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:93: "--max_old_gen_size=%d --max_executable_size=%d", heap_size, code_size); I think it would be cleaner to use SetResourceConstraints from the API instead of SetFlagsFromString. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkitp... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkitp... webkit/glue/webkitplatformsupport_impl.cc:692: // If memory usage is above this threshold, force GC more aggressively. I think in the long run we should try to use the external memory notification of V8 to tell V8 more about what is going on with memory pressure. Calling low memory notification is a big hammer to swing on a phone where a stop the world GC can easily cost 10ms per Mbyte.
Thanks for the comments. I uploaded new patch set. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... File webkit/glue/dalvik_heap_size_android.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.cc:14: On 2012/04/19 16:04:30, tonyg wrote: > Just one blank line between methods. Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... File webkit/glue/dalvik_heap_size_android.h (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:11: #include "base/string_number_conversions.h" On 2012/04/19 16:04:30, tonyg wrote: > The two includes above should move to the .cc file. Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:13: namespace webkit_glue{ On 2012/04/19 16:04:30, tonyg wrote: > nit: space before { Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:21: ~DalvikHeapSize(); On 2012/04/19 16:04:30, tonyg wrote: > These should both be private since it is only supposed to be used as a > singleton. This gives compiler error, Singleton apparently requires both to be public. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:23: int HeapSizeMB(); On 2012/04/19 16:04:30, tonyg wrote: > Mark this const. Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:26: On 2012/04/19 16:04:30, tonyg wrote: > No need for this linebreak Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/dalvik_... webkit/glue/dalvik_heap_size_android.h:29: int ParseHeapSize(const std::string& str); On 2012/04/19 16:04:30, tonyg wrote: > const Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... File webkit/glue/webkit_glue.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:85: void SetJavaScriptFlags(const std::string& str) { On 2012/04/19 16:04:30, tonyg wrote: > This method's contract seems to be to set the flags in str. In fact, > render_process_impl.cc may call it multiple times, so it seems silly to re-set > the same flags for android inside this method each time. > > Can you instead add the heap size related flags in render_process_impl.cc > instead of here? Or if it is important to update the test callsites, maybe there > is some other way? I am not sure about moving dependency on DalvikHeapSize to content/renderer (see also reply to Erik's comment below). James, what do you think? https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:89: // Set V8 heap size to 6/8 of the device heap size. On 2012/04/19 16:52:56, Erik Corry wrote: > Why not 3/4? Done. https://chromiumcodereview.appspot.com/10113009/diff/3001/webkit/glue/webkit_... webkit/glue/webkit_glue.cc:93: "--max_old_gen_size=%d --max_executable_size=%d", heap_size, code_size); On 2012/04/19 16:52:56, Erik Corry wrote: > I think it would be cleaner to use SetResourceConstraints from the API instead > of SetFlagsFromString. Proper place to call SetResourceConstraints seems to be WebKit (https://codesearch.corp.google.com/#chrome/src/third_party/WebKit/Source/WebC... ). To do that we would need to export DalvikHeapSize properly. So we would need to abstract it for all platforms and then put it into PlatformSupport. This would take some time to land. I would prefer to land a quick fix first and then follow up with another CL.
https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... File webkit/glue/dalvik_heap_size_android.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:5: #include "webkit/glue/dalvik_heap_size_android.h" i can't figure out why this class is here. it has nothing to do with WebKit or with glue - so why on earth is it in webkit/glue ? https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:30: DCHECK_EQ('m', str[str.size() - 1]); this seems a bit fragile - you can't have a value of '2g' say? https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:32: base::StringToInt(str.substr(0, str.size() - 1), &result); base::StringToInt() returns a bool indicating if the parse was successful - why aren't we checking it if we're parsing a user-writable value? https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... File webkit/glue/webkit_glue.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... webkit/glue/webkit_glue.cc:90: int heap_size = std::max(32, heap_size_singleton->HeapSizeMB() / 4 * 3); you're clamping the heap size in dalvik_heap_size_android.cc, why are you doing another round of clamping here? https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... webkit/glue/webkit_glue.cc:93: "--max_old_gen_size=%d --max_executable_size=%d", heap_size, code_size); formatting a string flag is really hacky and very likely to break. please use programmatic APIs to control the heap size and if they don't exist ask the V8 team to add them
Thanks for reviewing, uploaded new patch set. https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... File webkit/glue/dalvik_heap_size_android.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:5: #include "webkit/glue/dalvik_heap_size_android.h" On 2012/04/20 00:44:26, jamesr wrote: > i can't figure out why this class is here. it has nothing to do with WebKit or > with glue - so why on earth is it in webkit/glue ? Moved it to webkitplatformsupport_impl.cc, where it is used to set memory limits. https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:30: DCHECK_EQ('m', str[str.size() - 1]); On 2012/04/20 00:44:26, jamesr wrote: > this seems a bit fragile - you can't have a value of '2g' say? Added handling of 'k', 'g' and switched to int64 to avoid overflows. https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.cc:32: base::StringToInt(str.substr(0, str.size() - 1), &result); On 2012/04/20 00:44:26, jamesr wrote: > base::StringToInt() returns a bool indicating if the parse was successful - why > aren't we checking it if we're parsing a user-writable value? Done. https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... File webkit/glue/webkit_glue.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... webkit/glue/webkit_glue.cc:90: int heap_size = std::max(32, heap_size_singleton->HeapSizeMB() / 4 * 3); On 2012/04/20 00:44:26, jamesr wrote: > you're clamping the heap size in dalvik_heap_size_android.cc, why are you doing > another round of clamping here? This heap size is for setting V8 heap limit, which should be smaller than renderer's heap size. https://chromiumcodereview.appspot.com/10113009/diff/11001/webkit/glue/webkit... webkit/glue/webkit_glue.cc:93: "--max_old_gen_size=%d --max_executable_size=%d", heap_size, code_size); On 2012/04/20 00:44:26, jamesr wrote: > formatting a string flag is really hacky and very likely to break. please use > programmatic APIs to control the heap size and if they don't exist ask the V8 > team to add them Okay, removing this part from CL, will set this via API call during WebKit initialization.
James, could you please take another look?
On 2012/04/25 14:43:27, ulan wrote: > James, could you please take another look? Ping.
On 2012/04/30 14:54:11, ulan wrote: > On 2012/04/25 14:43:27, ulan wrote: > > James, could you please take another look? > > Ping. Darin could you please take a look at this CL? James asked to redirect it to another owner of webkit/
why are these android specific mechanisms? http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... File webkit/glue/webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... webkit/glue/webkitplatformsupport_impl.cc:121: class DalvikHeapSize { it seems like this should be factored out into a separate .h/.cc file. http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... webkit/glue/webkitplatformsupport_impl.cc:750: size_t WebKitPlatformSupportImpl::highUsageDeltaMB() { nit: this function name seems a little strange. shouldn't it have the word "memory" in its name? perhaps "highMemoryUsageDeltaMB"? i'm not sure what a "usage delta" is though. can you explain? the comment doesn't explain it very well to me.
Thanks, I uploaded new patch set. > why are these android specific mechanisms? Functions lowMemoryUsageMB(), highMemoryUsageMB(), highUsageDeltaMB() are used to determine when to send a GC request from WebKit to V8 (see third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.cpp). GC request is sent when (currentMemoryUsage > lowMemoryUsageMB() and currentMemoryUsage > 2 * previousMemoryUsage) or (currentMemoryUsage > highMemoryUsageMB() and currentMemoryUsage > previousMemoryUsage + highUsageDeltaMB()) The return values of these functions are hard-coded constants in third_party/WebKit/Source/Platform/chromium/public/Platform.h Since these constants are too large for Android, downstream Android Chrome code just overrides the limit functions in WebKitPlatformSupportImpl to smaller constants. Unfortunately, the constants were chosen too small, and this leads to too aggressive GCs. In this CL I am trying to - upstream existing Android changes in WebKitPlatformSupportImpl; - implement more generic computation of the limit functions based on Android application heap size. http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... File webkit/glue/webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... webkit/glue/webkitplatformsupport_impl.cc:121: class DalvikHeapSize { On 2012/05/09 14:57:35, darin wrote: > it seems like this should be factored out into a separate .h/.cc file. Done. http://codereview.chromium.org/10113009/diff/15002/webkit/glue/webkitplatform... webkit/glue/webkitplatformsupport_impl.cc:750: size_t WebKitPlatformSupportImpl::highUsageDeltaMB() { On 2012/05/09 14:57:35, darin wrote: > nit: this function name seems a little strange. shouldn't it have the word > "memory" in its name? perhaps "highMemoryUsageDeltaMB"? i'm not sure what a > "usage delta" is though. can you explain? the comment doesn't explain it very > well to me. The name is already fixed on WebKit side. Added explanation in reply message.
On 2012/05/09 17:33:27, ulan wrote: > Thanks, I uploaded new patch set. > > > why are these android specific mechanisms? > > Functions lowMemoryUsageMB(), highMemoryUsageMB(), highUsageDeltaMB() are used > to determine when to send a GC request from WebKit to V8 > (see third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.cpp). > > GC request is sent when > > (currentMemoryUsage > lowMemoryUsageMB() > and currentMemoryUsage > 2 * previousMemoryUsage) > or > (currentMemoryUsage > highMemoryUsageMB() > and currentMemoryUsage > previousMemoryUsage + highUsageDeltaMB()) > > The return values of these functions are hard-coded constants in > third_party/WebKit/Source/Platform/chromium/public/Platform.h > > Since these constants are too large for Android, downstream Android Chrome code > just overrides the limit functions in WebKitPlatformSupportImpl to smaller > constants. Unfortunately, the constants were chosen too small, and this leads to > too aggressive GCs. OK > In this CL I am trying to > - upstream existing Android changes in WebKitPlatformSupportImpl; > - implement more generic computation of the limit functions based on Android > application heap size. OK > > nit: this function name seems a little strange. shouldn't it have the word > > "memory" in its name? perhaps "highMemoryUsageDeltaMB"? i'm not sure what a > > "usage delta" is though. can you explain? the comment doesn't explain it > very > > well to me. > > The name is already fixed on WebKit side. Added explanation in reply message. Why is the name fixed? I don't understand. It takes time, but it should be trivial to improve the name of a WebKit API function.
http://codereview.chromium.org/10113009/diff/26002/webkit/glue/dalvik_heap_si... File webkit/glue/dalvik_heap_size_android.h (right): http://codereview.chromium.org/10113009/diff/26002/webkit/glue/dalvik_heap_si... webkit/glue/dalvik_heap_size_android.h:10: namespace webkit_glue { nit: add a new line here http://codereview.chromium.org/10113009/diff/26002/webkit/glue/webkitplatform... File webkit/glue/webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10113009/diff/26002/webkit/glue/webkitplatform... webkit/glue/webkitplatformsupport_impl.cc:688: // If memory usage is below this threshold, do not bother forcing GC. Are you expecting this function to be called on multiple threads? I'm pretty sure that using the Singleton pattern to allocate a DalvikHeapSize object is overkill. You just have an integer value that could be held as a static. If you need it to be a thread-safe static, then perhaps you should use LazyInstance. But, I'd expect that a simple global function to query the dalvik heapsize would do. size_t QueryDalvikVMHeapSize(); I'm also curious if this should be part of base/sys_info.h instead.
Thank you for suggestions, Darin. I addressed the comments and uploaded a new patch set. Could you please take another look? > Why is the name fixed? I don't understand. It takes time, but it should be trivial to improve the name of a WebKit API function. Changing the WebKit function name is easy, but that would make this CL dependent on the corresponding WebKit CL. This would complicated cherry-picking of this CL to older branches in the downstream Android Chrome. If you don't mind, I will submit the function name change in separate CLs after this CL lands. https://chromiumcodereview.appspot.com/10113009/diff/26002/webkit/glue/dalvik... File webkit/glue/dalvik_heap_size_android.h (right): https://chromiumcodereview.appspot.com/10113009/diff/26002/webkit/glue/dalvik... webkit/glue/dalvik_heap_size_android.h:10: namespace webkit_glue { On 2012/05/14 16:23:49, darin wrote: > nit: add a new line here Done. https://chromiumcodereview.appspot.com/10113009/diff/26002/webkit/glue/webkit... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/26002/webkit/glue/webkit... webkit/glue/webkitplatformsupport_impl.cc:688: // If memory usage is below this threshold, do not bother forcing GC. On 2012/05/14 16:23:49, darin wrote: > Are you expecting this function to be called on multiple threads? > > I'm pretty sure that using the Singleton pattern to allocate a DalvikHeapSize > object is overkill. You just have an integer value that could be held as a > static. If you need it to be a thread-safe static, then perhaps you should use > LazyInstance. But, I'd expect that a simple global function to query the dalvik > heapsize would do. > > size_t QueryDalvikVMHeapSize(); > > I'm also curious if this should be part of base/sys_info.h instead. Thanks, converted DalvikHeapSize to LazyInstance since it can be called from multiple threads. Moved it to base/android and made it accessible via a static method of SysInfo.
https://chromiumcodereview.appspot.com/10113009/diff/30004/webkit/glue/webkit... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/30004/webkit/glue/webkit... webkit/glue/webkitplatformsupport_impl.cc:685: return base::SysInfo::DalvikHeapSizeMB() / 4; This seems too conservative. We ever used the dalvik heap size (got from Java ActivityManager.getLargeMemoryClass() which is also the value of system property dalvik.vm.heapsize) as the return value of lowMemoryUsageMB(), and 1.5 time of it as the return value of highMemoryUsageMB(). As discussed with klobag@ and vegorov@ yesterday, V8::LowMemoryNotification should be only called when the process meets critical memory condition. Too small return values of these functions will result to many unnecessary expensive V8::LowMemoryNotification calls.
https://chromiumcodereview.appspot.com/10113009/diff/30004/webkit/glue/webkit... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/30004/webkit/glue/webkit... webkit/glue/webkitplatformsupport_impl.cc:685: return base::SysInfo::DalvikHeapSizeMB() / 4; After further testing, I found that the change about memoryUsageMB() (which is now about 1/5 of the original memoryUsageMBGeneric()) made these values not that conservative. Now the values look good to me and I'd discuss internally for further optimization of them.
https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... File base/android/dalvik_heap_size.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... base/android/dalvik_heap_size.cc:8: Should include "base/logging.h" for CHECK and DCHECK. (encountered error when building with this patch in an Android downstream branch) https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... base/android/dalvik_heap_size.cc:13: namespace android{ Nit: space before '{'
Thanks, uploaded new patch set. https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... File base/android/dalvik_heap_size.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... base/android/dalvik_heap_size.cc:8: On 2012/05/15 21:10:25, Xianzhu wrote: > Should include "base/logging.h" for CHECK and DCHECK. > (encountered error when building with this patch in an Android downstream > branch) Added "base/logging.h" and removed "base/memory/singleton.h" (for some reason I don't get compile error without logging.h) https://chromiumcodereview.appspot.com/10113009/diff/30004/base/android/dalvi... base/android/dalvik_heap_size.cc:13: namespace android{ On 2012/05/15 21:10:25, Xianzhu wrote: > Nit: space before '{' Done.
Patch looks good, just one comment.. https://chromiumcodereview.appspot.com/10113009/diff/33009/webkit/glue/webkit... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/33009/webkit/glue/webkit... webkit/glue/webkitplatformsupport_impl.cc:696: // Avoid constant V8 GC when memory usage equals to working set estimate. Can you mention here that this should be sufficiently higher than the memory used by v8 internal data structures (8M?) so any changes to this in future would be done with that in mind?
https://chromiumcodereview.appspot.com/10113009/diff/33009/webkit/glue/webkit... File webkit/glue/webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/33009/webkit/glue/webkit... webkit/glue/webkitplatformsupport_impl.cc:696: // Avoid constant V8 GC when memory usage equals to working set estimate. On 2012/05/16 11:23:51, Satish wrote: > Can you mention here that this should be sufficiently higher than the memory > used by v8 internal data structures (8M?) so any changes to this in future would > be done with that in mind? Done.
https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... File base/sys_info_linux.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... base/sys_info_linux.cc:20: static base::LazyInstance<android::DalvikHeapSize> g_dalvik_heap_size = nit: shouldn't this be in a sys_info_android.cc file? nit: why do you need LazyInstance? it seems like you could also just let it race at startup since it doesn't hurt to compute the Dalvik heap size twice. int SysInfo::DalvikHeapSizeMB() { static int heap_size = GetDalvikHeapSizeMB(); return heap_size; } If this lives in a file named sys_info_android.cc, then you could also just put the GetDalvikHeapSizeMB() function implementation into an anonymous namespace within this file. No need for a class, etc.
https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... File base/sys_info_linux.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... base/sys_info_linux.cc:20: static base::LazyInstance<android::DalvikHeapSize> g_dalvik_heap_size = On 2012/05/16 23:55:03, darin wrote: > nit: shouldn't this be in a sys_info_android.cc file? > > nit: why do you need LazyInstance? it seems like you could also just let it > race at startup since it doesn't hurt to compute the Dalvik heap size twice. > > int SysInfo::DalvikHeapSizeMB() { > static int heap_size = GetDalvikHeapSizeMB(); > return heap_size; > } > > If this lives in a file named sys_info_android.cc, then you could also just put > the GetDalvikHeapSizeMB() function implementation into an anonymous namespace > within this file. No need for a class, etc. Done. I am not sure if __system_property_get is thread-safe or not. Its implementation in bionic/libc/bionic/system_properties.c looks thread-safe. I will check with Android NDK people to be sure.
On 2012/05/18 10:24:06, ulan wrote: > https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... > File base/sys_info_linux.cc (right): > > https://chromiumcodereview.appspot.com/10113009/diff/38009/base/sys_info_linu... > base/sys_info_linux.cc:20: static base::LazyInstance<android::DalvikHeapSize> > g_dalvik_heap_size = > On 2012/05/16 23:55:03, darin wrote: > > nit: shouldn't this be in a sys_info_android.cc file? > > > > nit: why do you need LazyInstance? it seems like you could also just let it > > race at startup since it doesn't hurt to compute the Dalvik heap size twice. > > > > int SysInfo::DalvikHeapSizeMB() { > > static int heap_size = GetDalvikHeapSizeMB(); > > return heap_size; > > } > > > > If this lives in a file named sys_info_android.cc, then you could also just > put > > the GetDalvikHeapSizeMB() function implementation into an anonymous namespace > > within this file. No need for a class, etc. > > Done. > > I am not sure if __system_property_get is thread-safe or not. Its implementation > in bionic/libc/bionic/system_properties.c looks thread-safe. I will check with > Android NDK people to be sure. Android NDK people confirmed that __system_property_get is thread-safe. Darin, could you please take another look?
LGTM http://codereview.chromium.org/10113009/diff/40002/base/sys_info_android.cc File base/sys_info_android.cc (right): http://codereview.chromium.org/10113009/diff/40002/base/sys_info_android.cc#n... base/sys_info_android.cc:14: int ParseHeapSize(const std::string& str) { nit: consider changing the parameter to base::StringPiece. the code should just all be the same otherwise, and then you won't need the std::string(heap_size_str) at the call-site. http://codereview.chromium.org/10113009/diff/40002/base/sys_info_linux.cc File base/sys_info_linux.cc (right): http://codereview.chromium.org/10113009/diff/40002/base/sys_info_linux.cc#new... base/sys_info_linux.cc:10: #include "base/lazy_instance.h" nit: no longer need this include
Comments addressed, landing. https://chromiumcodereview.appspot.com/10113009/diff/40002/base/sys_info_andr... File base/sys_info_android.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/40002/base/sys_info_andr... base/sys_info_android.cc:14: int ParseHeapSize(const std::string& str) { On 2012/05/24 22:27:43, darin wrote: > nit: consider changing the parameter to base::StringPiece. > the code should just all be the same otherwise, and then you won't need the > std::string(heap_size_str) at the call-site. Done. https://chromiumcodereview.appspot.com/10113009/diff/40002/base/sys_info_linu... File base/sys_info_linux.cc (right): https://chromiumcodereview.appspot.com/10113009/diff/40002/base/sys_info_linu... base/sys_info_linux.cc:10: #include "base/lazy_instance.h" On 2012/05/24 22:27:43, darin wrote: > nit: no longer need this include Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ulan@chromium.org/10113009/44003
Change committed as 139038 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
