Use system properties for heap profiler in Android instead of environment variables.

Use system properties for heap profiler in Android instead of environment variables.

The following system properties are used instead of the environment variables.
HEAPPROFILE => heapprof
HEAP_PROFILE_ALLOCATION_INTERVAL => heapprof.allocation_interval
HEAP_PROFILE_DEALLOCATION_INTERVAL => heapprof.deallocation_interval
HEAP_PROFILE_INUSE_INTERVAL => heapprof.inuse_interval
HEAP_PROFILE_TIME_INTERVAL => heapprof.time_interval
HEAP_PROFILE_MMAP_LOG => heapprof.mmap_log
HEAP_PROFILE_MMAP => heapprof.mmap
HEAP_PROFILE_ONLY_MMAP => heapprof.only_mmap
DEEP_HEAP_PROFILE => heapprof.deep_heap_profile
HEAP_PROFILE_TYPE_STATISTICS => heapprof.type_statistics

BUG=162208
R=bulach@chromium.org, willchan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198355

[Dai Mikurube (NOT FULLTIME), 2013-05-01 17:26:49]

bulach@,

Could you review it at this from a viewpoint of Android?


willchan@,

ditto from OWNER's viewpoint?

[willchan no longer on Chromium, 2013-05-01 17:48:36]

Is this time sensitive? I have some travel coming up, so I may want to
delay reviewing this CL for up to a week. If you have enough flexibility
such that that would not be an issue, that'd be great. Otherwise, I will
endeavor to review as soon as I can.


On Wed, May 1, 2013 at 2:26 PM, <dmikurube@chromium.org> wrote:

> Reviewers: bulach, willchan,
>
> Message:
> bulach@,
>
> Could you review it at this from a viewpoint of Android?
>
>
> willchan@,
>
> ditto from OWNER's viewpoint?
>
> Description:
> Use system properties for heap profiler in Android instead of environment
> variables.
>
> The following system properties are used instead of the environment
> variables.
> HEAPPROFILE => heapprof
> HEAP_PROFILE_ALLOCATION_**INTERVAL => heapprof.allocation_interval
> HEAP_PROFILE_DEALLOCATION_**INTERVAL => heapprof.deallocation_interval
> HEAP_PROFILE_INUSE_INTERVAL => heapprof.inuse_interval
> HEAP_PROFILE_TIME_INTERVAL => heapprof.time_interval
> HEAP_PROFILE_MMAP_LOG => heapprof.mmap_log
> HEAP_PROFILE_MMAP => heapprof.mmap
> HEAP_PROFILE_ONLY_MMAP => heapprof.only_mmap
> DEEP_HEAP_PROFILE => heapprof.deep_heap_profile
> HEAP_PROFILE_TYPE_STATISTICS => heapprof.type_statistics
>
> BUG=162208
>
> Please review this at
https://codereview.chromium.**org/14645016/<https://codereview.chromium.org/1...
>
> SVN Base:
https://chromium.googlesource.**com/chromium/src.git@master<https://chromium....
>
> Affected files:
>   M third_party/tcmalloc/chromium/**src/base/commandlineflags.h
>   M third_party/tcmalloc/chromium/**src/base/sysinfo.cc
>   M third_party/tcmalloc/chromium/**src/deep-heap-profile.h
>   M third_party/tcmalloc/chromium/**src/deep-heap-profile.cc
>   M third_party/tcmalloc/chromium/**src/heap-profiler.cc
>
>
>

[bulach, 2013-05-01 17:55:30]

lgtm on the android side..

will: thanks for the heads up! :) 
this is not time sensitive: we're working on bringing the DMP, Deep Memory
Profiler to android, and one of the requirements is to bring up TCMalloc...
myself and dai still have a bunch of work to do locally, so waiting for a week
or so is not that troublesome...

FYI, we'll eventually send you another meaty patch (right now it's a WIP, so
don't spend time on it...)
https://codereview.chromium.org/14321006/

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:125: static char
system_property_value[PROP_VALUE_MAX];
nit: s/static//

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:132: char
system_property_value[PROP_VALUE_MAX];
nit: const char kTrueValues[] = "tTyY1"; then sizeof(kTrueValues);

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profiler.cc:84: #if defined(__ANDROID__)
|| defined(ANDROID)
nit: perhaps add a short explanation here?

// On android, there are no environment variables.
// Instead, we use system properties, set via:
//   adb shell setprop prop_name prop_value
// From <sys/system_properties.h>,
//   PROP_NAME_MAX   32
//   PROP_VALUE_MAX  92

[Dai Mikurube (NOT FULLTIME), 2013-05-01 18:10:58]

willchan@,

It's not time sensitive as bulach@ says.  I'm involved in working with bulach@
for Android till mid May (coding maybe till May 17).

[Dai Mikurube (NOT FULLTIME), 2013-05-02 16:26:40]

Thanks for the comments, bulach@.

Updated the patch.

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:125: static char
system_property_value[PROP_VALUE_MAX];
On 2013/05/01 17:55:30, bulach wrote:
> nit: s/static//

It actually needs static since it returns the string.  (Returning a pointer to a
static variable is not so good, but it's a behavior similar to getenv.)

Added a comment.

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:132: char
system_property_value[PROP_VALUE_MAX];
On 2013/05/01 17:55:30, bulach wrote:
> nit: const char kTrueValues[] = "tTyY1"; then sizeof(kTrueValues);

Done.

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://codereview.chromium.org/14645016/diff/6001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profiler.cc:84: #if defined(__ANDROID__)
|| defined(ANDROID)
On 2013/05/01 17:55:30, bulach wrote:
> nit: perhaps add a short explanation here?
> 
> // On android, there are no environment variables.
> // Instead, we use system properties, set via:
> //   adb shell setprop prop_name prop_value
> // From <sys/system_properties.h>,
> //   PROP_NAME_MAX   32
> //   PROP_VALUE_MAX  92

Done.

[willchan no longer on Chromium, 2013-05-03 14:12:20]

Only one comment. Otherwise, LGTM.

https://codereview.chromium.org/14645016/diff/15001/third_party/tcmalloc/chro...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/14645016/diff/15001/third_party/tcmalloc/chro...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:134: const char
kTrueValues[] = "tTyY1";
Perhaps you should make this static, because the compiler is going to have to
initialize this local array on every call otherwise.

[Dai Mikurube (NOT FULLTIME), 2013-05-03 14:38:42]

Thanks while your travel, willchan.  Checking the box...

[commit-bot: I haz the power, 2013-05-03 14:38:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/14645016/21001

[commit-bot: I haz the power, 2013-05-03 15:04:29]

Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-03 16:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/14645016/21001

[commit-bot: I haz the power, 2013-05-03 16:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/14645016/21001

[commit-bot: I haz the power, 2013-05-03 23:26:32]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-05-04 19:09:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/14645016/21001

[commit-bot: I haz the power, 2013-05-05 03:47:47]

Change committed as 198355