Add support for dump allocations created in a certain time window

Add support for dump allocations created in a certain time window

This allows for debugging e.g. objects that are not deleted during a page reload. I also added a tool for parsing and symbolizing the traces.

BUG=none
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141404

[jochen (gone - plz use gerrit), 2012-06-06 12:55:17]

Jim, can you review the tcmalloc bits
Marja, can you have a look at the python script (feel free to comment on the
tcmalloc stuff as well)

Dai, mainly fyi

[marja, 2012-06-06 13:11:06]

python part lgtm

https://chromiumcodereview.appspot.com/10541026/diff/1/tools/tcmalloc/print-l...
File tools/tcmalloc/print-live-objects.py (right):

https://chromiumcodereview.appspot.com/10541026/diff/1/tools/tcmalloc/print-l...
tools/tcmalloc/print-live-objects.py:26: line_no = 0;
nit: no ; says the style guide

https://chromiumcodereview.appspot.com/10541026/diff/1/tools/tcmalloc/print-l...
tools/tcmalloc/print-live-objects.py:45: addresses.add(frame);
here too

[jochen (gone - plz use gerrit), 2012-06-06 13:12:06]

comments addresses (also fixed the compile error)

[jar (doing other things), 2012-06-06 22:13:46]

I'd like to add dmikurube to the reviewer list, to be sure these patches are
fitting in with what he's been doing.

Some initial comments below.

https://chromiumcodereview.appspot.com/10541026/diff/6002/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

https://chromiumcodereview.appspot.com/10541026/diff/6002/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profile-table.h:220: //   3. Invoke
MarkAllAsLive. New allocations are now marked as "live".
Given this description, is this name right?  I had to re-read this a few times
to make sense of it, and decided the names adding to my confusion.

Should the call on line 220 be something incredibly verbose like:
MarkAllThatAreNotIgnoredAsNewAllocations

When we start going that route... the word "Ignored" seems to be a questionable
choice in line 216, and I of course hate how long these names are getting.

Perhaps the names should be:
MarkAllAllocations  (rather than "ignored")

And then the Live marking may be more like:

MarkAllNewAllocations

Perhaps each call should take a "mark" and the first call should set the mark
everywhere, and the second should only set a new mark on things that are not yet
marked.

That seems much more general, and allows for multiple marks.


...or... if data density is key (you just want one bit), maybe you need:
MarkAll

to set the marks to true (and assume that new items appear as unmarked (false). 
Then call:

ToggleMark

to isolate new stuff.... and:

DumpMarkedObjects

to dump them.

Bottom line: MarkAllAsLive is really confusing when it not All, but only Some.

[Dai Mikurube (NOT FULLTIME), 2012-06-07 05:48:16]

It looks no conflicts with my work, and it basically looks good to me.

But I'm worried that it's changing meanings and semantics of 'live' and
'ignored' flags.

Who calls the three global functions in heap-profiler.cc?
How are they used?
Does calling them break existing heap profiling?

[jochen (gone - plz use gerrit), 2012-06-07 06:00:43]

The idea is to call those methods from a debugger.

Calling them doesn't interfere with the heap profiler, but you can't use it
together with tcmalloc's leak checker.
On Jun 7, 2012 7:48 AM, <dmikurube@chromium.org> wrote:

> It looks no conflicts with my work, and it basically looks good to me.
>
> But I'm worried that it's changing meanings and semantics of 'live' and
> 'ignored' flags.
>
> Who calls the three global functions in heap-profiler.cc?
> How are they used?
> Does calling them break existing heap profiling?
>
>
http://codereview.chromium.**org/10541026/<http://codereview.chromium.org/105...
>

[Dai Mikurube (NOT FULLTIME), 2012-06-07 06:12:28]

I see.  'live' and 'ignored' are used in heap-checker*, but not used in
heap-profile*.

I think we should add comments like "Don't use these functions with the
heap-checker." at least.  Some flags to check or avoid simultaneous use of them
are better.

On 2012/06/07 06:00:43, jochen wrote:
> The idea is to call those methods from a debugger.
> 
> Calling them doesn't interfere with the heap profiler, but you can't use it
> together with tcmalloc's leak checker.
> On Jun 7, 2012 7:48 AM, <mailto:dmikurube@chromium.org> wrote:
> 
> > It looks no conflicts with my work, and it basically looks good to me.
> >
> > But I'm worried that it's changing meanings and semantics of 'live' and
> > 'ignored' flags.
> >
> > Who calls the three global functions in heap-profiler.cc?
> > How are they used?
> > Does calling them break existing heap profiling?
> >
> >
>
http://codereview.chromium.**org/10541026/%3Chttp://codereview.chromium.org/1...>
> >

[jochen (gone - plz use gerrit), 2012-06-08 11:16:14]

I renamed the methods as suggested by Jim (although I picked different names
than he suggested)

I also introduced an explicit concept of marks instead of abusing live/ignored

I added a comment saying that you shouldn't use those methods on the
HeapLeakChecker's heap profile table.

Note that I'm invoking the methods from the HeapProfiler's heap-profile-table,
so which doesn't use the marks, so there shouldn't be any problem

[jochen (gone - plz use gerrit), 2012-06-09 18:45:15]

friendly ping

[Dai Mikurube (NOT FULLTIME), 2012-06-10 03:07:50]

On 2012/06/09 18:45:15, jochen wrote:
> friendly ping

almost looks good.  I'll look at it in details tomorrow morning.  (Sorry, I was
in flight.)

[Dai Mikurube (NOT FULLTIME), 2012-06-10 11:52:19]

On 2012/06/08 11:16:14, jochen wrote:
> I renamed the methods as suggested by Jim (although I picked different names
> than he suggested)
> 
> I also introduced an explicit concept of marks instead of abusing live/ignored
> 
> I added a comment saying that you shouldn't use those methods on the
> HeapLeakChecker's heap profile table.
> 
> Note that I'm invoking the methods from the HeapProfiler's heap-profile-table,
> so which doesn't use the marks, so there shouldn't be any problem

Got it.  Both heap-profiler and heap-(leak-)checker have their own
heap-profile-table instance.  'live' and 'ignore' are previously not used in
heap-profiler.  right?  (I haven't touched heap-checker so far.)

lgtm if jar@ say ok.

[jochen (gone - plz use gerrit), 2012-06-10 11:57:13]

Right, those bits are only used by the leak checker
On Jun 10, 2012 1:52 PM, <dmikurube@chromium.org> wrote:

> On 2012/06/08 11:16:14, jochen wrote:
>
>> I renamed the methods as suggested by Jim (although I picked different
>> names
>> than he suggested)
>>
>
>  I also introduced an explicit concept of marks instead of abusing
>> live/ignored
>>
>
>  I added a comment saying that you shouldn't use those methods on the
>> HeapLeakChecker's heap profile table.
>>
>
>  Note that I'm invoking the methods from the HeapProfiler's
>> heap-profile-table,
>> so which doesn't use the marks, so there shouldn't be any problem
>>
>
> Got it.  Both heap-profiler and heap-(leak-)checker have their own
> heap-profile-table instance.  'live' and 'ignore' are previously not used
> in
> heap-profiler.  right?  (I haven't touched heap-checker so far.)
>
> lgtm if jar@ say ok.
>
>
http://codereview.chromium.**org/10541026/<http://codereview.chromium.org/105...
>

[jar (doing other things), 2012-06-11 08:53:14]

See nits below (and apply if we can use our style).

then LGTM

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profile-table.h:313: : fd(a), mark(m) { }
nit: one initializer per line if you wrap... with the colon indented 4 spaces.

In this case, you don't need to wrap... and could put it all on the previous
line.

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profile-table.h:323: : mark(m),
mark_all(a) { }
nit: again... one init per line ... with ":" indented 4 spaces...

...but again... you didn't need to wrap at all.

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profiler.cc:205: static bool  is_on =
false;           // If are on as a subsytem.
nit: I couldn't make sense of the variable name, or the comment.

If this were to comply with Chromimum coding, then globals should have g_ as a
prefix.  I'm not sure if this is Chromium only code, or code based on some other
google distribution.

For this variable, it would be helpful to say something like
heap_profiler_active (unless this is google code... and we need to avoid
forking).

The current name has a pronoun, which is meaningless in a global context (though
it might mean something in a class), and especially confusing when I couldn't
even guess that this related to a global static :-/.

[jochen (gone - plz use gerrit), 2012-06-11 13:13:47]

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profile-table.h:313: : fd(a), mark(m) { }
On 2012/06/11 08:53:14, jar wrote:
> nit: one initializer per line if you wrap... with the colon indented 4 spaces.
> 
> In this case, you don't need to wrap... and could put it all on the previous
> line.

Done.

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profile-table.h:323: : mark(m),
mark_all(a) { }
On 2012/06/11 08:53:14, jar wrote:
> nit: again... one init per line ... with ":" indented 4 spaces...
> 
> ...but again... you didn't need to wrap at all.

Done.

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10541026/diff/9003/third_party/tcmallo...
third_party/tcmalloc/chromium/src/heap-profiler.cc:205: static bool  is_on =
false;           // If are on as a subsytem.
I agree that the variable naming is not optimal, but to keep the diff to
upstream as small as possible, I think it's better not to change it