- Do not mix scalar values and object fields in RawInstance to

- Do not mix scalar values and object fields in RawInstance to
  simplify the traversal for GC. Classes with native fields
  have a hidden reference to a typed array containing the
  native fields.
Committed: https://code.google.com/p/dart/source/detail?r=14821

[cshapiro, 2012-11-12 18:48:45]

Something until you re-upload with your new changes...

https://codereview.chromium.org/11312183/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11312183/diff/1/runtime/vm/object.h#newcode3078
runtime/vm/object.h:3078: void SetNativeField(int index, intptr_t value) const;
Interesting, GetNativeField needs the isolate but SetNativeField does not?

https://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.cc
File runtime/vm/raw_object.cc (right):

https://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.cc#newc...
runtime/vm/raw_object.cc:596: uword tags = raw_obj->ptr()->tags_;
Any reason we are not just using RawObject::Size() to populate instance_size?

https://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:1240: #define RawIntPtrArray RawInt32Array
Feel free to disregard this but... given that this will store what is
essentially bits as opposed to integers, an unsigned type would seem more
natural.

https://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:1248: 
2 spaces?

[siva, 2012-11-12 19:53:01]

Adding a redirection to the native fields is going to
impact access to native fields which is done quite
frequently by the DOM code (adding Tom to the cc list as
he did some work recently in this area to speed up
Drameo benchmark).

Why not consider some alternatives, e.g:
- set the instance size to 0 in the header for classes
  with native fields
- burn a bit to indicate class has native fields

[Tom Ball, 2012-11-12 21:05:15]

I'm not sure the indirection will cost that much, since the offset still had to
be calculated.  I'll patch this into a clean client and see what the actual
delta is for the UseDartApi benchmark.

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

http://codereview.chromium.org/11312183/diff/1/runtime/vm/object.h#newcode3078
runtime/vm/object.h:3078: void SetNativeField(int index, intptr_t value) const;
On 2012/11/12 18:48:45, cshapiro wrote:
> Interesting, GetNativeField needs the isolate but SetNativeField does not?

SetNativeField calls Object::Handle(RawObject*), which calls
Isolate::Current().  If the isolate is already known to the caller, passing it
in would avoid this extra call.

[Ivan Posva, 2012-11-12 22:01:03]

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

http://codereview.chromium.org/11312183/diff/1/runtime/vm/object.h#newcode3078
runtime/vm/object.h:3078: void SetNativeField(int index, intptr_t value) const;
On 2012/11/12 21:05:15, Tom Ball wrote:
> On 2012/11/12 18:48:45, cshapiro wrote:
> > Interesting, GetNativeField needs the isolate but SetNativeField does not?
> 
> SetNativeField calls Object::Handle(RawObject*), which calls
> Isolate::Current().  If the isolate is already known to the caller, passing it
> in would avoid this extra call.

The reason I pass the isolate in GetNativeField is because that is a much more
frequent operation according to our measurement of Dart API usage. The impact of
this change drops from 16.5% to 6% with passing the isolate and inlining the
call. I am sure there is more optimization opportunity inside GetNativeField.

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

http://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.cc#newco...
runtime/vm/raw_object.cc:596: uword tags = raw_obj->ptr()->tags_;
On 2012/11/12 18:48:45, cshapiro wrote:
> Any reason we are not just using RawObject::Size() to populate instance_size?

I tried to keep it as similar as possible to the original code, and since I
think one of the next steps is to explode this code into VisitPointers directly,
I did not prettify it much.

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

http://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.h#newcod...
runtime/vm/raw_object.h:1240: #define RawIntPtrArray RawInt32Array
On 2012/11/12 18:48:45, cshapiro wrote:
> Feel free to disregard this but... given that this will store what is
> essentially bits as opposed to integers, an unsigned type would seem more
> natural.

The external API defines it as intptr_t, which is why I chose the signed
versions here.

http://codereview.chromium.org/11312183/diff/1/runtime/vm/raw_object.h#newcod...
runtime/vm/raw_object.h:1248: 
On 2012/11/12 18:48:45, cshapiro wrote:
> 2 spaces?

Done.

[Ivan Posva, 2012-11-12 22:29:24]

PTAL

[Tom Ball, 2012-11-12 22:36:19]

lgtm

[cshapiro, 2012-11-12 22:49:30]

lgtm

[siva, 2012-11-12 23:12:29]

lgtm