Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18314006:
Enable DMP for system Chromium WebView on Android (Closed)
|
Created:
7 years, 5 months ago by mnaganov (inactive) Modified:
6 years, 10 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, dmikurube+memory_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, android-webview-reviews_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable DMP for system Chromium WebView on Android
A couple of tweaks are needed in order to enable
compiling inside the Android tree.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212490
Patch Set 1 #Patch Set 2 : Ready for a review #
Total comments: 2
Patch Set 3 : Added a workaround for malloc_usable_size #
Total comments: 2
Patch Set 4 : Using a compiler define for detecting build type #
Total comments: 2
Patch Set 5 : No need for another gyp variable #
Total comments: 2
Patch Set 6 : Removed android_use_tcmalloc==1 condition #
Total comments: 6
Patch Set 7 : Do not indent preprocessor directives #
Messages
Total messages: 29 (0 generated)
lgtm (but I'm not an owner) thanks!! looking forward to see the results! one question below though: https://codereview.chromium.org/18314006/diff/2001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right): https://codereview.chromium.org/18314006/diff/2001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:98: size_t malloc_usable_size(const void* p) __THROW ALIAS(tc_malloc_size); does this work with the NDK, say content shell?
https://codereview.chromium.org/18314006/diff/2001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h (right): https://codereview.chromium.org/18314006/diff/2001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/libc_override_gcc_and_weak.h:98: size_t malloc_usable_size(const void* p) __THROW ALIAS(tc_malloc_size); On 2013/07/11 15:10:11, bulach wrote: > does this work with the NDK, say content shell? Surely, it doesn't. This function is cursed to have different signatures inside the Android tree and in the NDK. Added a workaround, now it compiles in both environments.
I'm not familiar with Android and WebView, I basically defer it to the OWNER. But, a question and a nit. I'm not sure if using "__NR_SYSCALL_BASE" and "__SIGN" is appropriate and enough general. They look a little ad-hoc for this specific case to me. Are they only possible ways, or actually good reasonable ways? Is it acceptable even if these workarounds are upstreamed to the original tcmalloc (gperftools)? https://codereview.chromium.org/18314006/diff/12001/build/android/envsetup_fu... File build/android/envsetup_functions.sh (right): https://codereview.chromium.org/18314006/diff/12001/build/android/envsetup_fu... build/android/envsetup_functions.sh:300: if [[ -n "$CHROME_ANDROID_WEBVIEW_ENABLE_DMP" ]]; then "DMP" is often ambiguous. We usually use "dmprof" to mean the deep memory profiler.
Adding David. David, we have encountered peculiar differences in header files provided in NDK vs. the ones from the Android tree. I couldn't find any explicit way of telling in what environment happens the build. As this is only needed for a specific scenario -- enabling DMProf, I don't want to introduce a high-level define, so I resorted to using little hacks. Can you please comment on whether you finding this acceptable, or is there any cleaner way to resolve these issues.
digit@: Ping!
[-digit, +torne] Switched to an explicit define for detecting build type. Adding Torne for build rules review. Dai, Marcus, can you please reconsider the patch? https://codereview.chromium.org/18314006/diff/12001/build/android/envsetup_fu... File build/android/envsetup_functions.sh (right): https://codereview.chromium.org/18314006/diff/12001/build/android/envsetup_fu... build/android/envsetup_functions.sh:300: if [[ -n "$CHROME_ANDROID_WEBVIEW_ENABLE_DMP" ]]; then On 2013/07/11 17:45:37, Dai Mikurube wrote: > "DMP" is often ambiguous. We usually use "dmprof" to mean the deep memory > profiler. Done.
lgtm, but not an owner everywhere. just one suggestion below: https://codereview.chromium.org/18314006/diff/17001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): https://codereview.chromium.org/18314006/diff/17001/base/allocator/allocator.... base/allocator/allocator.gyp:435: ['OS=="android" and android_use_tcmalloc==1 and android_sdk_build==1', { hmm.. the "use_tcmalloc" seems a bit odd to toggle ANDROID_SDK_BUILD.. maybe just remove that condition?
lgtm
https://codereview.chromium.org/18314006/diff/17001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): https://codereview.chromium.org/18314006/diff/17001/base/allocator/allocator.... base/allocator/allocator.gyp:435: ['OS=="android" and android_use_tcmalloc==1 and android_sdk_build==1', { On 2013/07/15 12:31:59, bulach wrote: > hmm.. the "use_tcmalloc" seems a bit odd to toggle ANDROID_SDK_BUILD.. maybe > just remove that condition? Sorry, I've uploaded a better version already. Torne has pointed out that android_webview_build is actually the indicator for the build inside the Android tree.
lgtm, clarification below: https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.... base/allocator/allocator.gyp:434: ['OS=="android" and android_use_tcmalloc==1 and android_webview_build==1', { I'd still suggest just dropping the "android_use_tcmalloc==1" condition and keeping as just keeping: 'OS=="android" and android_webview_build==1' wdyt?
https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.... base/allocator/allocator.gyp:434: ['OS=="android" and android_use_tcmalloc==1 and android_webview_build==1', { On 2013/07/15 12:36:24, bulach wrote: > I'd still suggest just dropping the "android_use_tcmalloc==1" condition and > keeping as just keeping: > 'OS=="android" and android_webview_build==1' > > wdyt? Currently it's needed for TCMalloc source files only. Torne has suggested to be as specific as possible. You are right that using TCMalloc doesn't actually relate to using an SDK vs. a non-SDK build. Let's see what Dai thinks about this.
On 2013/07/15 12:43:01, Mikhail Naganov (Chromium) wrote: > https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > https://codereview.chromium.org/18314006/diff/21001/base/allocator/allocator.... > base/allocator/allocator.gyp:434: ['OS=="android" and android_use_tcmalloc==1 > and android_webview_build==1', { > On 2013/07/15 12:36:24, bulach wrote: > > I'd still suggest just dropping the "android_use_tcmalloc==1" condition and > > keeping as just keeping: > > 'OS=="android" and android_webview_build==1' > > > > wdyt? > > Currently it's needed for TCMalloc source files only. Torne has suggested to be > as specific as possible. > > You are right that using TCMalloc doesn't actually relate to using an SDK vs. a > non-SDK build. > > Let's see what Dai thinks about this. I'm fine with dropping the condition; it's still only defined in allocator.gyp. I just don't really want this defined in common.gypi where it can be used all over the tree.
exactly: it makes no sense anywhere else but allocator.gyp, but it also feels strange to toggle the value based on whether or not it's using tcmalloc :) so I guess we're all agreeing here?
On 2013/07/15 12:52:07, bulach wrote: > exactly: it makes no sense anywhere else but allocator.gyp, but it also feels > strange to toggle the value based on whether or not it's using tcmalloc :) so I > guess we're all agreeing here? OK, dropped the tcmalloc condition from allocator.gyp.
lgtm, thanks!
Thanks for working on this. https://codereview.chromium.org/18314006/diff/35001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): https://codereview.chromium.org/18314006/diff/35001/base/allocator/allocator.... base/allocator/allocator.gyp:436: }], Hmm, sorry, my comment was confusing. I wanted that the changes on tcmalloc/chromium can be eventually upstreamed to the original gperftools. The difference between the original gperftools and Chromium's branch should be minimum. Are "SDK build" and "non-SDK build" generic build settings on Android, or Chromium-specific? I'm not sure on Android. If they're generic, the best way is to use a stable condition without depending on an external definition like ANDROID_NON_SDK_BUILD, or to import the definition into gperftools' build configuration. (FYI, __NR_SYSCALL_BASE is actually ok if it's a stable condition for past and future Android. I used __ANDROID__ since it looked stable.) If they're Chromium-specific, ANDROID_NON_SDK_BUILD definition is ok. The only way is to accept some difference between the original gperftools and Chromium's branch, and then minimize the difference.
On 2013/07/15 16:48:05, Dai Mikurube wrote: > Thanks for working on this. > > https://codereview.chromium.org/18314006/diff/35001/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > https://codereview.chromium.org/18314006/diff/35001/base/allocator/allocator.... > base/allocator/allocator.gyp:436: }], > Hmm, sorry, my comment was confusing. > > I wanted that the changes on tcmalloc/chromium can be eventually upstreamed to > the original gperftools. The difference between the original gperftools and > Chromium's branch should be minimum. > > Are "SDK build" and "non-SDK build" generic build settings on Android, or > Chromium-specific? I'm not sure on Android. Android in general has the concept of things that are built as part of the OS, versus things that are built with the SDK/NDK, but Chromium is as far as I know unique in that it tries to be both of these things at different times (Chrome for Android, versus the Android WebView). So, this condition is very much a Chromium-specific distinction, and there isn't, as far as I know, any other way to know this other than our custom build flag. > > If they're generic, the best way is to use a stable condition without depending > on an external definition like ANDROID_NON_SDK_BUILD, or to import the > definition into gperftools' build configuration. (FYI, __NR_SYSCALL_BASE is > actually ok if it's a stable condition for past and future Android. I used > __ANDROID__ since it looked stable.) > > If they're Chromium-specific, ANDROID_NON_SDK_BUILD definition is ok. The only > way is to accept some difference between the original gperftools and Chromium's > branch, and then minimize the difference.
What Torne said. Also, I think we are moving towards not depending on tcmalloc when using DMProf to avoid shaking things up by using non-standard allocator (when building in Android). So hopefully those conditional defines are short-term workarounds and will be removed.
Thanks for the information. So, lgtm for heap-profiler things. (Please ask OWNERS for other tcmalloc -- maybe willchan.) BTW, I'm interested in the way how you're making it tcmalloc-independent. I also thought about it, but it had low priority at this time.
Hello, William! Can you please a take a look at this change?
Sorry for the delay, I somehow missed this in my inbox. https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right): https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:161: # include <sys/linux-syscalls.h> Is this a typo? Why is there a space between '#' and 'include'? https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h (right): https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/malloc_hook_mmap_linux.h:46: # include <sys/linux-syscalls.h> You did this here too, I must be misunderstanding what's going on.
https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right): https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:161: # include <sys/linux-syscalls.h> On 2013/07/18 16:20:26, willchan wrote: > Is this a typo? Why is there a space between '#' and 'include'? It's indentation within preprocessor directives. Examples: https://code.google.com/p/chromium/codesearch#search/&q=%22%23%20include%22&s... Probably looks inconsistent with the rest. I can remove it.
https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right): https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:161: # include <sys/linux-syscalls.h> On 2013/07/18 16:24:37, Mikhail Naganov (Chromium) wrote: > On 2013/07/18 16:20:26, willchan wrote: > > Is this a typo? Why is there a space between '#' and 'include'? > > It's indentation within preprocessor directives. Examples: > https://code.google.com/p/chromium/codesearch#search/&q=%2522%2523%2520includ... > > Probably looks inconsistent with the rest. I can remove it. Sorry, the link doesn't work. Just search for "# include" (with quotes) in codesearch.
I've approved this with a caveat and also with the understanding that bulach/torne have done the Android specific review. If that's wrong, please make sure you have an appropriate Android expert go over this. https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right): https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:161: # include <sys/linux-syscalls.h> On 2013/07/18 16:26:02, Mikhail Naganov (Chromium) wrote: > On 2013/07/18 16:24:37, Mikhail Naganov (Chromium) wrote: > > On 2013/07/18 16:20:26, willchan wrote: > > > Is this a typo? Why is there a space between '#' and 'include'? > > > > It's indentation within preprocessor directives. Examples: > > > https://code.google.com/p/chromium/codesearch#search/&q=%252522%252523%252520... > > > > Probably looks inconsistent with the rest. I can remove it. > > Sorry, the link doesn't work. Just search for "# include" (with quotes) in > codesearch. I learned some new things today :) Thanks! Never heard of it and lo and behold it's actually discussed in the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro.... The style guide says it's OK. That said, my preference would be not to use this because I think it's not well known, and confusing to people. I've never seen this before, and I don't think it's used much if at all in core Chromium code (everything in cs.chromium.org seems to indicate it's in chromium/usr/include/... which I don't consider core). If you feel strongly, then I'm willing to let it in. But my preference is not to do so. Up to you. Thanks. LGTM.
On 2013/07/18 16:35:20, willchan wrote: > I've approved this with a caveat and also with the understanding that > bulach/torne have done the Android specific review. If that's wrong, please make > sure you have an appropriate Android expert go over this. > > https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... > File third_party/tcmalloc/chromium/src/base/linux_syscall_support.h (right): > > https://codereview.chromium.org/18314006/diff/35001/third_party/tcmalloc/chro... > third_party/tcmalloc/chromium/src/base/linux_syscall_support.h:161: # include > <sys/linux-syscalls.h> > On 2013/07/18 16:26:02, Mikhail Naganov (Chromium) wrote: > > On 2013/07/18 16:24:37, Mikhail Naganov (Chromium) wrote: > > > On 2013/07/18 16:20:26, willchan wrote: > > > > Is this a typo? Why is there a space between '#' and 'include'? > > > > > > It's indentation within preprocessor directives. Examples: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%25252522%25252523%25... > > > > > > Probably looks inconsistent with the rest. I can remove it. > > > > Sorry, the link doesn't work. Just search for "# include" (with quotes) in > > codesearch. > > I learned some new things today :) Thanks! Never heard of it and lo and behold > it's actually discussed in the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro.... > The style guide says it's OK. > > That said, my preference would be not to use this because I think it's not well > known, and confusing to people. I've never seen this before, and I don't think > it's used much if at all in core Chromium code (everything in http://cs.chromium.org > seems to indicate it's in chromium/usr/include/... which I don't consider core). > > If you feel strongly, then I'm willing to let it in. But my preference is not to > do so. Up to you. Thanks. > OK, removed the indents. Thanks for reviewing! > LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/18314006/54001
Message was sent while issue was closed.
Change committed as 212490
Message was sent while issue was closed.
Hi, is there a tracking bug for this? Is this feature in use? Which bot uses it? I don't see any results for "CHROME_ANDROID_WEBVIEW_ENABLE_DMPROF" on cs.chromium.org) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
