Phantom references support internal fields

src/global-handles.cc

 // Copyright 2009 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/api.h"
 #include "src/global-handles.h"
 
 #include "src/vm-state-inl.h"
@@ skipping 12 matching lines @@
   delete[] children;
 }
 
 
 class GlobalHandles::Node {
  public:
   // State transition diagram:
   // FREE -> NORMAL <-> WEAK -> PENDING -> NEAR_DEATH -> { NORMAL, WEAK, FREE }
   enum State {
     FREE = 0,
-    NORMAL,     // Normal global handle.
-    WEAK,       // Flagged as weak but not yet finalized.
-    PENDING,    // Has been recognized as only reachable by weak handles.
-    NEAR_DEATH  // Callback has informed the handle is near death.
+    NORMAL,      // Normal global handle.
+    WEAK,        // Flagged as weak but not yet finalized.
+    PENDING,     // Has been recognized as only reachable by weak handles.
+    NEAR_DEATH,  // Callback has informed the handle is near death.
+    NUMBER_OF_NODE_STATES
+  };
+
+  enum WeaknessType {
+    NORMAL_WEAK,   // Embedder gets a handle to the dying object.
+    PHANTOM_WEAK,  // Embedder gets the parameter they passed in earlier.
+    INTERNAL_FIELDS_WEAK  // Embedded gets 2 internal fields from dying object.
   };
 
   // Maps handle location (slot) to the containing node.
   static Node* FromLocation(Object** location) {
     DCHECK(OFFSET_OF(Node, object_) == 0);
     return reinterpret_cast<Node*>(location);
   }
 
   Node() {
     DCHECK(OFFSET_OF(Node, class_id_) == Internals::kNodeClassIdOffset);
@@ skipping 38 matching lines @@
     object_ = object;
     class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId;
     set_independent(false);
     set_partially_dependent(false);
     set_state(NORMAL);
     parameter_or_next_free_.parameter = NULL;
     weak_callback_ = NULL;
     IncreaseBlockUses();
   }
 
+  void Zap() {

[picksi1, 2014/11/28 12:15:11]

Is 'Zap' a known term in the world of GC? For the naive reader (i.e. me) it
doesn't tell me what the function does! Should your comment inside the function
be turned into the name? e.g. MarkForEagerTrapping()?

[Erik Corry, 2014/11/28 14:21:25]

It's pretty well established, I think.

+    DCHECK(state() != FREE);
+    // Zap the values for eager trapping.
+    object_ = reinterpret_cast<Object*>(kGlobalHandleZapValue);
+  }
+
   void Release() {
     DCHECK(state() != FREE);
     set_state(FREE);
     // Zap the values for eager trapping.
     object_ = reinterpret_cast<Object*>(kGlobalHandleZapValue);
     class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId;
     set_independent(false);
     set_partially_dependent(false);
     weak_callback_ = NULL;
     DecreaseBlockUses();
@@ skipping 34 matching lines @@
     flags_ = IsPartiallyDependent::update(flags_, v);
   }
 
   bool is_in_new_space_list() {
     return IsInNewSpaceList::decode(flags_);
   }
   void set_in_new_space_list(bool v) {
     flags_ = IsInNewSpaceList::update(flags_, v);
   }
 
-  bool is_zapped_during_weak_callback() {
-    return IsZappedDuringWeakCallback::decode(flags_);
+  WeaknessType weakness_type() const {
+    return NodeWeaknessType::decode(flags_);
   }
-  void set_is_zapped_during_weak_callback(bool v) {
-    flags_ = IsZappedDuringWeakCallback::update(flags_, v);
+  void set_weakness_type(WeaknessType t) {
+    flags_ = NodeWeaknessType::update(flags_, t);

[picksi1, 2014/11/28 12:15:11]

is t a good variable name here? should it be weakness_type?

[Erik Corry, 2014/11/28 14:21:25]

Done.

   }
 
   bool IsNearDeath() const {
     // Check for PENDING to ensure correct answer when processing callbacks.
     return state() == PENDING || state() == NEAR_DEATH;
   }
 
   bool IsWeak() const { return state() == WEAK; }
 
   bool IsRetainer() const { return state() != FREE; }
@@ skipping 23 matching lines @@
   }
   void clear_partially_dependent() { set_partially_dependent(false); }
 
   // Callback accessor.
   // TODO(svenpanne) Re-enable or nuke later.
   // WeakReferenceCallback callback() { return callback_; }
 
   // Callback parameter accessors.
   void set_parameter(void* parameter) {
     DCHECK(state() != FREE);
+    DCHECK(weakness_type() == NORMAL_WEAK || weakness_type() == PHANTOM_WEAK);
     parameter_or_next_free_.parameter = parameter;
   }
   void* parameter() const {
     DCHECK(state() != FREE);
     return parameter_or_next_free_.parameter;
   }
 
+  void set_internal_fields(
+      int internal_field_index1, int internal_field_index2) {
+    DCHECK(weakness_type() == INTERNAL_FIELDS_WEAK);
+    parameter_or_next_free_.internal_field_indeces.internal_field1 =
+        internal_field_index1;
+    parameter_or_next_free_.internal_field_indeces.internal_field2 =
+        internal_field_index2;
+  }
+
+  int internal_field1() const {
+    DCHECK(weakness_type() == INTERNAL_FIELDS_WEAK);
+    return parameter_or_next_free_.internal_field_indeces.internal_field1;
+  }
+
+  int internal_field2() const {
+    DCHECK(weakness_type() == INTERNAL_FIELDS_WEAK);
+    return parameter_or_next_free_.internal_field_indeces.internal_field2;
+  }
+
   // Accessors for next free node in the free list.
   Node* next_free() {
     DCHECK(state() == FREE);
     return parameter_or_next_free_.next_free;
   }
   void set_next_free(Node* value) {
     DCHECK(state() == FREE);
     parameter_or_next_free_.next_free = value;
   }
 
-  void MakeWeak(void* parameter, WeakCallback weak_callback,
-                bool is_zapped_during_weak_callback = false) {
+  void MakeWeak(void* parameter, WeakCallback weak_callback) {
     DCHECK(weak_callback != NULL);
     DCHECK(state() != FREE);
     CHECK(object_ != NULL);
     set_state(WEAK);
+    set_weakness_type(Node::NORMAL_WEAK);
     set_parameter(parameter);
-    set_is_zapped_during_weak_callback(is_zapped_during_weak_callback);
     weak_callback_ = weak_callback;
   }
 
+  void MakePhantom(void* parameter, PhantomCallback phantom_callback,
+                   int internal_field_index1, int internal_field_index2) {
+    DCHECK(phantom_callback != NULL);
+    DCHECK(state() != FREE);
+    CHECK(object_ != NULL);
+    set_state(WEAK);
+    if (parameter != NULL) {

[picksi1, 2014/11/28 12:15:11]

Bikeshed: Might be clearer to swap the if/else bodies and turn the != into ==

[Erik Corry, 2014/11/28 14:21:25]

Done.

+      DCHECK(internal_field_index1 == v8::Object::kNoInternalFieldIndex);
+      DCHECK(internal_field_index2 == v8::Object::kNoInternalFieldIndex);
+      set_weakness_type(Node::PHANTOM_WEAK);
+      set_parameter(parameter);
+    } else {
+      set_weakness_type(Node::INTERNAL_FIELDS_WEAK);
+      set_internal_fields(internal_field_index1, internal_field_index2);
+    }
+    weak_callback_ = reinterpret_cast<WeakCallback>(phantom_callback);
+  }
+
   void* ClearWeakness() {
     DCHECK(state() != FREE);
     void* p = parameter();
     set_state(NORMAL);
     set_parameter(NULL);
     return p;
   }
 
   bool PostGarbageCollectionProcessing(Isolate* isolate) {
     if (state() != Node::PENDING) return false;
     if (weak_callback_ == NULL) {
       Release();
       return false;
     }
-    void* param = parameter();
     set_state(NEAR_DEATH);
-    set_parameter(NULL);
 
-    Object** object = location();
     {
       // Check that we are not passing a finalized external string to
       // the callback.
       DCHECK(!object_->IsExternalOneByteString() ||
              ExternalOneByteString::cast(object_)->resource() != NULL);
       DCHECK(!object_->IsExternalTwoByteString() ||
              ExternalTwoByteString::cast(object_)->resource() != NULL);
       // Leaving V8.
       VMState<EXTERNAL> vmstate(isolate);
       HandleScope handle_scope(isolate);
-      if (is_zapped_during_weak_callback()) {
-        // Phantom weak pointer case.
-        DCHECK(*object == Smi::FromInt(kPhantomReferenceZap));
-        // Make data with a null handle.
-        v8::WeakCallbackData<v8::Value, void> data(
-            reinterpret_cast<v8::Isolate*>(isolate), v8::Local<v8::Object>(),
-            param);
-        weak_callback_(data);
-        if (state() != FREE) {
-          // Callback does not have to clear the global handle if it is a
-          // phantom handle.
-          Release();
-        }
-      } else {
+      if (weakness_type() == Node::NORMAL_WEAK) {

[picksi1, 2014/11/28 12:15:11]

Could this code be refactored to switch on the weakness_type? It might be [very
slightly!] quicker than the cascading ifs, and possible clearer? Although you
may end up with duplicate phantom callback code in the PHANTOM_WEAK and
INTERNAL_FIELDS_WEAK case.

[Erik Corry, 2014/11/28 14:21:25]

I have to rewrite this anyway because I have worked out why it's completely
wrong.

+        Object** object = location();
         Handle<Object> handle(*object, isolate);
         v8::WeakCallbackData<v8::Value, void> data(
             reinterpret_cast<v8::Isolate*>(isolate), v8::Utils::ToLocal(handle),
-            param);
+            parameter());
+        set_parameter(NULL);
         weak_callback_(data);
+      } else {
+        v8::PhantomCallbackData<void>::Callback callback =
+            reinterpret_cast<v8::PhantomCallbackData<void>::Callback>(
+                weak_callback_);
+        if (weakness_type() == Node::PHANTOM_WEAK) {
+          // Phantom weak pointer case.
+          DCHECK(*location() == Smi::FromInt(kPhantomReferenceZap));
+          // Make data with a null handle.
+          v8::PhantomCallbackData<void> data(
+              reinterpret_cast<v8::Isolate*>(isolate), parameter());
+          callback(data);
+          if (state() != FREE) {

[picksi1, 2014/11/28 12:15:11]

There are lots of 'state() != FREE's in the code, is it worth creating a
function (state_not_free()?) that captures this concept?

[Erik Corry, 2014/11/28 14:21:25]

Created IsInUse() for this.

+            // Callback does not have to clear the global handle if it is a
+            // phantom handle.
+            Release();
+          }
+        } else {
+          DCHECK(weakness_type() == Node::INTERNAL_FIELDS_WEAK);
+          // Phantom weak pointer case, passing internal fields instead of
+          // parameter.
+          Handle<Object> handle(object(), isolate);
+          Handle<JSObject> jsobject = Handle<JSObject>::cast(handle);
+          v8::PhantomCallbackData<void> data(
+              reinterpret_cast<v8::Isolate*>(isolate),
+              jsobject->GetInternalField(internal_field1()),
+              jsobject->GetInternalField(internal_field2()));
+          // In the future, we want to delay the callback. In that case we will
+          // zap when we queue up, to stop the C++ side accessing the dead V8
+          // object, but we will call Release only after the callback (allowing
+          // the node to be reused).
+          Zap();
+          callback(data);
+          Release();
+        }
       }
     }
     // Absence of explicit cleanup or revival of weak handle
     // in most of the cases would lead to memory leak.
     CHECK(state() != NEAR_DEATH);
     return true;
   }
 
   inline GlobalHandles* GetGlobalHandles();
 
@@ skipping 10 matching lines @@
   // Note: the most aligned fields should go first.
 
   // Wrapper class ID.
   uint16_t class_id_;
 
   // Index in the containing handle block.
   uint8_t index_;
 
   // This stores three flags (independent, partially_dependent and
   // in_new_space_list) and a State.
-  class NodeState : public BitField<State, 0, 4> {};
-  class IsIndependent : public BitField<bool, 4, 1> {};
-  class IsPartiallyDependent : public BitField<bool, 5, 1> {};
-  class IsInNewSpaceList : public BitField<bool, 6, 1> {};
-  class IsZappedDuringWeakCallback : public BitField<bool, 7, 1> {};
+  class NodeState : public BitField<State, 0, 3> {};
+  class IsIndependent : public BitField<bool, 3, 1> {};
+  class IsPartiallyDependent : public BitField<bool, 4, 1> {};
+  class IsInNewSpaceList : public BitField<bool, 5, 1> {};
+  class NodeWeaknessType : public BitField<WeaknessType, 6, 2> {};
 
   uint8_t flags_;
 
   // Handle specific callback - might be a weak reference in disguise.
   WeakCallback weak_callback_;
 
   // Provided data for callback.  In FREE state, this is used for
   // the free list link.
   union {
     void* parameter;
+    struct {
+      uint16_t internal_field1;
+      uint16_t internal_field2;
+    } internal_field_indeces;
     Node* next_free;
   } parameter_or_next_free_;
 
   DISALLOW_COPY_AND_ASSIGN(Node);
 };
 
 
 class GlobalHandles::NodeBlock {
  public:
   static const int kSize = 256;
@@ skipping 165 matching lines @@
   return Node::FromLocation(location)->GetGlobalHandles()->Create(*location);
 }
 
 
 void GlobalHandles::Destroy(Object** location) {
   if (location != NULL) Node::FromLocation(location)->Release();
 }
 
 
 void GlobalHandles::MakeWeak(Object** location, void* parameter,
-                             WeakCallback weak_callback, PhantomState phantom) {
-  Node::FromLocation(location)
-      ->MakeWeak(parameter, weak_callback, phantom == Phantom);
+                             WeakCallback weak_callback) {
+  Node::FromLocation(location)->MakeWeak(parameter, weak_callback);
 }
 
 
+void GlobalHandles::MakePhantom(Object** location, void* parameter,
+                                PhantomCallback phantom_callback,
+                                int internal_field_index1,
+                                int internal_field_index2) {
+  Node::FromLocation(location)

[picksi1, 2014/11/28 12:15:11]

Do internal_field_index1/2 have anything to do with the internal_field1/2
elements in the union+struct on line 388 above? They are named almost the same
but are different types, should they be "harmonised"?

[Erik Corry, 2014/11/28 14:21:25]

Done.

+      ->MakePhantom(parameter, phantom_callback, internal_field_index1,
+                    internal_field_index2);
+}
+
+
 void* GlobalHandles::ClearWeakness(Object** location) {
   return Node::FromLocation(location)->ClearWeakness();
 }
 
 
 void GlobalHandles::MarkIndependent(Object** location) {
   Node::FromLocation(location)->MarkIndependent();
 }
 
 
@@ skipping 14 matching lines @@
 
 bool GlobalHandles::IsWeak(Object** location) {
   return Node::FromLocation(location)->IsWeak();
 }
 
 
 void GlobalHandles::IterateWeakRoots(ObjectVisitor* v) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
     Node* node = it.node();
     if (node->IsWeakRetainer()) {
-      if (node->state() == Node::PENDING &&
-          node->is_zapped_during_weak_callback()) {
-        *(node->location()) = Smi::FromInt(kPhantomReferenceZap);
+      // Weakness type can be normal, phantom or internal fields.
+      // For normal weakness we mark through the handle so that
+      // the object and things reachable from it are available
+      // to the callback.
+      // In the case of phantom we can zap the object handle now
+      // and we won't need it, so we don't need to mark through it.
+      // In the internal fields case we will need the internal
+      // fields, so we can't zap the handle, but we don't need to
+      // mark through it, because it will die in this GC round.
+      if (node->state() == Node::PENDING) {

[picksi1, 2014/11/28 12:15:11]

switch? Not sure if a 3 case switch beats 2 ifs on performance!

[Erik Corry, 2014/11/28 14:21:25]

The compiler will certainly convert a 3 case switch into two 'if's.  I think the
ifs are more readable.

+        if (node->weakness_type() == Node::PHANTOM_WEAK) {
+          *(node->location()) = Smi::FromInt(kPhantomReferenceZap);
+        } else if (node->weakness_type() == Node::NORMAL_WEAK) {
+          v->VisitPointer(node->location());
+        } else {
+          DCHECK(node->weakness_type() == Node::INTERNAL_FIELDS_WEAK);
+        }
       } else {
         v->VisitPointer(node->location());
       }
     }
   }
 }
 
 
 void GlobalHandles::IdentifyWeakHandles(WeakSlotCallback f) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
@@ skipping 28 matching lines @@
   }
 }
 
 
 void GlobalHandles::IterateNewSpaceWeakIndependentRoots(ObjectVisitor* v) {
   for (int i = 0; i < new_space_nodes_.length(); ++i) {
     Node* node = new_space_nodes_[i];
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || node->is_partially_dependent()) &&
         node->IsWeakRetainer()) {
-      if (node->is_zapped_during_weak_callback()) {
+      if (node->weakness_type() == Node::PHANTOM_WEAK) {
         *(node->location()) = Smi::FromInt(kPhantomReferenceZap);
+      } else if (node->weakness_type() == Node::NORMAL_WEAK) {
+        v->VisitPointer(node->location());
       } else {
-        v->VisitPointer(node->location());
+        DCHECK(node->weakness_type() == Node::INTERNAL_FIELDS_WEAK);
+        // For this case we only need to trace if it's alive: The tracing of
+        // something that is already alive is just to get the pointer updated
+        // to the new location of the object).
+        if (!node->IsNearDeath()) {
+          v->VisitPointer(node->location());
+        }
       }
     }
   }
 }
 
 
 bool GlobalHandles::IterateObjectGroups(ObjectVisitor* v,
                                         WeakSlotCallbackWithHeap can_skip) {
   ComputeObjectGroupsAndImplicitReferences();
   int last = 0;
@@ skipping 475 matching lines @@
   DCHECK_EQ(isolate->heap()->the_hole_value(), blocks_[block][offset]);
   blocks_[block][offset] = object;
   if (isolate->heap()->InNewSpace(object)) {
     new_space_indices_.Add(size_);
   }
   *index = size_++;
 }
 
 
 } }  // namespace v8::internal