Route calls to tcmalloc MallocExtension through allocator agnostic interface

Route calls to tcmalloc MallocExtension through allocator agnostic interface

Our various libraries should not be picking or depending on a particular
allocator choice - that should be an application level choice. This creates
a generic AllocatorExtension interface that can optionally be backed by
entry points to a particular allocator's MallocExtension interface if the
application chooses to.

BUG=125003
TEST=compiles, chrome://tcmalloc works


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

[jamesr, 2012-04-26 21:15:53]

I believe this is ready for review.  The one thing I'm not sure about is the
content_main_runner hook - I'm trying to get as close as possible to running
that logic in a main() if I know that tcmalloc is being used without having to
copy-paste it everywhere under the sun.  Let me know if where I've added the
code is a good place for this or not.

[jam, 2012-04-26 21:28:20]

On 2012/04/26 21:15:53, jamesr wrote:
> I believe this is ready for review.  The one thing I'm not sure about is the
> content_main_runner hook - I'm trying to get as close as possible to running
> that logic in a main() if I know that tcmalloc is being used without having to
> copy-paste it everywhere under the sun.  Let me know if where I've added the
> code is a good place for this or not.

this is a good place if you require this to be run at the very start for all
processes

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator.gy...
base/allocator/allocator.gyp:434: 'allocator_extension.cc',
nit: 2 space tabbing..

http://codereview.chromium.org/10239012/diff/9001/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/10239012/diff/9001/content/app/content_main_ru...
content/app/content_main_runner.cc:323: static void get_stats_thunk(char*
buffer, int buffer_length) {
nit: chrome style is GetStatsThunk. also below

http://codereview.chromium.org/10239012/diff/9001/content/app/content_main_ru...
content/app/content_main_runner.cc:324:
MallocExtension::instance()->GetStats(buffer, buffer_length);
nit: 2 spaces.. also below

http://codereview.chromium.org/10239012/diff/9001/content/app/content_main_ru...
content/app/content_main_runner.cc:365: #if !defined(OS_WIN) &&
defined(USE_TCMALLOC)
nit: if you don't want windows, then move this up inside the second part of the
ifdef above

[willchan no longer on Chromium, 2012-04-26 22:08:48]

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
File base/allocator/allocator_extension.cc (right):

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
base/allocator/allocator_extension.cc:12: static GetStatsFunction*
g_get_stats_function = 0;
nit: s/0/NULL/. I've never seen = 0 for a pointer in chromium code s'all. If you
strongly prefer this style though, I won't argue :)

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
base/allocator/allocator_extension.cc:15: void GetStats(char* buffer, int
buffer_length) {
You may need BASE_EXPORT here too.

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
File base/allocator/allocator_extension.h (right):

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
base/allocator/allocator_extension.h:31: // routine.
You should note that they are not threadsafe.

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_sh...
File base/allocator/allocator_shim.cc (right):

http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_sh...
base/allocator/allocator_shim.cc:233: static void get_stats_thunk(char* buffer,
int buffer_length) {
CamelCase naming.

[jamesr, 2012-04-27 00:00:04]

On 2012/04/26 22:08:48, willchan wrote:
>
http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
> File base/allocator/allocator_extension.cc (right):
> 
>
http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
> base/allocator/allocator_extension.cc:12: static GetStatsFunction*
> g_get_stats_function = 0;
> nit: s/0/NULL/. I've never seen = 0 for a pointer in chromium code s'all. If
you
> strongly prefer this style though, I won't argue :)

I prefer NULL but WebKit has brainwashed me.  Will change.

> 
>
http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
> base/allocator/allocator_extension.cc:15: void GetStats(char* buffer, int
> buffer_length) {
> You may need BASE_EXPORT here too.

Oh?  On all impls of BASE_EXPORT'd functions?  Are these guidelines written down
somewhere, or is it "try things until trybots are green"?

> 
>
http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
> File base/allocator/allocator_extension.h (right):
> 
>
http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_ex...
> base/allocator/allocator_extension.h:31: // routine.
> You should note that they are not threadsafe.

I thought that they were, so long as the function pointers are set before any
threads spin up that try to call the functions.  Are you worried about racing
setting the function ptrs with calling the routines not being threadsafe, or the
actual implementations not being threadsafe?  I think tcmalloc's
MallocExtension::intance()->GetStats() and
MallocExtension::instance()->ReleaseFreeMemory() can be called from any thread
(if not we have some code to fix!)

[willchan no longer on Chromium, 2012-04-27 00:06:10]

On Thu, Apr 26, 2012 at 5:00 PM, <jamesr@chromium.org> wrote:

> On 2012/04/26 22:08:48, willchan wrote:
>
> http://codereview.chromium.**org/10239012/diff/9001/base/**
>
allocator/allocator_extension.**cc<http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_extension.cc>
>
>> File base/allocator/allocator_**extension.cc (right):
>>
>
>
> http://codereview.chromium.**org/10239012/diff/9001/base/**
>
allocator/allocator_extension.**cc#newcode12<http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_extension.cc#newcode12>
>
>> base/allocator/allocator_**extension.cc:12: static GetStatsFunction*
>> g_get_stats_function = 0;
>> nit: s/0/NULL/. I've never seen = 0 for a pointer in chromium code s'all.
>> If
>>
> you
>
>> strongly prefer this style though, I won't argue :)
>>
>
> I prefer NULL but WebKit has brainwashed me.  Will change.
>
>
>
>
> http://codereview.chromium.**org/10239012/diff/9001/base/**
>
allocator/allocator_extension.**cc#newcode15<http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_extension.cc#newcode15>
>
>> base/allocator/allocator_**extension.cc:15: void GetStats(char* buffer,
>> int
>> buffer_length) {
>> You may need BASE_EXPORT here too.
>>
>
> Oh?  On all impls of BASE_EXPORT'd functions?  Are these guidelines
> written down
> somewhere, or is it "try things until trybots are green"?


try things until trybots are green :) but i looked at other code and never
see BASE_EXPORT in the .cc file, so i dunno!


>
>
>
>
> http://codereview.chromium.**org/10239012/diff/9001/base/**
>
allocator/allocator_extension.**h<http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_extension.h>
>
>> File base/allocator/allocator_**extension.h (right):
>>
>
>
> http://codereview.chromium.**org/10239012/diff/9001/base/**
>
allocator/allocator_extension.**h#newcode31<http://codereview.chromium.org/10239012/diff/9001/base/allocator/allocator_extension.h#newcode31>
>
>> base/allocator/allocator_**extension.h:31: // routine.
>> You should note that they are not threadsafe.
>>
>
> I thought that they were, so long as the function pointers are set before
> any
> threads spin up that try to call the functions.  Are you worried about
> racing
> setting the function ptrs with calling the routines not being threadsafe,
> or the
> actual implementations not being threadsafe?  I think tcmalloc's
> MallocExtension::intance()->**GetStats() and
> MallocExtension::instance()->**ReleaseFreeMemory() can be called from any
> thread
> (if not we have some code to fix!)
>

Yes, I meant that the setting is not threadsafe. Sorry for not being clear.


>
>
>
>
>
http://codereview.chromium.**org/10239012/<http://codereview.chromium.org/102...
>

[jamesr, 2012-04-27 05:37:18]

I have no clue wtf is going on with base_nacl_win64, still need some help there.
 Otherwise, PTAL.

[willchan no longer on Chromium, 2012-04-27 19:51:51]

lgtm

http://codereview.chromium.org/10239012/diff/34001/base/allocator/allocator_e...
File base/allocator/allocator_extension_thunks.cc (right):

http://codereview.chromium.org/10239012/diff/34001/base/allocator/allocator_e...
base/allocator/allocator_extension_thunks.cc:13: // This slightly odd
translation unit exists because of the peculularity of how
peculiarity

ah, this makes more sense. a description of this target in the gyp libraries
would help

http://codereview.chromium.org/10239012/diff/34001/base/allocator/allocator_s...
File base/allocator/allocator_shim.cc (right):

http://codereview.chromium.org/10239012/diff/34001/base/allocator/allocator_s...
base/allocator/allocator_shim.cc:234:
MallocExtension::instance()->GetStats(buffer, buffer_length);
indent 2 spaces

http://codereview.chromium.org/10239012/diff/34001/base/allocator/allocator_s...
base/allocator/allocator_shim.cc:238:
MallocExtension::instance()->ReleaseFreeMemory();
ditto

[jam, 2012-04-27 21:04:26]

lgtm

[commit-bot: I haz the power, 2012-04-27 23:32:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/10239012/32003

[commit-bot: I haz the power, 2012-04-27 23:58:46]

Try job failure for 10239012-32003 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-28 00:03:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/10239012/32003

[commit-bot: I haz the power, 2012-04-28 02:56:46]

Change committed as 134434