Phantom references support internal fields

src/global-handles.cc

Index: src/global-handles.cc
diff --git a/src/global-handles.cc b/src/global-handles.cc
index 574b2489ea2bc1e21dc8d917a2935700a050bdee..8a0ba977bc16f74daadb4d0f712cb0b930e59f93 100644
--- a/src/global-handles.cc
+++ b/src/global-handles.cc
@@ -30,10 +30,17 @@ class GlobalHandles::Node {
   // 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.
@@ -92,6 +99,12 @@ class GlobalHandles::Node {
     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);
@@ -146,11 +159,11 @@ class GlobalHandles::Node {
     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 {
@@ -194,6 +207,7 @@ class GlobalHandles::Node {
   // 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 {
@@ -201,6 +215,25 @@ class GlobalHandles::Node {
     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);
@@ -211,17 +244,34 @@ class GlobalHandles::Node {
     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();
@@ -236,11 +286,8 @@ class GlobalHandles::Node {
       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.
@@ -251,25 +298,48 @@ class GlobalHandles::Node {
       // 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
@@ -300,11 +370,11 @@ class GlobalHandles::Node {
 
   // 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_;
 
@@ -315,6 +385,10 @@ class GlobalHandles::Node {
   // 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_;
 
@@ -500,9 +574,18 @@ void GlobalHandles::Destroy(Object** location) {
 
 
 void GlobalHandles::MakeWeak(Object** location, void* parameter,
-                             WeakCallback weak_callback, PhantomState 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.

-      ->MakeWeak(parameter, weak_callback, phantom == Phantom);
+      ->MakePhantom(parameter, phantom_callback, internal_field_index1,
+                    internal_field_index2);
 }
 
 
@@ -540,9 +623,23 @@ 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());
       }
@@ -591,10 +688,18 @@ void GlobalHandles::IterateNewSpaceWeakIndependentRoots(ObjectVisitor* v) {
     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 {
+      } else if (node->weakness_type() == Node::NORMAL_WEAK) {
         v->VisitPointer(node->location());
+      } else {
+        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());
+        }
       }
     }
   }