[gjs/wip/ptomato/develop: 6/11] object: don't use toggle references unless necessary

[Philip Chimento <pchimento src gnome org>]

commit 57186b05debcfca3b882daf38224749e80dfd09d
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Sun Sep 17 15:09:47 2017 -0700

    object: don't use toggle references unless necessary
    
    Many GObjects (such as widgets and actors) don't have JS state, but
    they're kept alive by C code. We can therefore save some memory
    by GCing these objects, and creating new wrappers when needed.
    If state is ever set, we transparently switch to toggle refs, so
    no change should be visible at the JS level.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=679688

 gi/object.cpp |   76 ++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 56 insertions(+), 20 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index ea78b50..e47a076 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -73,6 +73,11 @@ struct ObjectInstance {
     std::deque<GjsCallbackTrampoline *> vfuncs;
 
     unsigned js_object_finalized : 1;
+
+    /* True if this object has visible JS state, and thus its lifecycle is
+     * managed using toggle references. False if this object just keeps a
+     * hard ref on the underlying GObject, and may be finalized at will. */
+    unsigned uses_toggle_ref : 1;
 };
 
 static std::stack<JS::PersistentRootedObject> object_init_list;
@@ -88,6 +93,7 @@ static std::set<ObjectInstance *> dissociate_list;
 GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class)
 
 static void            disassociate_js_gobject (GObject *gobj);
+static void ensure_uses_toggle_ref(JSContext *cx, ObjectInstance *priv);
 
 typedef enum {
     SOME_ERROR_OCCURRED = false,
@@ -438,6 +444,9 @@ set_g_param_from_prop(JSContext          *context,
     case SOME_ERROR_OCCURRED:
         return false;
     case NO_SUCH_G_PROPERTY:
+        /* We need to keep the wrapper alive in order not to lose custom
+         * "expando" properties */
+        ensure_uses_toggle_ref(context, priv);
         return result.succeed();
     case VALUE_WAS_SET:
     default:
@@ -499,19 +508,11 @@ object_instance_set_prop(JSContext              *context,
                          JS::MutableHandleValue  value_p,
                          JS::ObjectOpResult&     result)
 {
-    ObjectInstance *priv;
     GjsAutoJSChar name(context);
     bool ret = true;
     bool g_param_was_set = false;
 
-    if (!gjs_get_string_id(context, id, &name))
-        return result.succeed();  /* not resolved, but no error */
-
-    priv = priv_from_js(context, obj);
-    gjs_debug_jsprop(GJS_DEBUG_GOBJECT,
-                     "Set prop '%s' hook obj %p priv %p",
-                     name.get(), obj.get(), priv);
-
+    ObjectInstance *priv = priv_from_js(context, obj);
     if (priv == nullptr)
         /* see the comment in object_instance_get_prop() on this */
         return result.succeed();
@@ -519,6 +520,18 @@ object_instance_set_prop(JSContext              *context,
     if (priv->gobj == NULL) /* prototype, not an instance. */
         return result.succeed();
 
+    if (!gjs_get_string_id(context, id, &name)) {
+        /* We need to keep the wrapper alive in order not to lose custom
+         * "expando" properties. In this case if gjs_get_string_id() is false
+         * then a number or symbol property was probably set. */
+        ensure_uses_toggle_ref(context, priv);
+        return result.succeed();  /* not resolved, but no error */
+    }
+
+    gjs_debug_jsprop(GJS_DEBUG_GOBJECT,
+                     "Set prop '%s' hook obj %p priv %p",
+                     name.get(), obj.get(), priv);
+
     ret = set_g_param_from_prop(context, priv, name, g_param_was_set, value_p, result);
     if (g_param_was_set || !ret)
         return ret;
@@ -1116,7 +1129,10 @@ static void
 release_native_object (ObjectInstance *priv)
 {
     priv->keep_alive.reset();
-    g_object_remove_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, NULL);
+    if (priv->uses_toggle_ref)
+        g_object_remove_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, nullptr);
+    else
+        g_object_unref(priv->gobj);
     priv->gobj = NULL;
 }
 
@@ -1226,16 +1242,28 @@ associate_js_gobject (JSContext       *context,
     ObjectInstance *priv;
 
     priv = priv_from_js(context, object);
+    priv->uses_toggle_ref = false;
     priv->gobj = gobj;
 
     g_assert(!priv->keep_alive.rooted());
 
     set_object_qdata(gobj, priv);
 
+    priv->keep_alive = object;
     ensure_weak_pointer_callback(context);
     weak_pointer_list.insert(priv);
 
     g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, priv);
+}
+
+static void
+ensure_uses_toggle_ref(JSContext      *cx,
+                       ObjectInstance *priv)
+{
+    if (priv->uses_toggle_ref)
+        return;
+
+    g_assert(!priv->keep_alive.rooted());
 
     /* OK, here is where things get complicated. We want the
      * wrapped gobj to keep the JSObject* wrapper alive, because
@@ -1248,9 +1276,15 @@ associate_js_gobject (JSContext       *context,
      * the wrapper to be garbage collected (and thus unref the
      * wrappee).
      */
-    priv->keep_alive.root(context, object, gobj_no_longer_kept_alive_func, priv);
+    priv->uses_toggle_ref = true;
+    priv->keep_alive.switch_to_rooted(cx, gobj_no_longer_kept_alive_func, priv);
     dissociate_list_add(priv);
-    g_object_add_toggle_ref(gobj, wrapped_gobj_toggle_notify, NULL);
+    g_object_add_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, nullptr);
+
+    /* We now have both a ref and a toggle ref, we only want the toggle ref.
+     * This may immediately remove the GC root we just added, since refcount
+     * may drop to 1. */
+    g_object_unref(priv->gobj);
 }
 
 static void
@@ -1354,6 +1388,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS
          * we're not actually using it, so just let it get collected. Avoiding
          * this would require a non-trivial amount of work.
          * */
+        ensure_uses_toggle_ref(context, other_priv);
         object.set(other_priv->keep_alive);
         g_object_unref(gobj); /* We already own a reference */
         gobj = NULL;
@@ -1381,11 +1416,6 @@ G_GNUC_END_IGNORE_DEPRECATIONS
 
     if (priv->gobj == NULL)
         associate_js_gobject(context, object, gobj);
-    /* We now have both a ref and a toggle ref, we only want the
-     * toggle ref. This may immediately remove the GC root
-     * we just added, since refcount may drop to 1.
-     */
-    g_object_unref(gobj);
 
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                         "JSObject created with GObject %p %s",
@@ -1646,6 +1676,8 @@ real_connect_func(JSContext *context,
         return false;
     }
 
+    ensure_uses_toggle_ref(context, priv);
+
     if (argc != 2 || !argv[0].isString() || !JS::IsCallable(&argv[1].toObject())) {
         gjs_throw(context, "connect() takes two args, the signal name and the callback");
         return false;
@@ -2071,9 +2103,6 @@ gjs_object_from_g_object(JSContext    *context,
         g_object_ref_sink(gobj);
         associate_js_gobject(context, obj, gobj);
 
-        /* see the comment in init_object_instance() for this */
-        g_object_unref(gobj);
-
         g_assert(priv->keep_alive == obj.get());
     }
 
@@ -2608,6 +2637,10 @@ gjs_object_custom_init(GTypeInstance *instance,
 
     associate_js_gobject(context, object, G_OBJECT (instance));
 
+    /* Custom JS objects will most likely have visible state, so
+    * just do this from the start */
+    ensure_uses_toggle_ref(context, priv);
+
     JS::RootedValue v(context);
     if (!gjs_object_get_property(context, object,
                                  GJS_STRING_INSTANCE_INIT, &v)) {
@@ -3070,6 +3103,9 @@ gjs_object_associate_closure(JSContext       *cx,
     if (!priv)
         return false;
 
+    if (priv->gobj)
+        ensure_uses_toggle_ref(cx, priv);
+
     do_associate_closure(priv, closure);
     return true;
 }