Avoid duplicate null checks when calling SetRaw from InitializeHandle

vm/object.h

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #ifndef VM_OBJECT_H_
 #define VM_OBJECT_H_
 
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "platform/utils.h"
@@ skipping 95 matching lines @@
   }                                                                            \
   static Raw##object* null() {                                                 \
     return reinterpret_cast<Raw##object*>(Object::null());                     \
   }                                                                            \
   virtual const char* ToCString() const;                                       \
   static const ClassId kClassId = k##object##Cid;                              \
  protected:  /* NOLINT */                                                      \
   object() : super() {}                                                        \
  private:  /* NOLINT */                                                        \
   /* 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;                                                      \
       obj->set_vtable(fake_object.vtable());                                   \
     }                                                                          \
   }                                                                            \
   /* Disallow allocation, copy constructors and override super assignment. */  \
   void* operator new(size_t size);                                             \
   object(const object& value);                                                 \
   void operator=(Raw##super* value);                                           \
   void operator=(const object& value);                                         \
   void operator=(const super& value);                                          \
@@ skipping 188 matching lines @@
 
  protected:
   // Used for extracting the C++ vtable during bringup.
   Object() : raw_(null_) {}
 
   uword raw_value() const {
     return reinterpret_cast<uword>(raw());
   }
 
   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; }
 
   static RawObject* Allocate(intptr_t cls_id,
                              intptr_t size,
                              Heap::Space space);
 
   static intptr_t RoundedAllocationSize(intptr_t size) {
     return Utils::RoundUp(size, kObjectAlignment);
@@ skipping 13 matching lines @@
     if (!value->IsHeapObject()) return;
     if (value->IsNewObject() && raw()->IsOldObject()) {
       uword ptr = reinterpret_cast<uword>(addr);
       Isolate::Current()->store_buffer()->AddPointer(ptr);
     }
   }
 
   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,
                                               const Script& script,
                                               const Library& lib);
   static void RegisterClass(const Class& cls,
                             const String& name,
                             const Library& lib);
   static void RegisterPrivateClass(const Class& cls,
                                    const String& name,
@@ skipping 5270 matching lines @@
 // Breaking cycles and loops.
 RawClass* Object::clazz() const {
   uword raw_value = reinterpret_cast<uword>(raw_);
   if ((raw_value & kSmiTagMask) == kSmiTag) {
     return Smi::Class();
   }
   return Isolate::Current()->class_table()->At(raw()->GetClassId());
 }
 
 
+
 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) {
     Heap* isolate_heap = isolate->heap();
     Heap* vm_isolate_heap = Dart::vm_isolate()->heap();
     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]);
-  }
 }
 
 
 bool Function::HasCode() const {
   return raw_ptr()->code_ != Code::null();
 }
 
 
 intptr_t Field::Offset() const {
   ASSERT(!is_static());  // Offset is valid only for instance fields.
@@ skipping 37 matching lines @@
     if (this->CharAt(i) != str.CharAt(begin_index + i)) {
       return false;
     }
   }
   return true;
 }
 
 }  // namespace dart
 
 #endif  // VM_OBJECT_H_