Avoid duplicate null checks when calling SetRaw from InitializeHandle

vm/object.h

Index: vm/object.h
===================================================================
--- vm/object.h	(revision 14420)
+++ vm/object.h	(working copy)
@@ -115,7 +115,7 @@
   /* Initialize the handle based on the raw_ptr in the presence of null. */    \
   static void initializeHandle(object* obj, RawObject* raw_ptr) {              \

[Florian Schneider, 2012/11/01 22:35:36]

How about adding a 

initializeHandle(object* obj, Raw##object* raw_ptr)

and 

Smi::SetRaw 

to avoid the smi-tag check that is currently in Object::SetRaw when creating a
Smi::Handle?

[siva, 2012/11/06 02:27:08]

Will have to consider this in a different CL.

On 2012/11/01 22:35:36, Florian Schneider wrote:

     if (raw_ptr != Object::null()) {                                           \
-      obj->SetRaw(raw_ptr);                                                    \
+      obj->SetNonNullRaw(raw_ptr);                                             \
     } else {                                                                   \
       obj->raw_ = Object::null();                                              \

[Ivan Posva, 2012/11/01 21:33:34]

We had discussed in the past whether this is the correct behaviour for null_
handles. Maybe we should reevaluate whether this check is really needed here.

[siva, 2012/11/01 21:59:53]

I think we wanted to avoid the case where assignment of
a null_ would suddenly make Is##object() return false.

e.g:

String& str = String::Handle(...);
ASSERT(str.IsString());  // Valid.
str ^= String::null();
ASSERT(str.IsString());  // Still Valid.

If we don't do this check in initializeHandle we end up with
the second assert being invalid.

On 2012/11/01 21:33:34, Ivan Posva wrote:

       object fake_object;                                                      \
@@ -326,6 +326,7 @@
   }
 
   inline void SetRaw(RawObject* value);
+  inline void SetNonNullRaw(RawObject* value);
 
   cpp_vtable vtable() const { return bit_copy<cpp_vtable>(*this); }
   void set_vtable(cpp_vtable value) { *vtable_address() = value; }
@@ -359,6 +360,8 @@
   RawObject* raw_;  // The raw object reference.
 
  private:
+  inline void SetRawHelper();
+
   static void InitializeObject(uword address, intptr_t id, intptr_t size);
 
   static RawClass* CreateAndRegisterInterface(const char* cname,
@@ -5649,19 +5652,40 @@
 }
 
 
+
 void Object::SetRaw(RawObject* value) {
   // NOTE: The assignment "raw_ = value" should be the first statement in
   // this function. Also do not use 'value' in this function after the
   // assignment (use 'raw_' instead).
   raw_ = value;
+  if (raw_ == null_) {

[Ivan Posva, 2012/11/01 21:33:34]

The special case should not have been necessary in the original code if we set
the vtable of null_ to the Object vtable in the builtin_vtables_ list. That
might actually be a fine solution avoiding the split here.

[siva, 2012/11/01 21:59:53]

the vtable of kNullCid in builtin_vtables_ you mean..

essentially:
  // Allocate and initialize the null class.
  cls = Class::New<Instance>(kNullCid);

should have been

  // Allocate and initialize the null class.
  cls = Class::New<Object>(kNullCid);

then yes we don't need this additional check and the
initializeHandle code can remain the way it is currently.


On 2012/11/01 21:33:34, Ivan Posva wrote:

+    set_vtable(handle_vtable_);
+    return;
+  }
+  SetRawHelper();
+}
+
+
+void Object::SetNonNullRaw(RawObject* value) {
+  // NOTE: The assignment "raw_ = value" should be the first statement in
+  // this function. Also do not use 'value' in this function after the
+  // assignment (use 'raw_' instead).
+  raw_ = value;
+  ASSERT(raw_ != null_);
+  SetRawHelper();
+}
+
+
+void Object::SetRawHelper() {
   if ((reinterpret_cast<uword>(raw_) & kSmiTagMask) == kSmiTag) {
     set_vtable(Smi::handle_vtable_);
     return;
-  } else if (raw_ == null_) {
-    set_vtable(handle_vtable_);
-    return;
   }
-
+  intptr_t cid = raw_->GetClassId();
+  if (cid >= kNumPredefinedCids) {
+    cid = kInstanceCid;
+  }
+  set_vtable(builtin_vtables_[cid]);
 #if defined(DEBUG)
   Isolate* isolate = Isolate::Current();
   if (FLAG_verify_handles) {
@@ -5670,17 +5694,9 @@
     ASSERT(isolate_heap->Contains(reinterpret_cast<uword>(raw_->ptr())) ||
            vm_isolate_heap->Contains(reinterpret_cast<uword>(raw_->ptr())));
   }
+  ASSERT(builtin_vtables_[cid] ==
+         isolate->class_table()->At(cid)->ptr()->handle_vtable_);
 #endif
-  intptr_t cid = raw_->GetClassId();
-  if (cid < kNumPredefinedCids) {
-#if defined(DEBUG)
-    ASSERT(builtin_vtables_[cid] ==
-           isolate->class_table()->At(cid)->ptr()->handle_vtable_);
-#endif
-    set_vtable(builtin_vtables_[cid]);
-  } else {
-    set_vtable(builtin_vtables_[kInstanceCid]);
-  }
 }