Enable memory profiler when TC_MALLOC is enabled for

Enable memory profiler on Linux when TC_MALLOC is enabled.

Added a tc_malloc memory extension to get bytes allocated on
current thread (GetBytesAllocatedOnCurrentThread API call).

R=jam@chromium.org,jar@chromium.org
BUG=139667
TEST=test about:profiler after setting the env variable 
     CHROME_PROFILER_TIME=1. In about:profiler, it will
     show the memory allocated per thread. This change
     impacts linux only.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150135

[ramant (doing other things), 2012-07-31 00:15:52]

Hi jar and jam,
  Code to enable to memory profiler for linux (moved the code that was in
allocator_shim.cc into content_app_runner.cc. This  code is in the same place as
willchan's changes to enable other tcmalloc callbacks).

thanks,
raman

[jam, 2012-07-31 15:38:43]

can you explain why this needs to move? i.e. was the code in base not called
before when running chrome/content_shell?

the patch says this is linux only, so why isn't the code in content a few blocks
above inside the posix & !mac section?

[ramant (doing other things), 2012-07-31 16:44:09]

On 2012/07/31 15:38:43, John Abd-El-Malek wrote:
> can you explain why this needs to move? i.e. was the code in base not called
> before when running chrome/content_shell?

jar has mentioned as long as we set up the callback before threads have started,
that it would be ok (memory profiler shows the total number of bytes allocated
per thread).

> 
> the patch says this is linux only, so why isn't the code in content a few
blocks
> above inside the posix & !mac section?

Wanted to share the code for posix & !mac and windows. In Patch Set# 3, I had
the change in posix & !mac  section. I wasn't very happy to see the code
duplication (moved the code to avoid code duplication).

Will update the description that this patch shares the code posix & !mac and
windows (and this change impacts windows also).

Should I move the code back into posix & !mac (and undo change to
allocator_shim.cc)?

many many thanks for your comments.

[jam, 2012-07-31 16:46:24]

On Tue, Jul 31, 2012 at 9:44 AM, <rtenneti@chromium.org> wrote:

> On 2012/07/31 15:38:43, John Abd-El-Malek wrote:
>
>> can you explain why this needs to move? i.e. was the code in base not
>> called
>> before when running chrome/content_shell?
>>
>
> jar has mentioned as long as we set up the callback before threads have
> started,
> that it would be ok (memory profiler shows the total number of bytes
> allocated
> per thread).


I'm not following. what does that have to do with why the code was moved
from base to content?


>
>
>
>  the patch says this is linux only, so why isn't the code in content a few
>>
> blocks
>
>> above inside the posix & !mac section?
>>
>
> Wanted to share the code for posix & !mac and windows. In Patch Set# 3, I
> had
> the change in posix & !mac  section. I wasn't very happy to see the code
> duplication (moved the code to avoid code duplication).
>
> Will update the description that this patch shares the code posix & !mac
> and
> windows (and this change impacts windows also).
>

i see, that's why I was confused (since the change said only linux)


>
> Should I move the code back into posix & !mac (and undo change to
> allocator_shim.cc)?
>
> many many thanks for your comments.
>
>
http://codereview.chromium.**org/10820063/<http://codereview.chromium.org/108...
>

[ramant (doing other things), 2012-07-31 17:02:23]

On 2012/07/31 16:46:24, John Abd-El-Malek wrote:
> On Tue, Jul 31, 2012 at 9:44 AM, <mailto:rtenneti@chromium.org> wrote:
> 
> > On 2012/07/31 15:38:43, John Abd-El-Malek wrote:
> >
> >> can you explain why this needs to move? i.e. was the code in base not
> >> called
> >> before when running chrome/content_shell?
> >>
> >
> > jar has mentioned as long as we set up the callback before threads have
> > started,
> > that it would be ok (memory profiler shows the total number of bytes
> > allocated
> > per thread).
> 
> 
> I'm not following. what does that have to do with why the code was moved
> from base to content?

For windows, this change has changed when we initialize (or set up) the
callbacks. Was trying to explain that moving the code wouldn't have side
effects.
  
Was trying to answer  "was the code in base not called before when running
chrome/content_shell?" (which is a good question). 

I just learned from jar on windows, TCMALLOC could be turned on dynamically (via
env variable).

Will make the change for LINUX only (and will undo the change in
allocator_shim.cc).

thanks
raman

[ramant (doing other things), 2012-07-31 17:32:58]

On windows TC_MALLOC can be enabled dynamically, thus moved this change to posix
& !mac section of content_main_runner.cc.

thanks
raman

[jar (doing other things), 2012-07-31 17:41:48]

Yes... my mistake... I agreed that we could probably coalesce the code...
but forgot about the issue that we have dynamic (run-time) selection of the
allocator on Windows.  As a result, we shouldn't make calls into TCMalloc
on widows unless we're really running TCMalloc... so it is probably better
to leave the selection of this profiler over the allocator_shim.

Jim


On Tue, Jul 31, 2012 at 10:32 AM, <rtenneti@chromium.org> wrote:

> On windows TC_MALLOC can be enabled dynamically, thus moved this change to
> posix
> & !mac section of content_main_runner.cc.
>
> thanks
> raman
>
>
http://codereview.chromium.**org/10820063/<http://codereview.chromium.org/108...
>

[jar (doing other things), 2012-08-01 16:38:19]

LGTM with the exclusion of the file listed below.

http://codereview.chromium.org/10820063/diff/11013/third_party/tcmalloc/vendo...
File third_party/tcmalloc/vendor/src/malloc_extension.cc (right):

http://codereview.chromium.org/10820063/diff/11013/third_party/tcmalloc/vendo...
third_party/tcmalloc/vendor/src/malloc_extension.cc:1: // Copyright (c) 2012,
Google Inc.
I think this file is meant to be a snapshot of exactly what we downloaded (and
already merged), so you shouldn't bother to edit it.

[ramant (doing other things), 2012-08-01 17:04:25]

Made all the changes you had suggested. Thanks much Jim.
-raman

http://codereview.chromium.org/10820063/diff/11013/third_party/tcmalloc/vendo...
File third_party/tcmalloc/vendor/src/malloc_extension.cc (right):

http://codereview.chromium.org/10820063/diff/11013/third_party/tcmalloc/vendo...
third_party/tcmalloc/vendor/src/malloc_extension.cc:1: // Copyright (c) 2012,
Google Inc.
On 2012/08/01 16:38:19, jar wrote:
> I think this file is meant to be a snapshot of exactly what we downloaded (and
> already merged), so you shouldn't bother to edit it.  

Done.

[jar (doing other things), 2012-08-01 18:12:28]

lgtm

[ramant (doing other things), 2012-08-06 17:49:11]

Hi John Abd-El-Malek,
  Would appreciate if you could take a look at this CL.

thanks much,
raman

[jam, 2012-08-06 18:23:25]

lgtm

[commit-bot: I haz the power, 2012-08-06 18:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10820063/10017

[ramant (doing other things), 2012-08-06 18:24:09]

On 2012/08/06 18:23:25, John Abd-El-Malek wrote:
> lgtm

thanks very much John Abd-El-Malek and Jar.

[commit-bot: I haz the power, 2012-08-06 20:00:58]

Change committed as 150135