More cleanup to avoid creation of redundant handles

More cleanup to avoid creation of redundant handles
- create read only handles for null object and sentinel smi value
- add a RawCast function to allow direct casting of Raw pointers without the
  need to create a Handle for it
- use Smi::Value where possible
- hoist out handle creation from some loops

R=hausner@google.com

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

[hausner, 2013-05-31 16:40:25]

LGTM iff I'm wrong about the handle allocation bug in RawCast.  :-)

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

https://codereview.chromium.org/15689012/diff/8003/runtime/vm/object.h#newcod...
runtime/vm/object.h:114: ASSERT(Object::Handle(raw).Is##object());              
                   \
I always cringe at expensive asserts in very low level functions. The difference
between debug and release code in terms of performance should be reasonably
small, otherwise debug builds become unusable.

I would let the call site of this function determine whether an assertion is
desired or not.

In fact, I think the assertion even introduces a bug. Allocating a handle can
cause the GC to run and invalidate the raw pointer that is passed to RawCast.

https://codereview.chromium.org/15689012/diff/8003/runtime/vm/object.h#newcod...
runtime/vm/object.h:560: static Smi* sentinel_smi_;
I would name this Smi illegal_cid or something similar, to make the connection
with the value it holds (kIllegalCid).

[siva, 2013-05-31 22:35:47]

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

https://codereview.chromium.org/15689012/diff/8003/runtime/vm/object.h#newcod...
runtime/vm/object.h:114: ASSERT(Object::Handle(raw).Is##object());              
                   \
A handle creation can not lead to garbage collection as it is a C
heap allocation. It can lead to an OOM from the process which is
possible anywhere.

Regarding the assert making the debug build slower that is true.
We have a number of such checks which are true in debug mode.
I need to see how this check can be made less expensive in
a new CL.

On 2013/05/31 16:40:25, hausner wrote:
> I always cringe at expensive asserts in very low level functions. The
difference
> between debug and release code in terms of performance should be reasonably
> small, otherwise debug builds become unusable.
> 
> I would let the call site of this function determine whether an assertion is
> desired or not.
> 
> In fact, I think the assertion even introduces a bug. Allocating a handle can
> cause the GC to run and invalidate the raw pointer that is passed to RawCast.

https://codereview.chromium.org/15689012/diff/8003/runtime/vm/object.h#newcod...
runtime/vm/object.h:560: static Smi* sentinel_smi_;
Renamed it smi_illegal_cid_

On 2013/05/31 16:40:25, hausner wrote:
> I would name this Smi illegal_cid or something similar, to make the connection
> with the value it holds (kIllegalCid).

[siva, 2013-05-31 22:55:34]

Committed patchset #6 manually as r23502 (presubmit successful).

[Ivan Posva, 2013-06-01 00:34:40]

We should address the bad null handles here. Probably by creating a null handle
for each particular handle type.

-Ivan

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

https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1066: cls.set_super_type(Type::null_object());
There is a big difference here: On the left set_super_type is called with a
handle that has the vtable set to Type, while on the right the vtable is from
Object even though it masquerades as Type here.

https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4880: result.set_value(Instance::null_object());
ditto, here and below.

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

https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.h#newco...
runtime/vm/object.h:121: return reinterpret_cast<const
object&>(Object::null_object());             \
This will not have the correct vtable setup.

[siva, 2013-06-03 21:24:33]

On 2013/06/01 00:34:40, Ivan Posva wrote:
> We should address the bad null handles here. Probably by creating a null
handle
> for each particular handle type.
> 
> -Ivan
> 
> https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.cc#newc...
> runtime/vm/object.cc:1066: cls.set_super_type(Type::null_object());
> There is a big difference here: On the left set_super_type is called with a
> handle that has the vtable set to Type, while on the right the vtable is from
> Object even though it masquerades as Type here.
> 
>
https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.cc#newc...
> runtime/vm/object.cc:4880: result.set_value(Instance::null_object());
> ditto, here and below.
> 
> https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.h
> File runtime/vm/object.h (right):
> 
>
https://codereview.chromium.org/15689012/diff/15006/runtime/vm/object.h#newco...
> runtime/vm/object.h:121: return reinterpret_cast<const
> object&>(Object::null_object());             \
> This will not have the correct vtable setup.

Addressed this comment in https://codereview.chromium.org/16336021/