Expose memory allocator internal stats and properties.

Expose memory allocator internal stats and properties.


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

[alexeif, 2012-08-08 17:06:58]

needed for https://chromiumcodereview.appspot.com/10823205/

[jamesr, 2012-08-09 01:22:12]

You need a base/allocator/ reviewer - I suggest willchan@ (he did the last
allocator thunks review and understands this area pretty wel).

[alexeif, 2012-08-10 18:44:19]

Jim, could you please take a look. Thanks!

[jar (doing other things), 2012-08-11 03:39:46]

Several nits below to consider... or say why you disagree.

Thanks.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:14:
base::allocator::thunks::GetGetPropertyFunction())
nit: No reason to put a declaration in an if.... so I'd suggest pulling it out
for easier reading.
That said, if you make the "if" line longer than one line, then you need
curlies.  

IMO, pull the declaration out, then if you want to be "fancy, " do something
closer to:

return (get_property_function != NULL) &&
    get_property_function(name, value);

Also... you might use:
property_getter()

as a shorter name for your variable |get_property_function|.

The current variable name seems to use a Hungarian notation, of putting the type
of "_function" on the end of the variable name.  I don't know if that is a
special common thing for thunks... but it just grows your variable names to be
bigger than needed IMO.

Given that the style guide specifically asks us to avoid Hungarian notations,
you might similarly change the other postfixed names in this file to something
like:

free_memory_releaser for release_free_memory_function
and
stats_getter for get_stats_function

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:16: else
nit: no need for else... as you're returning.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:31:
base::allocator::thunks::GetReleaseFreeMemoryFunction())
nit: I think this should just be a 4 character indent, rather than 4 characters
atop the paren.  Same thing on line 23.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension_thunks.cc (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension_thunks.cc:41: ReleaseFreeMemoryFunction*
release_free_memory_function) {
nit: 4 character indent.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension_thunks.h (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension_thunks.h:18: typedef bool
GetPropertyFunction(const char*, size_t*);
nit: I think you should provide variable names for the args.

It is not clear why we picked up the (older) TCMalloc style here... and we're
writing prototypes without var names, etc. etc.

I think this is Chrome only code... so despite linking to TCMalloc, we should
probably comply with our style guide.

Personally, I'd also rather see a typedef for a pointer to function... as it is
jarring to see a typedef for a function.

I'd rather see:

typedef bool (*GetPropertyFunction)(const char* name, size_t * value);

with the related changes on next lines (and in cc files).

...but that is personal... and I can't see style-guide support.

I also see the existing code used that style... so if you are in any way
convinced, you should probably change them all to be consistent.

[alexeif, 2012-08-11 11:19:55]

Thanks for your comments! PTAL

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:14:
base::allocator::thunks::GetGetPropertyFunction())
On 2012/08/11 03:39:46, jar wrote:
> nit: No reason to put a declaration in an if.... so I'd suggest pulling it out
> for easier reading.
> That said, if you make the "if" line longer than one line, then you need
> curlies.  
> 
> IMO, pull the declaration out, then if you want to be "fancy, " do something
> closer to:
> 
> return (get_property_function != NULL) &&
>     get_property_function(name, value);
> 
> Also... you might use:
> property_getter()
> 
> as a shorter name for your variable |get_property_function|.
> 
> The current variable name seems to use a Hungarian notation, of putting the
type
> of "_function" on the end of the variable name.  I don't know if that is a
> special common thing for thunks... but it just grows your variable names to be
> bigger than needed IMO.
> 
> Given that the style guide specifically asks us to avoid Hungarian notations,
> you might similarly change the other postfixed names in this file to something
> like:
> 
> free_memory_releaser for release_free_memory_function
> and
> stats_getter for get_stats_function

I don't like assignments inside 'if' either. I just tried to make it consistent
with the rest of the original code. You know I'm kinda guest in these files, so
I tried to keep the changes minimal.

Fixed it.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:16: else
On 2012/08/11 03:39:46, jar wrote:
> nit: no need for else... as you're returning.

Done.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension.cc:31:
base::allocator::thunks::GetReleaseFreeMemoryFunction())
On 2012/08/11 03:39:46, jar wrote:
> nit: I think this should just be a 4 character indent, rather than 4
characters
> atop the paren.  Same thing on line 23.

Done.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension_thunks.cc (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension_thunks.cc:41: ReleaseFreeMemoryFunction*
release_free_memory_function) {
On 2012/08/11 03:39:46, jar wrote:
> nit: 4 character indent.

Done.

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
File base/allocator/allocator_extension_thunks.h (right):

https://chromiumcodereview.appspot.com/10825250/diff/1/base/allocator/allocat...
base/allocator/allocator_extension_thunks.h:18: typedef bool
GetPropertyFunction(const char*, size_t*);
On 2012/08/11 03:39:46, jar wrote:
> nit: I think you should provide variable names for the args.
> 
> It is not clear why we picked up the (older) TCMalloc style here... and we're
> writing prototypes without var names, etc. etc.
> 
> I think this is Chrome only code... so despite linking to TCMalloc, we should
> probably comply with our style guide.
> 
> Personally, I'd also rather see a typedef for a pointer to function... as it
is
> jarring to see a typedef for a function.
> 
> I'd rather see:
> 
> typedef bool (*GetPropertyFunction)(const char* name, size_t * value);
> 
> with the related changes on next lines (and in cc files).
> 
> ...but that is personal... and I can't see style-guide support.
> 
> I also see the existing code used that style... so if you are in any way
> convinced, you should probably change them all to be consistent.

Done.

[jar (doing other things), 2012-08-11 18:56:50]

base LGTM (you may still need a content reviewer)

[alexeif, 2012-08-12 17:39:57]

Darin, could you please take a look at the tiny change in
content/app/content_main_runner.cc. Thank you!

[alexeif, 2012-09-11 13:29:44]

friendly ping.

[darin (slow to review), 2012-09-11 20:21:13]

LGTM

[commit-bot: I haz the power, 2012-09-12 12:36:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10825250/6001

[commit-bot: I haz the power, 2012-09-12 13:30:14]

Try job failure for 10825250-6001 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[alexeif, 2012-09-14 09:48:56]

Jim, could you please take another look.
I had to add jemalloc.h include for jemalloc_stats_t to fix Windows builds.

[jar (doing other things), 2012-09-15 02:07:44]

lgtm

[commit-bot: I haz the power, 2012-09-17 09:52:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeif@chromium.org/10825250/25001

[commit-bot: I haz the power, 2012-09-17 11:44:11]

Change committed as 157102