Track live instance and allocation counts for classes

Track live instance and allocation counts for classes.

R=iposva@google.com

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

[srdjan, 2013-10-30 20:50:12]

DBC
DId you notice any performance impact?

https://codereview.chromium.org/51653006/diff/30001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/30001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2786: summary->set_temp(0,
Location::RequiresRegister());
Ia32 does not have many registers available, and the performance can quickly
deteriorate if we run out of them.  Alternative would be to push/pop a register
in the allocation code.

[Cutch, 2013-10-31 03:19:46]

Some benchmarks (Against Patch Set 3):

Without:
NavierStokes(RunTime): 4856.796116504855 us.
Splay(RunTime): 1992.0713577799802 us.
Tracer(RunTime): 823.7232289950576 us.
Richards(RunTime): 900.0900090009001 us.
DeltaBlue(RunTime): 988.6307464162136 us.
StringInterpolation(RunTime): 1706500.0 us.
StringBuffer(RunTime): 13304.635761589405 us.
StringIdiomatic(RunTime): 13264.900662251655 us.
BuildTableInterpolation(RunTime): 32.001024032769045 us.
BuildTableBuffer(RunTime): 21.978988087388455 us.
BuildTableIdiomatic(RunTime): 21.955101816784676 us.

With: 
NavierStokes(RunTime): 4821.686746987952 us.
Splay(RunTime): 2237.513873473918 us.
Tracer(RunTime): 913.2420091324201 us.
Richards(RunTime): 881.4455707360071 us.
DeltaBlue(RunTime): 978.4735812133073 us.
StringInterpolation(RunTime): 1698500.0 us.
StringBuffer(RunTime): 13223.684210526315 us.
StringIdiomatic(RunTime): 13203.947368421053 us.
BuildTableInterpolation(RunTime): 30.923850019327407 us.
BuildTableBuffer(RunTime): 21.591976421561746 us.
BuildTableIdiomatic(RunTime): 21.580560231343604 us.

Let me know if you want me to run a specific benchmark.

[Florian Schneider, 2013-10-31 09:56:00]

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:2300: pushl(instance_reg);
I suspect that this will make bump allocation a _lot_ slower:
Before: 2 loads, 2 stores.
After:  4 loads, 4 stores.
How will this cost be amortized?

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2889: summary->set_temp(0,
Location::RequiresRegister());
Where and why is temp(0) used?

[Cutch, 2013-10-31 11:20:02]

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:2300: pushl(instance_reg);
On 2013/10/31 09:56:01, Florian Schneider wrote:
> I suspect that this will make bump allocation a _lot_ slower:
> Before: 2 loads, 2 stores.
> After:  4 loads, 4 stores.
> How will this cost be amortized?

I will run some tests tomorrow against patch set 2 (which uses temp registers).
In some places, like intrinsics, we have registers to use.

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/80001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2889: summary->set_temp(0,
Location::RequiresRegister());
On 2013/10/31 09:56:01, Florian Schneider wrote:
> Where and why is temp(0) used?

This is left over from PS2

[Cutch, 2013-11-12 17:19:34]

CL extended to track (per class id):

*) Allocation count and size in new space (since last major GC).
*) Allocation count and size in old space (since last major GC).
*) Live count and size in new space (updated after major GC).
*) Live count and size in old space (updated after major GC).

PTAL.

[Cutch, 2013-11-12 23:29:38]

Patch Set 17 is ready for review.

[Florian Schneider, 2013-11-13 18:06:53]

Two questions:

1. What do you think about adding a flag to control counting?

2. Where are allocations from C++ land accounted for (e.g. String::New,
Array::New, etc.)

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/class_table.h...
runtime/vm/class_table.h:84: void AllocateClassNewSpace(intptr_t cid, intptr_t
size);
Maybe ReportAllocation would be a better name?

You could also think about making this one function for new/old/code space:

ReportAllocation(intptr_t cid, intptr_t size, Heap::Space space);

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:542: void
StubCode::GenerateAllocateArrayStub(Assembler* assembler) {
Also count allocation for arrays?

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:898: void
StubCode::GenerateAllocateContextStub(Assembler* assembler) {
Also count allocations of contexts?

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1267: void
StubCode::GenerateAllocationStubForClosure(Assembler* assembler,
Also count allocation for closures?

[Cutch, 2013-11-13 19:08:17]

On 2013/11/13 18:06:53, Florian Schneider wrote:
> Two questions:
> 
> 1. What do you think about adding a flag to control counting?
> 

I'm fine with a flag that is enabled by default.

> 2. Where are allocations from C++ land accounted for (e.g. String::New,
> Array::New, etc.)
> 

Moved the reporting from the runtime entry to Object::Allocate which should
catch both Dart and C++ allocations.

[Cutch, 2013-11-13 19:09:33]

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/class_table.h...
runtime/vm/class_table.h:84: void AllocateClassNewSpace(intptr_t cid, intptr_t
size);
On 2013/11/13 18:06:53, Florian Schneider wrote:
> Maybe ReportAllocation would be a better name?
> 
> You could also think about making this one function for new/old/code space:
> 
> ReportAllocation(intptr_t cid, intptr_t size, Heap::Space space);

Done.

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:542: void
StubCode::GenerateAllocateArrayStub(Assembler* assembler) {
On 2013/11/13 18:06:53, Florian Schneider wrote:
> Also count allocation for arrays?

Done.

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:898: void
StubCode::GenerateAllocateContextStub(Assembler* assembler) {
On 2013/11/13 18:06:53, Florian Schneider wrote:
> Also count allocations of contexts?

Done.

https://codereview.chromium.org/51653006/diff/540001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1267: void
StubCode::GenerateAllocationStubForClosure(Assembler* assembler,
On 2013/11/13 18:06:53, Florian Schneider wrote:
> Also count allocation for closures?

Done.

[Ivan Posva, 2013-12-12 13:53:23]

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2644: void
Assembler::BumpAllocationCount(Heap::Space space,
UpdateAllocationCount or UpdateAllocationStats.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2659: const Address& count_address =
Address(temp_reg, count_field_offset);
Please move the Address calculation after the LoadImmediate. LoadImmediate is
setting up temp_reg, but then it is unclear where it is being used when trying
to read through the code.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2701: LoadImmediate(temp_reg,
class_heap_stats_table_address + class_offset);
ditto

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:36: predefined_class_heap_stats_table_ =
reinterpret_cast<ClassHeapStats*>(
Why is this allocated twice?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:165: if (CollectInstanceSizesForClass(cid)) {
How about avoiding the check and just updating the size when coming from C++
code? It should be ignored in the reporting code.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:176: if (CollectInstanceSizesForClass(cid)) {
ditto

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:182: //  #define DEBUG_PRINT
Please make this a command line flag or remove the debugging code.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:196: void ClassTable::Collect() {
How about only exposing ResetCounters()?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:233: ClassHeapStats*
ClassTable::FindClassHeapStats(intptr_t cid) {
StatsAt(intptr_t cid)

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.h...
runtime/vm/class_table.h:79: static intptr_t table_offset() {
Why are we using the offset of the field here and the address of the field
below?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:112: // Report allocate to heap class stats.
?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:213:
class_table_->ReportLiveNewSpace(raw_obj->GetClassId(), raw_obj->Size());
This is not correct since the marker does not trace through new objects,
suddenly the new gen live stats will only report the count/size of objects
reached from old gen, but nothing within.

The correct thing here is to not update the new stats as part of marking.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:216:
class_table_->ReportLiveOldSpace(raw_obj->GetClassId(), raw_obj->Size());
ReportLiveOldSpace -> UpdateLiveOld

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:176: UpdateClassStats();
Why isn't this being done as part of new collection? What does it really mean to
update class stats before GC?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/intrinsifier_...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/intrinsifier_...
runtime/vm/intrinsifier_ia32.cc:1185: EBX);
When do you need to pass a temp register on ia32? Here you do, above you don't.

The way I understand it: predefined cid, you do not need to, otherwise you do,
correct?

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1450:
isolate->class_table()->ReportAllocationNewSpace(cls_id, size);
This separation should probably be done in the class table.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:591: // EDI: Size of allocation in bytes.
At this point EDI does not contain the size of the allocation in bytes. The
comments are supposed to explain what is in the registers at that particular
point. It is also supposed to convey "register allocation" high-lighting which
registers hold values which will be needed later.

So in this case the comment should be:
// EAX: new object start as tagged pointer
// EBX: next object start (new top)
// ECX: array element type
// EDX: array lenght as Smi

This shows that the length in bytes calculation is broken in this case. It needs
to be moved before the tagging of the new object.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:937: // EBX: Size of allocation in bytes.
ditto

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1314: // EDX: Clobbered.
This should not be necessary if the list of live registers is correct in the
comments.

[srdjan, 2013-12-12 15:52:31]

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2656: const uword count_field_offset = space ==
Heap::kNew ?
I find it more readable to put parentheses around (space == Heap::kNew)  ...
also below.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.h:739: Register temp_reg);
I find method overloading leads easily to typos, and makes the code harder to
read as it is not cleat which method is being called ... so I would prefer
different names instead.
Also you may consider using similar interface as for allocation: pass 'space' as
last argument and use kNew as default.

[Cutch, 2013-12-13 01:31:09]

Updated CL addressing all comments from Srdjan and Ivan except for one from
Ivan:

Why isn't this being done as part of new collection? What does it really mean
to update class stats before GC?

Agreed. Let's discuss this Friday morning.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2644: void
Assembler::BumpAllocationCount(Heap::Space space,
On 2013/12/12 13:53:24, Ivan Posva wrote:
> UpdateAllocationCount or UpdateAllocationStats.

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2656: const uword count_field_offset = space ==
Heap::kNew ?
On 2013/12/12 15:52:32, srdjan wrote:
> I find it more readable to put parentheses around (space == Heap::kNew)  ...
> also below.

Done in all 16 places.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2659: const Address& count_address =
Address(temp_reg, count_field_offset);
On 2013/12/12 13:53:24, Ivan Posva wrote:
> Please move the Address calculation after the LoadImmediate. LoadImmediate is
> setting up temp_reg, but then it is unclear where it is being used when trying
> to read through the code.

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2701: LoadImmediate(temp_reg,
class_heap_stats_table_address + class_offset);
On 2013/12/12 13:53:24, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.h:739: Register temp_reg);
On 2013/12/12 15:52:32, srdjan wrote:
> I find method overloading leads easily to typos, and makes the code harder to
> read as it is not cleat which method is being called ... so I would prefer
> different names instead.
> Also you may consider using similar interface as for allocation: pass 'space'
as
> last argument and use kNew as default. 

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:36: predefined_class_heap_stats_table_ =
reinterpret_cast<ClassHeapStats*>(
On 2013/12/12 13:53:24, Ivan Posva wrote:
> Why is this allocated twice?

The predefined class stats table is allocated once at startup. It's address is
constant so that we can bake the address into the code.

The class stats table is allocated once at startup but is realloced as we add
classes. The address moves around.

I can kill the overlap if you would like.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:165: if (CollectInstanceSizesForClass(cid)) {
On 2013/12/12 13:53:24, Ivan Posva wrote:
> How about avoiding the check and just updating the size when coming from C++
> code? It should be ignored in the reporting code.

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:176: if (CollectInstanceSizesForClass(cid)) {
On 2013/12/12 13:53:24, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:182: //  #define DEBUG_PRINT
On 2013/12/12 13:53:24, Ivan Posva wrote:
> Please make this a command line flag or remove the debugging code.

This will be removed before committing. It's just for testing.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:196: void ClassTable::Collect() {
On 2013/12/12 13:53:24, Ivan Posva wrote:
> How about only exposing ResetCounters()?

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:233: ClassHeapStats*
ClassTable::FindClassHeapStats(intptr_t cid) {
On 2013/12/12 13:53:24, Ivan Posva wrote:
> StatsAt(intptr_t cid)

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/class_table.h...
runtime/vm/class_table.h:79: static intptr_t table_offset() {
On 2013/12/12 13:53:24, Ivan Posva wrote:
> Why are we using the offset of the field here and the address of the field
> below?

This function table_offset() is used by the assembler in LoadClass and
LoadClassById. I'm not using it.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:112: // Report allocate to heap class stats.
On 2013/12/12 13:53:24, Ivan Posva wrote:
> ?

Dead code with a zombie comment.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:213:
class_table_->ReportLiveNewSpace(raw_obj->GetClassId(), raw_obj->Size());
On 2013/12/12 13:53:24, Ivan Posva wrote:
> This is not correct since the marker does not trace through new objects,
> suddenly the new gen live stats will only report the count/size of objects
> reached from old gen, but nothing within.
> 
> The correct thing here is to not update the new stats as part of marking.

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:216:
class_table_->ReportLiveOldSpace(raw_obj->GetClassId(), raw_obj->Size());
On 2013/12/12 13:53:24, Ivan Posva wrote:
> ReportLiveOldSpace -> UpdateLiveOld

Done.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:176: UpdateClassStats();
On 2013/12/12 13:53:24, Ivan Posva wrote:
> Why isn't this being done as part of new collection? What does it really mean
to
> update class stats before GC?

Agreed. Let's discuss this Friday morning.

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51653006/diff/680001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1450:
isolate->class_table()->ReportAllocationNewSpace(cls_id, size);
On 2013/12/12 13:53:24, Ivan Posva wrote:
> This separation should probably be done in the class table.

Agreed, but, #including heap.h into class_table.h ends up with a #include cycle
involving growable_array.h that looks painful to resolve.

[Ivan Posva, 2013-12-13 23:13:17]

-Ivan

https://codereview.chromium.org/51653006/diff/700001/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/51653006/diff/700001/runtime/vm/class_table.h...
runtime/vm/class_table.h:28: intptr_t new_count_since_gc_new_space;
new_count_since_gc -> allocated

https://codereview.chromium.org/51653006/diff/700001/runtime/vm/class_table.h...
runtime/vm/class_table.h:85: void ReportAllocationNew(intptr_t cid, intptr_t
size);
UpdateAllocatedNew which would be in sync with UpdateLiveOld.

[Cutch, 2013-12-23 01:29:48]

Patch Set 22:

- Renamed variables and methods as requested.
- Integration with scavenger promotion (reported as allocation in old space) and
copy (reported as post-GC live instance in new space).
- New / Old GC counter updates separated.
- Unit test.

[Ivan Posva, 2014-01-14 22:54:22]

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:36: predefined_class_heap_stats_table_ =
reinterpret_cast<ClassHeapStats*>(
Duplicate allocation here.

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1325: __ UpdateAllocationStatsWithSize(cls.id(),
EBX, EDX);
We are potentially allocating two objects here in the case of implicit instance
closures.

[Cutch, 2014-01-15 19:49:48]

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/class_table.c...
runtime/vm/class_table.cc:36: predefined_class_heap_stats_table_ =
reinterpret_cast<ClassHeapStats*>(
On 2014/01/14 22:54:23, Ivan Posva wrote:
> Duplicate allocation here.

Done.

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/720001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1325: __ UpdateAllocationStatsWithSize(cls.id(),
EBX, EDX);
On 2014/01/14 22:54:23, Ivan Posva wrote:
> We are potentially allocating two objects here in the case of implicit
instance
> closures.

Done here and x64,arm, and mips.

[Ivan Posva, 2014-01-17 06:53:15]

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:237: //  #define DEBUG_PRINT
Debugging code should go.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:251: bool
ClassTable::CollectInstanceSizesForClass(intptr_t cid) {
We have other lists of class ids in raw_object, for example
RawObject::IsStringClassId. Maybe we should move this in there as well?
Something like:

bool RawObject::IsVariableSizeClassId(intptr_t index);

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:272: (cid == kSmiCid)) {
SmiCid is definitely not variable in size.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:317: JSONObject heaps(&obj, "heap");
The heap should print itself here at least as a ref.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:230: class_table_->UpdateLiveOld(raw_obj->GetClassId(),
raw_obj->Size());
We only need the size for variable size objects. I am wondering whether we
should avoid getting the size for the others.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:354: double seconds = static_cast<double>(micros) /
RoundMicrosecondsToSeconds

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:170: double GCTimeInSeconds(Space space) const;
Why do you need both? Micros and Seconds?

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/object.cc#new...
runtime/vm/object.cc:1546: return raw_ptr()->user_name_;
Why is this allocated eagerly? Is there a reason why we do not lazily create and
cache this on first access?

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1055: { "heap", HandleHeap },
Why twice?

Also "heap"/HandleHeap is a bit of a misnomer. Shouldn't it be allocation stats,
live stats or similar?

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1330: // EDX: Will be clobbered.
You might want to comment in this code what the two update allocation stats
update. The upper one is a bit clearer as it uses the kContextCid as a constant.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/symbols.h#new...
runtime/vm/symbols.h:303: V(StringSym, "String")                                
                      \
Can you move this together with the remaining String symbols?

Also either just String or if that does not work _String.

[Cutch, 2014-01-17 18:37:58]

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:237: //  #define DEBUG_PRINT
On 2014/01/17 06:53:16, Ivan Posva wrote:
> Debugging code should go.

Done.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/class_table.c...
runtime/vm/class_table.cc:251: bool
ClassTable::CollectInstanceSizesForClass(intptr_t cid) {
On 2014/01/17 06:53:16, Ivan Posva wrote:
> We have other lists of class ids in raw_object, for example
> RawObject::IsStringClassId. Maybe we should move this in there as well?
> Something like:
> 
> bool RawObject::IsVariableSizeClassId(intptr_t index);

Done.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:230: class_table_->UpdateLiveOld(raw_obj->GetClassId(),
raw_obj->Size());
On 2014/01/17 06:53:16, Ivan Posva wrote:
> We only need the size for variable size objects. I am wondering whether we
> should avoid getting the size for the others.

Done.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:354: double seconds = static_cast<double>(micros) /
On 2014/01/17 06:53:16, Ivan Posva wrote:
> RoundMicrosecondsToSeconds

Done.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:170: double GCTimeInSeconds(Space space) const;
On 2014/01/17 06:53:16, Ivan Posva wrote:
> Why do you need both? Micros and Seconds?

Removed Seconds.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/object.cc#new...
runtime/vm/object.cc:1546: return raw_ptr()->user_name_;
On 2014/01/17 06:53:16, Ivan Posva wrote:
> Why is this allocated eagerly? Is there a reason why we do not lazily create
and
> cache this on first access?

We write protect the VM isolate's heap.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1055: { "heap", HandleHeap },
On 2014/01/17 06:53:16, Ivan Posva wrote:
> Why twice?
> 

Bad merge.

> Also "heap"/HandleHeap is a bit of a misnomer. Shouldn't it be allocation
stats,
> live stats or similar?

allocationprofile?

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:1330: // EDX: Will be clobbered.
On 2014/01/17 06:53:16, Ivan Posva wrote:
> You might want to comment in this code what the two update allocation stats
> update. The upper one is a bit clearer as it uses the kContextCid as a
constant.

Done.

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/51653006/diff/920002/runtime/vm/symbols.h#new...
runtime/vm/symbols.h:303: V(StringSym, "String")                                
                      \
On 2014/01/17 06:53:16, Ivan Posva wrote:
> Can you move this together with the remaining String symbols?
> 
> Also either just String or if that does not work _String.

Done.

[Ivan Posva, 2014-01-21 21:39:59]

LGTMwC

-Ivan

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:412: 
Two lines.

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/service.cc#n...
runtime/vm/service.cc:1058: { "allocationprofile", HandleAllocationProfile },
Sorted order?

[Cutch, 2014-01-21 23:44:05]

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:412: 
On 2014/01/21 21:40:00, Ivan Posva wrote:
> Two lines.

Done.

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/51653006/diff/1090001/runtime/vm/service.cc#n...
runtime/vm/service.cc:1058: { "allocationprofile", HandleAllocationProfile },
On 2014/01/21 21:40:00, Ivan Posva wrote:
> Sorted order?

Done.

[Cutch, 2014-01-22 19:21:55]

Committed patchset #32 manually as r31978 (presubmit successful).