More cleanup to avoid creation of redundant handles

runtime/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 92 matching lines @@
   static object& CheckedZoneHandle(RawObject* raw_ptr) {                       \
     return CheckedZoneHandle(Isolate::Current(), raw_ptr);                     \
   }                                                                            \
   /* T::Cast cannot be applied to a null Object, because the object vtable */  \
   /* is not setup for type T, although some methods are supposed to work   */  \
   /* with null, for example Instance::Equals().                            */  \
   static const object& Cast(const Object& obj) {                               \
     ASSERT(obj.Is##object());                                                  \
     return reinterpret_cast<const object&>(obj);                               \
   }                                                                            \
+  static Raw##object* RawCast(RawObject* raw) {                                \
+    ASSERT(Object::Handle(raw).Is##object());                                  \

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

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.

[siva, 2013/05/31 22:35:48]

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:

+    return reinterpret_cast<Raw##object*>(raw);                                \
+  }                                                                            \
   static Raw##object* null() {                                                 \
     return reinterpret_cast<Raw##object*>(Object::null());                     \
   }                                                                            \
+  static const object& null_object() {                                         \
+    return reinterpret_cast<const object&>(Object::null_object());             \
+  }                                                                            \
   virtual const char* ToCString() const;                                       \
   static const ClassId kClassId = k##object##Cid;                              \
  private:  /* NOLINT */                                                        \
   /* Initialize the handle based on the raw_ptr in the presence of null. */    \
   static void initializeHandle(object* obj, RawObject* raw_ptr) {              \
     if (raw_ptr != Object::null()) {                                           \
       obj->SetRaw(raw_ptr);                                                    \
     } else {                                                                   \
       obj->raw_ = Object::null();                                              \
       object fake_object;                                                      \
@@ skipping 172 matching lines @@
 
   static Object& ZoneHandle() {
     return ZoneHandle(Isolate::Current(), null_);
   }
 
   static Object& ZoneHandle(RawObject* raw_ptr) {
     return ZoneHandle(Isolate::Current(), raw_ptr);
   }
 
   static RawObject* null() { return null_; }
+  static const Object& null_object() {
+    ASSERT(null_object_ != NULL);
+    return *null_object_;
+  }
   static const Array& empty_array() {
     ASSERT(empty_array_ != NULL);
     return *empty_array_;
   }
 
   // The sentinel is a value that cannot be produced by Dart code.
   // It can be used to mark special values, for example to distinguish
   // "uninitialized" fields.
   static const Instance& sentinel() {
     ASSERT(sentinel_ != NULL);
@@ skipping 17 matching lines @@
   }
 
   static const Bool& bool_true() {
     ASSERT(bool_true_ != NULL);
     return *bool_true_;
   }
   static const Bool& bool_false() {
     ASSERT(bool_false_ != NULL);
     return *bool_false_;
   }
+
+  static const Smi& sentinel_smi() {
+    ASSERT(sentinel_smi_ != NULL);
+    return *sentinel_smi_;
+  }
   static const LanguageError& snapshot_writer_error() {
     ASSERT(snapshot_writer_error_ != NULL);
     return *snapshot_writer_error_;
   }
 
   static RawClass* class_class() { return class_class_; }
   static RawClass* null_class() { return null_class_; }
   static RawClass* dynamic_class() { return dynamic_class_; }
   static RawClass* void_class() { return void_class_; }
   static RawClass* unresolved_class_class() { return unresolved_class_class_; }
@@ skipping 171 matching lines @@
   static RawClass* icdata_class_;  // Class of ICData.
   static RawClass* megamorphic_cache_class_;  // Class of MegamorphiCache.
   static RawClass* subtypetestcache_class_;  // Class of SubtypeTestCache.
   static RawClass* api_error_class_;  // Class of ApiError.
   static RawClass* language_error_class_;  // Class of LanguageError.
   static RawClass* unhandled_exception_class_;  // Class of UnhandledException.
   static RawClass* unwind_error_class_;  // Class of UnwindError.
 
   // The static values below are read-only handle pointers for singleton
   // objects that are shared between the different isolates.
+  static Object* null_object_;
   static Array* empty_array_;
   static Instance* sentinel_;
   static Instance* transition_sentinel_;
   static Instance* unknown_constant_;
   static Instance* non_constant_;
   static Bool* bool_true_;
   static Bool* bool_false_;
+  static Smi* sentinel_smi_;

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

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:48]

Renamed it smi_illegal_cid_

On 2013/05/31 16:40:25, hausner wrote:

   static LanguageError* snapshot_writer_error_;
 
   friend void ClassTable::Register(const Class& cls);
   friend void RawObject::Validate(Isolate* isolate) const;
   friend class Closure;
   friend class SnapshotReader;
   friend class OneByteString;
   friend class TwoByteString;
   friend class ExternalOneByteString;
   friend class ExternalTwoByteString;
@@ skipping 2691 matching lines @@
   void set_deopt_id(intptr_t value) const;
   void set_num_args_tested(intptr_t value) const;
   void set_ic_data(const Array& value) const;
 
 #if defined(DEBUG)
   // Used in asserts to verify that a check is not added twice.
   bool HasCheck(const GrowableArray<intptr_t>& cids) const;
 #endif  // DEBUG
 
   intptr_t TestEntryLength() const;
-  void WriteSentinel() const;
+  void WriteSentinel(const Array& data) const;
 
   FINAL_HEAP_OBJECT_IMPLEMENTATION(ICData, Object);
   friend class Class;
 };
 
 
 class MegamorphicCache : public Object {
  public:
   static const int kInitialCapacity = 16;
   static const double kLoadFactor;
@@ skipping 2557 matching lines @@
 
 
 RawObject* MegamorphicCache::GetTargetFunction(const Array& array,
                                                intptr_t index) {
   return array.At((index * kEntryLength) + kTargetFunctionIndex);
 }
 
 }  // namespace dart
 
 #endif  // VM_OBJECT_H_