Implement a heap profiler for the Dart managed heap.

Implement a heap profiler for the Dart managed heap.

This profiler will write binary HPROF data to a user provided stream that
can later be viewed by tools such as the Eclipse Memory Analyzer.

While all user defined types are accurately described in the profile data,
internal types that are not self-describing appear as empty objects.  This
will be addressed by a future change.

Committed: https://code.google.com/p/dart/source/detail?r=8636

[turnidge, 2012-05-29 17:50:01]

I would like to see a basic sanity test for the heap profiler, if possible. 
Something that just makes sure that the heap profiler's output isn't horribly
broken.

http://codereview.chromium.org/10452006/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:103: typedef void
(*Dart_HeapProfileWriteCallback)(const void* data,
Maybe add short comment above this.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:618: CHECK_ISOLATE(isolate);
I would move the CHECK_ISOLATE above your callback == NULL check -- the call to
Api::NewError will fail if there is no current isolate.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/freelist.h#newcode45
runtime/vm/freelist.h:45: static bool IsSpecialClass(const RawObject* raw_obj) {
We don't seem to use "const RawObject*" much in our code:

random:runtime turnidge$ find . -type f | grep h$ | xargs grep "const
RawObject*" | wc -l
       0
random:runtime turnidge$ find . -type f | grep h$ | xargs grep "RawObject*" | wc
-l
     274

That said, I don't see any harm...

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap.h
File runtime/vm/heap.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap.h#newcode117
runtime/vm/heap.h:117: // Generates a profile of the current and VM isolate
heaps.
Out of curiosity -- by including the VM isolate heap in all of our per-isolate
heap profiles, is there some chance tools will get a bloated sense of the
per-isolate space requirements?  Maybe this doesn't matter.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc
File runtime/vm/heap_profiler.cc (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:144: smi_table_.insert(raw_smi);
Add comment?

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:155: class_table_.insert(raw_class);
Add comment?

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:165: string_table_.insert(ptr);
Add comment?

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:189: if (FreeListElement::IsSpecialClass(raw_obj)) {
What does this mean?  Add comment?

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.h
File runtime/vm/heap_profiler.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.h#new...
runtime/vm/heap_profiler.h:17: 
Maybe add a top-level comment describing what the HeapProfiler does.  You might
want to give a reference to the file format description.

[cshapiro, 2012-06-01 04:27:02]

I will think about how to test this code and re-upload.

http://codereview.chromium.org/10452006/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:103: typedef void
(*Dart_HeapProfileWriteCallback)(const void* data,
Certainly.  Done.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:618: CHECK_ISOLATE(isolate);
I should have noticed that.  Done.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/freelist.h#newcode45
runtime/vm/freelist.h:45: static bool IsSpecialClass(const RawObject* raw_obj) {
As an aside, our only use of const_cast outside of test code is to cast away
const from a RawObject pointer.  I would like to stomp that out in my next
change.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap.h
File runtime/vm/heap.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap.h#newcode117
runtime/vm/heap.h:117: // Generates a profile of the current and VM isolate
heaps.
Based on experience, the most common use of this data will be to examine a heap
post-OOM to understand what retained storage.  In that situation it is usually
okay for the vm isolate data to be included as it will be dwarfed by the
application heap.  

I think having a snapshot of just the current isolate would be a good option to
have.  Once the profiler becomes more complete I would like to add a flag to
control whether the VM isolate contents are emitted.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc
File runtime/vm/heap_profiler.cc (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:144: smi_table_.insert(raw_smi);
On 2012/05/29 17:50:01, turnidge wrote:
> Add comment?

Done.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:155: class_table_.insert(raw_class);
On 2012/05/29 17:50:01, turnidge wrote:
> Add comment?

Done.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:165: string_table_.insert(ptr);
On 2012/05/29 17:50:01, turnidge wrote:
> Add comment?

Done.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.cc#ne...
runtime/vm/heap_profiler.cc:189: if (FreeListElement::IsSpecialClass(raw_obj)) {
Good question.  It means the raw_obj is actually a free-list element in
old-space and not an object.  I have added a comment.

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.h
File runtime/vm/heap_profiler.h (right):

http://codereview.chromium.org/10452006/diff/1/runtime/vm/heap_profiler.h#new...
runtime/vm/heap_profiler.h:17: 
A great idea.  Done.

[cshapiro, 2012-06-02 00:18:56]

PTAL

[turnidge, 2012-06-12 17:02:19]

LGTM.

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
File runtime/vm/heap_profiler.cc (right):

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
runtime/vm/heap_profiler.cc:27: size_ += size;
It's a bit confusing to me to have both "name_" and "name" in play at the same
time in a non-trivial way.  If you can think of a convenient rename for the
member or the parameter, that would help folks like me.  Your call.

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
runtime/vm/heap_profiler.cc:520: sub.Write32(sizeof(RawClass));
What is the instance size supposed to mean for a class?

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/raw_obje...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:1264: friend class HeapProfiler;
I'm wondering if it makes sense to add this friend decl to one of the macros. 
What do you think?

[cshapiro, 2012-06-14 00:08:36]

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
File runtime/vm/heap_profiler.cc (right):

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
runtime/vm/heap_profiler.cc:27: size_ += size;
I see where you are coming from.  I would like this to be as readable as
possible.  Can you suggest new names for data and size?

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/heap_pro...
runtime/vm/heap_profiler.cc:520: sub.Write32(sizeof(RawClass));
It is the size of instances, not the size of the class object we are dumping.  I
have adjusted the code and added a comment about this.  At present, we do not
handle variable sized objects well and many internal classes have an
indeterminate length.

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/raw_obje...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10452006/diff/8001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:1264: friend class HeapProfiler;
I think that is a good idea.  I have pulled this up into a HEAP_PROFILER_SUPPORT
macro.  It cleans up the diff nicely.