Implemented an averaging object histogram in the vm.

Implemented an averaging object histogram in the vm.
If you use the flag --print-object-histogram the histogram
will be printed when the isolate shuts down.

[ahe, 2013-06-12 11:24:01]

DBC

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.cc:56: // resize the table.
Make proper sentence: start with uppercase letter.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.cc:101: qsort(array, length, sizeof(Element*),
reinterpret_cast<CmpFunc>(compare)); // NOLINT
Long line.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:19: // is shut down. Information is gathered at the
backedge of each major GC event.
backedge -> back-edge or back edge.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:27: // Called when a new class is registered in the
isoalte.
isoalte -> isolate.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:30: // Collect sample for the histogram. Called at
backedge of major GC.
backedge -> back-edge or back edge.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:372: 
Add extra line.

[bakster, 2013-06-12 11:34:21]

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.cc:56: // resize the table.
On 2013/06/12 11:24:01, ahe wrote:
> Make proper sentence: start with uppercase letter.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.cc:101: qsort(array, length, sizeof(Element*),
reinterpret_cast<CmpFunc>(compare)); // NOLINT
On 2013/06/12 11:24:01, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.cc:101: qsort(array, length, sizeof(Element*),
reinterpret_cast<CmpFunc>(compare)); // NOLINT
On 2013/06/12 11:24:01, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:19: // is shut down. Information is gathered at the
backedge of each major GC event.
On 2013/06/12 11:24:01, ahe wrote:
> backedge -> back-edge or back edge.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:27: // Called when a new class is registered in the
isoalte.
On 2013/06/12 11:24:01, ahe wrote:
> isoalte -> isolate.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/heap_histogram....
runtime/vm/heap_histogram.h:30: // Collect sample for the histogram. Called at
backedge of major GC.
On 2013/06/12 11:24:01, ahe wrote:
> backedge -> back-edge or back edge.

Done.

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/16077018/diff/9001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:372: 
On 2013/06/12 11:24:01, ahe wrote:
> Add extra line.

Done.

[Ivan Posva, 2013-06-12 15:18:09]

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:31: ObjectHistogramVisitor
object_visitor(Isolate::Current());
Why not use isolate_ instead of Isolate::Current() here?

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:85: HandleScope handle_scope(isolate_);
Why is there a HandleScope here?

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:98: if (table_[index].count > 0) array[pos++] =
&table_[index];
You do not strictly need to have the class id in the elements, as the address of
the element in the sorted list encodes the class id when scaled correctly.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:117: if (e->class_id < kNumPredefinedCids) {
To print VM names in back quotes this should really be kInstanceCid.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:41: struct Element {
class Element : public ValueObject {

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:42: void Add(int s) {
intptr_t size

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:47: int count;
intptr_t for class_id_, count_ and size_.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:55: int table_length_;
intptr_t for these int fields.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:413: if (FLAG_print_object_histogram && !is_vm_isolate) {
if (FLAG_print_object_histogram &&  (Dart::vm_isolate() != NULL)) {

avoids the true/false parameter. We use this kind of check in other places
already.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:746: StackZone stack_zone(this);
Please move the HandleScope from within Print here.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:232: static Isolate* Init(const char* name_prefix, bool
is_vm_isolate = false);
Why was this needed?

[bakster, 2013-06-12 15:42:42]

//Lars

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:31: ObjectHistogramVisitor
object_visitor(Isolate::Current());
On 2013/06/12 15:18:09, Ivan Posva wrote:
> Why not use isolate_ instead of Isolate::Current() here?

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:85: HandleScope handle_scope(isolate_);
On 2013/06/12 15:18:09, Ivan Posva wrote:
> Why is there a HandleScope here?
Moved as suggested.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:98: if (table_[index].count > 0) array[pos++] =
&table_[index];
On 2013/06/12 15:18:09, Ivan Posva wrote:
> You do not strictly need to have the class id in the elements, as the address
of
> the element in the sorted list encodes the class id when scaled correctly.

Correct, but that code is error prone in my mind.
Keeping class_id_ is the better solution.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:117: if (e->class_id < kNumPredefinedCids) {
On 2013/06/12 15:18:09, Ivan Posva wrote:
> To print VM names in back quotes this should really be kInstanceCid.

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:41: struct Element {
On 2013/06/12 15:18:09, Ivan Posva wrote:
> class Element : public ValueObject {

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:42: void Add(int s) {
On 2013/06/12 15:18:09, Ivan Posva wrote:
> intptr_t size

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:47: int count;
On 2013/06/12 15:18:09, Ivan Posva wrote:
> intptr_t for class_id_, count_ and size_.

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:55: int table_length_;
On 2013/06/12 15:18:09, Ivan Posva wrote:
> intptr_t for these int fields.

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:413: if (FLAG_print_object_histogram && !is_vm_isolate) {
Seems flaky to me but, I'll change it.

On 2013/06/12 15:18:09, Ivan Posva wrote:
> if (FLAG_print_object_histogram &&  (Dart::vm_isolate() != NULL)) {
> 
> avoids the true/false parameter. We use this kind of check in other places
> already.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:746: StackZone stack_zone(this);
On 2013/06/12 15:18:09, Ivan Posva wrote:
> Please move the HandleScope from within Print here.

Done.

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/16077018/diff/15001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:232: static Isolate* Init(const char* name_prefix, bool
is_vm_isolate = false);
On 2013/06/12 15:18:09, Ivan Posva wrote:
> Why was this needed?

Done.

[Ivan Posva, 2013-06-13 06:43:49]

LGTM -ip