[tracing] Fix resident size of malloc dump provider when tcmalloc is used

[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.
To use GetNumericProperty function from base/trace_event, new api is
added in allocator_extension.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this should be added in tcmalloc.

BUG=546491
Committed: https://crrev.com/0943409e7b60ead1aa0b4f4ef8b4de5617b81917
Cr-Commit-Position: refs/heads/master@{#356162}

[ssid, 2015-10-22 16:34:52]

Description was changed from

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. This can be obtained by parsing the
GetStats() string, and will be done in follow up CL.

BUG=546491
==========

to

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches and the malloc metadata bytes by
parsing the GetStats() string.

BUG=546491
==========

[ssid, 2015-10-22 16:41:45]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2015-10-22 16:41:45]

PTAL, Thanks.

[ssid, 2015-10-22 16:53:19]

Patchset #3 (id:40001) has been deleted

[ssid, 2015-10-22 16:58:57]

Patchset #3 (id:60001) has been deleted

[ssid, 2015-10-22 17:02:06]

Patchset #3 (id:80001) has been deleted

[Primiano Tucci (use gerrit), 2015-10-22 17:24:04]

On 2015/10/22 16:41:45, ssid wrote:
> PTAL, Thanks.

Parsing a debug string used for himan consumption looks pretty fragile.
Is there any way we can use MallocExtension::GetNumericProperty() ?
I'd guess the same numbers are available from there?

[ssid, 2015-10-22 17:26:39]

On 2015/10/22 17:24:04, Primiano Tucci wrote:
> On 2015/10/22 16:41:45, ssid wrote:
> > PTAL, Thanks.
> 
> Parsing a debug string used for himan consumption looks pretty fragile.
> Is there any way we can use MallocExtension::GetNumericProperty() ?
> I'd guess the same numbers are available from there?

Hm I could use the MallocExtension::GetNumericProperty functino to get
approximate values. The malloc metadata size is not given by any of the property
in this method. The metadata accounts for around 1-2 Mib usually. Please see
patch set 1 for the difference.

[ssid, 2015-10-22 17:59:36]

On 2015/10/22 17:26:39, ssid wrote:
> On 2015/10/22 17:24:04, Primiano Tucci wrote:
> > On 2015/10/22 16:41:45, ssid wrote:
> > > PTAL, Thanks.
> > 
> > Parsing a debug string used for himan consumption looks pretty fragile.
> > Is there any way we can use MallocExtension::GetNumericProperty() ?
> > I'd guess the same numbers are available from there?
> 
> Hm I could use the MallocExtension::GetNumericProperty functino to get
> approximate values. The malloc metadata size is not given by any of the
property
> in this method. The metadata accounts for around 1-2 Mib usually. Please see
> patch set 1 for the difference.

If you think it is fine to not display this metadata size then I could use patch
set 1.
If you think this should be included then I see 2 options:
1. Change the tcmalloc api to return a resident size or metadata size. This has
to be done in main library and then rolled to chrome, would take a very long
time.
2. Parse the getStats String as done here. Have to hope that the get stats
format does not change. Though if it changes the checks will fail.

[Primiano Tucci (use gerrit), 2015-10-26 13:16:45]

On 2015/10/22 17:59:36, ssid wrote:
> On 2015/10/22 17:26:39, ssid wrote:
> > On 2015/10/22 17:24:04, Primiano Tucci wrote:
> > > On 2015/10/22 16:41:45, ssid wrote:
> > > > PTAL, Thanks.
> > > 
> > > Parsing a debug string used for himan consumption looks pretty fragile.
> > > Is there any way we can use MallocExtension::GetNumericProperty() ?
> > > I'd guess the same numbers are available from there?
> > 
> > Hm I could use the MallocExtension::GetNumericProperty functino to get
> > approximate values. The malloc metadata size is not given by any of the
> property
> > in this method. The metadata accounts for around 1-2 Mib usually. Please see
> > patch set 1 for the difference.
> 
> If you think it is fine to not display this metadata size then I could use
patch
> set 1.
> If you think this should be included then I see 2 options:
> 1. Change the tcmalloc api to return a resident size or metadata size. This
has
> to be done in main library and then rolled to chrome, would take a very long
> time.
> 2. Parse the getStats String as done here. Have to hope that the get stats
> format does not change. Though if it changes the checks will fail.

Ok I think we what we should do here is:
 - Go back to patchset 1.
 - Try to make a change upstream in tcmalloc to expose the metadata (and
speculatively land this, you can fix the name later)
 - If not, make the change downstream
 - Maybe add also the columns for the central vs threads cache.

[ssid, 2015-10-26 13:55:55]

Description was changed from

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches and the malloc metadata bytes by
parsing the GetStats() string.

BUG=546491
==========

to

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. This can be obtained by parsing the
GetStats() string, and will be done in follow up CL.

BUG=546491
==========

[ssid, 2015-10-26 13:56:23]

Description was changed from

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. This can be obtained by parsing the
GetStats() string, and will be done in follow up CL.

BUG=546491
==========

to

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this shoould be added in tcmalloc.

BUG=546491
==========

[ssid, 2015-10-26 14:05:29]

Patchset #4 (id:120001) has been deleted

[ssid, 2015-10-26 14:05:52]

Done, PTAL Thanks.

[Primiano Tucci (use gerrit), 2015-10-26 14:25:35]

LGTM with two comments

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
base/trace_event/malloc_dump_provider.cc:9: #include
"base/trace_event/process_memory_dump.h"
please add an include "build/build_config.h", otherwise you might end up never
seeing the USE_TCMALLOC define

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
base/trace_event/malloc_dump_provider.cc:56: resident_size = total_virtual_size
- pageheap_unmapped_bytes;
Maybe add a comment with a pointer to the tcmalloc dump code that explains how
did you choose to do this math.

[ssid, 2015-10-26 15:58:25]

Description was changed from

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this shoould be added in tcmalloc.

BUG=546491
==========

to

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this should be added in tcmalloc.

BUG=546491
==========

[ssid, 2015-10-26 17:10:40]

Changed to add api in allocator_extension_thunks , which base can use.

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
base/trace_event/malloc_dump_provider.cc:9: #include
"base/trace_event/process_memory_dump.h"
On 2015/10/26 14:25:35, Primiano Tucci wrote:
> please add an include "build/build_config.h", otherwise you might end up never
> seeing the USE_TCMALLOC define

Done.

https://codereview.chromium.org/1408403004/diff/140001/base/trace_event/mallo...
base/trace_event/malloc_dump_provider.cc:56: resident_size = total_virtual_size
- pageheap_unmapped_bytes;
On 2015/10/26 14:25:35, Primiano Tucci wrote:
> Maybe add a comment with a pointer to the tcmalloc dump code that explains how
> did you choose to do this math.

Done.

[ssid, 2015-10-26 17:13:05]

Description was changed from

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this should be added in tcmalloc.

BUG=546491
==========

to

==========
[tracing] Fix resident size of malloc dump provider when tcmalloc is used

When tcmalloc is used the dump provider only reports the total
allocated objects size as the resident size. but in reality tcmalloc
uses more resident memory than the allocated objects size since it has
thread cache and free list memory that are resident. It also has malloc
meta data that is resident. This CL changes the resident size of malloc
to include tcmalloc's page heap free list sizes, and memory used by
central, transfer and thread caches.
To use GetNumericProperty function from base/trace_event, new api is
added in allocator_extension.

It still does not include the malloc metadata bytes since it is not
returned by the tcmalloc api. So this should be added in tcmalloc.

BUG=546491
==========

[ssid, 2015-10-26 17:14:09]

ssid@chromium.org changed reviewers:
+ wfh@chromium.org

[ssid, 2015-10-26 17:14:09]

+wfh: Please review the changes for exposing the GetNumericProperty method for
tcmalloc. changes in: base/allocator
Thanks.

[Will Harris, 2015-10-26 20:54:32]

lgtm for base/allocator. you'll need to find a content/ owner for the wiring up
of the thunk.

[Primiano Tucci (use gerrit), 2015-10-26 21:00:20]

primiano@chromium.org changed reviewers:
+ sievers@chromium.org

[Primiano Tucci (use gerrit), 2015-10-26 21:00:20]

+sievers for the wire-up in content/

[no sievers, 2015-10-26 21:41:38]

content lgtm

[ssid, 2015-10-26 22:23:30]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-10-26 22:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1408403004/#ps200001
(title: "Nit.")

[commit-bot: I haz the power, 2015-10-26 22:24:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408403004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408403004/200001

[commit-bot: I haz the power, 2015-10-26 23:05:11]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-10-26 23:06:07]

Patchset 7 (id:??) landed as
https://crrev.com/0943409e7b60ead1aa0b4f4ef8b4de5617b81917
Cr-Commit-Position: refs/heads/master@{#356162}