Breakdown nonprofiled memory regions (f.k.a. 'unknown'), and add new policy files.

Breakdown nonprofiled memory regions (f.k.a. 'unknown'), and add new policy files.


It breaks down nonprofiled memory regions (f.k.a. 'unknown') with using information
from /proc/(pid)/maps.
'mustbezero' and 'nonprofiled-*' columns show broken down values.

- mustbezero: works as a sentinel value. it should be '0'. tell dmikurube@ if it > 0.
- nonprofiled-absent: works as a sentinel value. should be '0'.
- nonprofiled-anonymous: may be still 'unknown'. no special info from maps.
- nonprofiled-file-exec: is a mapped file here as executable.
- nonprofiled-file-nonexec: is a mapped file here as non-executable.
- nonprofiled-stack: is a stack.
- nonprofiled-other: includes [vdso] and some small portions.


It also removes dmpolicy (an old one) and adds new policy files: policy.l{0-2}.txt.
The new files 'policy.l{0-2}.txt' are recommended instead of 'dmpolicy'.
policy.l0.txt is the roughest, and policy.l2.txt is the finest breakdown.

We can check that summation from 'FROM_HERE_FOR_TOTAL' till 'UNTIL_HERE_FOR_TOTAL'
is equal to 'total-exclude-profiler'.

Finally, dmprof.py  is renamed into dmprof like pprof in tcmalloc.


BUG=none
TEST=none

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

[Dai Mikurube (NOT FULLTIME), 2012-04-12 12:20:24]

Alexander (, and Jim)

Could you take a look at *.{cc|h} ?

I've Cc'ed Jim.  But, it doesn't affect performance in usual run since it
changes only deep_heap_profile.*.


Marc,

Could you review 'dmprof' the Python file?  (It was 'dmprof.py'.)

http://codereview.chromium.org/9812010/diff/15002/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right):

http://codereview.chromium.org/9812010/diff/15002/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/deep-heap-profile.cc:724: "profiled-malloc",
used_in_buffer, buffer_size, buffer);
This part may be to be considered to simplify.

[M-A Ruel, 2012-04-12 12:40:36]

dmprof lgtm with nits

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:117: self.log_lines = []
Here's an example how to rewrite it in a functional way:
lines = (l.strip() for l in open(self.log_path, 'r'))
self.log_lines = [l for l in lines if l and not l.startswith('#')]

I don't know if you care about whitespaces and \n. Also your code will throw on
empty lines.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:469: sizes['mmap-no-log'] = self.counters[
I think this would be much more readable:
sizes['mmap-no-log'] = (
    self.counters['%s_committed' % mmap_prefix] - sizes['mmap-total-log'])

Same below. Splitting inside list element lookup operator "[]" is a weird place
to split the line. Use parenthesis () to group the lines as one statement
instead.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:501: 'other-vm': 'other_virtual' }.items():
Use iteritems() instead of items()

Otherwise, you're copying it

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:506: sizes['mustbezero'] = self.counters[
Use this format instead:
sizes['mustbezero'] = (
    self.counters['total_committed'] -
    self.counters['%s_committed' % mmap_prefix] -
    self.counters['nonprofiled-absent_committed'] -
...)

But at that point, I'd use a loop:
removed = ('%s_committed' % mmap_prefix, 'nonprofiled-absent_committed',
'nonprofiled-anonymous_committed', ...)
sizes['mustbezero'] = self.counters['total_committed'] - sum(self.counters[i]
for i in removed)

[Alexander Potapenko, 2012-04-12 12:51:33]

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:31: # Heap Profile Dump versions
Are you going to support all the versions? I guess at some moment the list can
be flattened to contain only 1-2 last versions.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:51: DUMP_DEEP_4 = 'DUMP_DEEP_4'
I think it's time to stop adding DUMP_DEEP_X constants. Maybe just parse
log_version string and strip DUMP_DEEP_ away instead?

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:120: if log_line[0] != '#':
I think "not log_line.startswith('#')" is more readable, but YMMV.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:323: ln: An integer representing the starting
line number in log_lines.
Please don't select short cryptic names for the function arguments. Let the
reader understand the meaning of the argument from its name.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:325: if self.log_version == DUMP_DEEP_3 or
self.log_version == DUMP_DEEP_4:
self.log_version in [DUMP_DEEP_3, DUMP_DEEP_4]

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:326: sys.stderr.write('  heap profile dump
version: %s\n' % self.log_version)
Please move this line to the top of the method, as it's printed for every log
version.
If you don't feel like checking for all the versions at the top of the function
(that should become easier if you drop all those DUMP_DEEP_ constants), at least
consider putting the common string into a constant.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:386: A string representing a version of the
stacktrace dump.  '' for invalid
Looks like you're returning two values here.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:398: if self.log_lines[ln].startswith('heap
profile: '):
I guess too much machinery is involved in getting the version. Are there ways to
get an incorrect heap profile? Can you just always include the version?

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:406: sys.stderr.write('  invalid heap profile
dump version:%s\n' % version)
Maybe just raise an exception here? You won't need to deal with an incorrect
version string this way.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:412: 
spare newline

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:484: for key, value in {
This can be generated automatically:
  map(lambda x: x + '_committed', ['total', 'filemapped'])
is ['total_committed', 'total_filemapped']

[Alexander Potapenko, 2012-04-12 13:01:18]

the rest of the file LGTM

[Dai Mikurube (NOT FULLTIME), 2012-04-13 05:41:10]

Thanks Alexander, Marc,

I fixed nits.  Old versions are still alive since I have much old data locally
and I'm comparing them.  I'm planning to remove old versions at some future
date.

I'll be committing this change.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:31: # Heap Profile Dump versions
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> Are you going to support all the versions? I guess at some moment the list can
> be flattened to contain only 1-2 last versions.

I'm planning to support all versions for a while since I have much old data
locally and I'm comparing them.  I'll be flatten them at some future date.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:51: DUMP_DEEP_4 = 'DUMP_DEEP_4'
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> I think it's time to stop adding DUMP_DEEP_X constants. Maybe just parse
> log_version string and strip DUMP_DEEP_ away instead?

I was planning to add other versions than 'DUMP_DEEP_', but I guess it'll need
more time.  I'll change it in another CL or the next DUMP_DEEP_ version.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:117: self.log_lines = []
On 2012/04/12 12:40:36, Marc-Antoine Ruel wrote:
> Here's an example how to rewrite it in a functional way:
> lines = (l.strip() for l in open(self.log_path, 'r'))
> self.log_lines = [l for l in lines if l and not l.startswith('#')]
> 
> I don't know if you care about whitespaces and \n. Also your code will throw
on
> empty lines.

Finally, replaced it with:
self.log_lines = [l for l in open(self.log_path, 'r') if l and not
l.startswith('#')]

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:120: if log_line[0] != '#':
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> I think "not log_line.startswith('#')" is more readable, but YMMV.

Exactly.  I used startswith as above.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:323: ln: An integer representing the starting
line number in log_lines.
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> Please don't select short cryptic names for the function arguments. Let the
> reader understand the meaning of the argument from its name.

Done.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:325: if self.log_version == DUMP_DEEP_3 or
self.log_version == DUMP_DEEP_4:
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> self.log_version in [DUMP_DEEP_3, DUMP_DEEP_4]

Done.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:326: sys.stderr.write('  heap profile dump
version: %s\n' % self.log_version)
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> Please move this line to the top of the method, as it's printed for every log
> version.
> If you don't feel like checking for all the versions at the top of the
function
> (that should become easier if you drop all those DUMP_DEEP_ constants), at
least
> consider putting the common string into a constant. 

Done.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:386: A string representing a version of the
stacktrace dump.  '' for invalid
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> Looks like you're returning two values here.

Modified as follows :

Returns:
A pair of (a string representing a version of the stacktrace dump, and an
integer indicating a line number next to the version string).

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:398: if self.log_lines[ln].startswith('heap
profile: '):
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> I guess too much machinery is involved in getting the version. Are there ways
to
> get an incorrect heap profile? Can you just always include the version?

The first versions didn't include version strings.  As I wrote above, I'll stop
support for old data in future.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:406: sys.stderr.write('  invalid heap profile
dump version:%s\n' % version)
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> Maybe just raise an exception here? You won't need to deal with an incorrect
> version string this way.

Sounds reasonable.  Just raised an exception.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:412: 
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> spare newline

Hmm, this newline is helpful for readability for me...  You feel spare?  I'll
remove it if it's against the style guide.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:469: sizes['mmap-no-log'] = self.counters[
On 2012/04/12 12:40:36, Marc-Antoine Ruel wrote:
> I think this would be much more readable:
> sizes['mmap-no-log'] = (
>     self.counters['%s_committed' % mmap_prefix] - sizes['mmap-total-log'])
> 
> Same below. Splitting inside list element lookup operator "[]" is a weird
place
> to split the line. Use parenthesis () to group the lines as one statement
> instead.

Agreed.  I think it's more readable.  Actaually, I was said to use this style
(splitting inside []) before...

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:484: for key, value in {
On 2012/04/12 12:51:33, Alexander Potapenko wrote:
> This can be generated automatically:
>   map(lambda x: x + '_committed', ['total', 'filemapped'])
> is ['total_committed', 'total_filemapped']

Just some of them doesn't have corresponding names for example, 'filemapped' =>
'file_committed', for historical reason.  I'm keeping them alive for my local
old data, but remove it in future.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:501: 'other-vm': 'other_virtual' }.items():
On 2012/04/12 12:40:36, Marc-Antoine Ruel wrote:
> Use iteritems() instead of items()
> 
> Otherwise, you're copying it

Done.

http://codereview.chromium.org/9812010/diff/15002/tools/deep_memory_profiler/...
tools/deep_memory_profiler/dmprof:506: sizes['mustbezero'] = self.counters[
On 2012/04/12 12:40:36, Marc-Antoine Ruel wrote:
> Use this format instead:
> sizes['mustbezero'] = (
>     self.counters['total_committed'] -
>     self.counters['%s_committed' % mmap_prefix] -
>     self.counters['nonprofiled-absent_committed'] -
> ...)
> 
> But at that point, I'd use a loop:
> removed = ('%s_committed' % mmap_prefix, 'nonprofiled-absent_committed',
> 'nonprofiled-anonymous_committed', ...)
> sizes['mustbezero'] = self.counters['total_committed'] - sum(self.counters[i]
> for i in removed)

Chose a loop.  Thanks!

[commit-bot: I haz the power, 2012-04-13 05:41:43]

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

[commit-bot: I haz the power, 2012-04-13 06:35:24]

Try job failure for 9812010-19013 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-13 06:44:13]

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

[commit-bot: I haz the power, 2012-04-13 09:27:16]

Try job failure for 9812010-19013 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-13 10:19:26]

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

[commit-bot: I haz the power, 2012-04-13 11:44:50]

Change committed as 132174

[M-A Ruel, 2012-04-13 12:22:26]

On 2012/04/13 05:41:10, Dai Mikurube wrote:
> Thanks Alexander, Marc,
> 
> I fixed nits.  Old versions are still alive since I have much old data locally
> and I'm comparing them.  I'm planning to remove old versions at some future
> date.

That's a reason I like to dump data in json format. It's significantly more
future proof, doesn't require a definition like protobuf and much more dense
than xml.

[Dai Mikurube (NOT FULLTIME), 2012-04-13 13:15:20]

On 2012/04/13 12:22:26, Marc-Antoine Ruel wrote:
> On 2012/04/13 05:41:10, Dai Mikurube wrote:
> > Thanks Alexander, Marc,
> > 
> > I fixed nits.  Old versions are still alive since I have much old data
locally
> > and I'm comparing them.  I'm planning to remove old versions at some future
> > date.
> 
> That's a reason I like to dump data in json format. It's significantly more
> future proof, doesn't require a definition like protobuf and much more dense
> than xml.

I like JSON, too, but it's hard to use JSON here since it's very low-level dump
data from tcmalloc.  The current tcmalloc dumper interface requires lots of
memory for dumping.  We have to pack more information in a smaller buffer.

[M-A Ruel, 2012-04-13 13:18:07]

On 2012/04/13 13:15:20, Dai Mikurube wrote:
> On 2012/04/13 12:22:26, Marc-Antoine Ruel wrote:
> > On 2012/04/13 05:41:10, Dai Mikurube wrote:
> > > Thanks Alexander, Marc,
> > > 
> > > I fixed nits.  Old versions are still alive since I have much old data
> locally
> > > and I'm comparing them.  I'm planning to remove old versions at some
future
> > > date.
> > 
> > That's a reason I like to dump data in json format. It's significantly more
> > future proof, doesn't require a definition like protobuf and much more dense
> > than xml.
> 
> I like JSON, too, but it's hard to use JSON here since it's very low-level
dump
> data from tcmalloc.  The current tcmalloc dumper interface requires lots of
> memory for dumping.  We have to pack more information in a smaller buffer.

You are right, json isn't memory efficient at all. :/
FTR, I've had relative success with csv, it works well with incremental append,
it's memory efficient and its parser isn't too bad.