Request for comments on overall approach.

Remove RawObject::class_ field.

Request for comments on overall approach.

Disregard changes in x64 related files (look at ia32 only and platform independent ones).

Contains some hacks and code duplication that are known, and will be addressed.

I will work on splitting this CL into pieces when I am ready to commit it.

NOT FOR COMMIT

BUG=
TEST=

[Vyacheslav Egorov (Google), 2012-05-24 12:29:34]

Ivan & Siva, 

Here is promised CL with what I have now with regards to class_ removal. VM
builds and runs simple programs.

I will work on splitting it into individual pieces, but if you want you can take
a glance at what I have.

Dirty hacks inside, it will be cleaned up during split.

Thanks.

[Vyacheslav Egorov (Google), 2012-05-24 13:36:19]

minor update: fixed register usage GenerateNArgsCheckInlineCacheStub [ideally I
would like to store class id instead of class in the IC data, but it seems that
sometimes it contains some smi instead of class (might be a bug, did not
investigate yet)]

Now almost all tests pass. All that do not pass fall with the same assertion in
the GC, so probably only one bug is left.

[01:53 | 100% | + 4637 | -   23]

[Vyacheslav Egorov (Google), 2012-05-24 15:57:00]

another minor update: we are down to one failure (that looks like a bug in the
test). scavenger and freelist handling is fixed now.

FAILED: none-vm debug_ia32 vm/cc/GrowableObjectArray
Expected: pass 
Actual: crash

stdout:
Running test: GrowableObjectArray

stderr:
runtime/vm/object_test.cc:1811: error: expected: obj.IsArray()
runtime/vm/object.h:3565: error: expected: IsNull() || IsArray()

Command line: out/Debug_ia32/run_vm_tests GrowableObjectArray
--ignore-unrecognized-flags

[02:11 | 100% | + 4659 | -    1]

[Ivan Posva, 2012-05-26 04:34:46]

First round of comments. I am sure Siva has some on the snapshotting code.

-Ivan

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/assemble...
File runtime/vm/assembler_ia32.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/assemble...
runtime/vm/assembler_ia32.h:538: void CompareClassOf(Register object, const
Class& clazz, Register scratch);
When do you ever have to compare classes? Or asked differently: In which
situation are classids not good enough?

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/freelist...
runtime/vm/freelist.h:56: uword next_;
I don't think this works. How do you plan to iterate over the heap? The first
word needs exactly the same header layout as RawObject for that to work. The
nice part is that for most free objects the header word already contains the
size so you can use the second word for next and a third word for size iff the
size in the header cannot fit the size of the free object.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/gc_marke...
File runtime/vm/gc_marker.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/gc_marke...
runtime/vm/gc_marker.cc:143: RawClass* raw_class = raw_obj->GetClass();
This does not need to be done early anymore as the marking bit does not
interfere with reading the class.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/gc_marke...
runtime/vm/gc_marker.cc:152: MarkObject(raw_class);
How about
MarkClass(raw_obj->ClassId());
or something similar? Within the MarkingVisitor we already have the isolate
handy by way of heap so it should be much cheaper than looking it up during
raw_obj->GetClass().

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.c...
runtime/vm/object.cc:6298: void Code::FindAndPrint(uword pc) {
?

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.h...
runtime/vm/object.h:4887: return raw_->GetClass();
Please no functionality on RawObjects.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/opt_code...
File runtime/vm/opt_code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/opt_code...
runtime/vm/opt_code_generator_ia32.cc:1425: __ CompareClassId(EAX, cls);
Not sure about others, but I find this more readable:
__ cmpl(EAX, Immediate(cls.index()));

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
File runtime/vm/raw_object.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.cc:272: RawClass* raw_class = GetClass();
The ObjectKind is equivalent to the instance_kind_ for all ids less than
kFreeListElement. All ids greater than kNumPredefineKinds are either Instance or
Closure, which we plan to resolve in a different manner with Siva.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.cc:447: if (instance_kind == kInstructions) {
Just use the class id field here.

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

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:320: return GetClass(ptr()->tags_);
If anything then all we should have here is GetClassId().

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:323: static RawClass* GetClass(uword tags);
Does not belong here. Please move to Object, or ClassTable.

[Vyacheslav Egorov (Google), 2012-05-26 16:48:22]

Thanks for the initial comments!

I will start splitting CLs: first CL would be changes in IA32 code generation
(macros, etc). I will revert changes in codegen in places where class_ was
initialized.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/assemble...
File runtime/vm/assembler_ia32.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/assemble...
runtime/vm/assembler_ia32.h:538: void CompareClassOf(Register object, const
Class& clazz, Register scratch);
On 2012/05/26 04:34:46, Ivan Posva wrote:
> When do you ever have to compare classes? Or asked differently: In which
> situation are classids not good enough?

This macros was introduced initially to cover a very common code pattern that
loads class and then compares to some constant. 

This does compare class ids internally. 

I can expand it in places where it is used but it seems cleaner when it's
collapsed.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/freelist...
runtime/vm/freelist.h:56: uword next_;
On 2012/05/26 04:34:46, Ivan Posva wrote:
> I don't think this works. How do you plan to iterate over the heap? The first
> word needs exactly the same header layout as RawObject for that to work. The
> nice part is that for most free objects the header word already contains the
> size so you can use the second word for next and a third word for size iff the
> size in the header cannot fit the size of the free object.

Yes it does have exactly the same layout as RawObject: I removed class_ from
RawObject so class_ has to be removed from here (interesting thing was that
there were absolutely no assertions verifying layout match. I added one now).

Iteration works as follows: first word in all objects now is tags_. When
calculating class based on tags_ we check for free bit and if it is present we
return FreeListElement class. 

When calculating size we just always look at the size_ (second word) because now
we have enough space in the header to put it there.

> nice part is that for most free objects the header word already contains the
> size

Sorry, I am not following here. next_ field was always destroying tags_ entirely
--- so there was not size encoded in tags for objects bigger than 2 words ->
iteration always had to take it from the size_ field. Current implementation of
RawObject::Size() checks for FreeBit and falls through to SizeFromClass in this
case; I did not even need to change it.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/gc_marke...
File runtime/vm/gc_marker.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/gc_marke...
runtime/vm/gc_marker.cc:152: MarkObject(raw_class);
On 2012/05/26 04:34:46, Ivan Posva wrote:
> How about
> MarkClass(raw_obj->ClassId());
> or something similar? Within the MarkingVisitor we already have the isolate
> handy by way of heap so it should be much cheaper than looking it up during
> raw_obj->GetClass().

Nice idea. Will do.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.c...
runtime/vm/object.cc:6298: void Code::FindAndPrint(uword pc) {
On 2012/05/26 04:34:46, Ivan Posva wrote:
> ?

This is a very useful helper method for debugging. If you have a crash in native
code it is easy to do: 

p dart::Code::FindAndPrint($pc)

to get the whole function (with comments). saves a lot of typing. 

I will obviously submit it as an entirely separate CL.

or I can try to write GDB macros, but having it right in the C++ has the
benefit: if we change something in Code/Instructions we will most definitely
forget to update GDB macros, but this guy will always stay updated.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/object.h...
runtime/vm/object.h:4887: return raw_->GetClass();
On 2012/05/26 04:34:46, Ivan Posva wrote:
> Please no functionality on RawObjects.

ok. There is RawObject::Size so I assumed that RawObject::GetClass would be also
fine. 

will fix.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/opt_code...
File runtime/vm/opt_code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/opt_code...
runtime/vm/opt_code_generator_ia32.cc:1425: __ CompareClassId(EAX, cls);
On 2012/05/26 04:34:46, Ivan Posva wrote:
> Not sure about others, but I find this more readable:
> __ cmpl(EAX, Immediate(cls.index()));

ok. I will inline them. I agree that macros does not buy as anything in this
case. I introduced it only because I wanted to be able to quickly find places
where I compare loaded class ids.

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

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:320: return GetClass(ptr()->tags_);
On 2012/05/26 04:34:46, Ivan Posva wrote:
> If anything then all we should have here is GetClassId().

Will do.

https://chromiumcodereview.appspot.com/10450014/diff/7001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:323: static RawClass* GetClass(uword tags);
On 2012/05/26 04:34:46, Ivan Posva wrote:
> Does not belong here. Please move to Object, or ClassTable.

Will do.