GNOME.org
 

[gjs] object: Queue object toggles and drain them in idle

commit 9f0e50f5d01314ca3b9182724a27d5278943fcf6
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Mar 8 23:48:09 2017 -0800

    object: Queue object toggles and drain them in idle
    
    Previously, object toggle notifications that happened on a non-owner
    thread would cause the toggles to happen in an idle function. That was
    not good if no main loop was running, because then the toggles would
    just queue up indefinitely, eventually causing a crash when cleaning up
    objects at context shutdown time.
    
    Instead of doing that, we make a threadsafe FIFO queue of toggles, and
    drain it in an idle function. This has just the same effect, except that
    we now also have the option of draining it outside of an idle function.
    So when preparing to shut down the context, we drain it synchronously.

 gi/object.cpp |  210 ++++++---------------------------------------------------
 gi/toggle.cpp |  147 ++++++++++++++++++++++++++++++++++++++++
 gi/toggle.h   |   88 ++++++++++++++++++++++++
 gjs-srcs.mk   |    2 +
 4 files changed, 258 insertions(+), 189 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 3ca662b..6d3ef97 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -41,6 +41,7 @@
 #include "function.h"
 #include "proxyutils.h"
 #include "param.h"
+#include "toggle.h"
 #include "value.h"
 #include "closure.h"
 #include "gjs_gi_trace.h"
@@ -79,24 +80,6 @@ typedef struct {
     GClosure *closure;
 } ConnectData;
 
-typedef enum
-{
-  TOGGLE_DOWN,
-  TOGGLE_UP,
-} ToggleDirection;
-
-typedef struct
-{
-    GObject         *gobj;
-    ToggleDirection  direction;
-    guint            needs_unref : 1;
-} ToggleRefNotifyOperation;
-
-enum {
-    PROP_0,
-    PROP_JS_HANDLED,
-};
-
 static std::stack<JS::PersistentRootedObject> object_init_list;
 static GHashTable *class_init_properties;
 
@@ -104,7 +87,7 @@ static bool weak_pointer_callback = false;
 static std::set<ObjectInstance *> weak_pointer_list;
 
 extern struct JSClass gjs_object_instance_class;
-static volatile gint pending_idle_toggles;
+
 static std::set<ObjectInstance *> dissociate_list;
 
 GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class)
@@ -147,26 +130,6 @@ gjs_object_priv_quark (void)
     return val;
 }
 
-static GQuark
-gjs_toggle_down_quark (void)
-{
-    static GQuark val = 0;
-    if (G_UNLIKELY (!val))
-        val = g_quark_from_static_string ("gjs::toggle-down-quark");
-
-    return val;
-}
-
-static GQuark
-gjs_toggle_up_quark (void)
-{
-    static GQuark val = 0;
-    if (G_UNLIKELY (!val))
-        val = g_quark_from_static_string ("gjs::toggle-up-quark");
-
-    return val;
-}
-
 /* Plain g_type_query fails and leaves @query uninitialized for
    dynamic types.
    See https://bugzilla.gnome.org/show_bug.cgi?id=687184 and
@@ -959,64 +922,6 @@ gobj_no_longer_kept_alive_func(JS::HandleObject obj,
     weak_pointer_list.erase(priv);
 }
 
-static GQuark
-get_qdata_key_for_toggle_direction(ToggleDirection direction)
-{
-    GQuark quark;
-
-    switch (direction) {
-        case TOGGLE_UP:
-            quark = gjs_toggle_up_quark();
-            break;
-        case TOGGLE_DOWN:
-            quark = gjs_toggle_down_quark();
-            break;
-        default:
-            g_assert_not_reached();
-    }
-
-    return quark;
-}
-
-static bool
-clear_toggle_idle_source(GObject          *gobj,
-                         ToggleDirection   direction)
-{
-    GQuark qdata_key;
-
-    qdata_key = get_qdata_key_for_toggle_direction(direction);
-
-    return g_object_steal_qdata(gobj, qdata_key) != NULL;
-}
-
-static bool
-toggle_idle_source_is_queued(GObject          *gobj,
-                             ToggleDirection   direction)
-{
-    GQuark qdata_key;
-
-    qdata_key = get_qdata_key_for_toggle_direction(direction);
-
-    return g_object_get_qdata(gobj, qdata_key) != NULL;
-}
-
-static bool
-cancel_toggle_idle(GObject         *gobj,
-                   ToggleDirection  direction)
-{
-    GQuark qdata_key;
-    GSource *source;
-
-    qdata_key = get_qdata_key_for_toggle_direction(direction);
-
-    source = (GSource*) g_object_steal_qdata(gobj, qdata_key);
-
-    if (source)
-        g_source_destroy(source);
-
-    return source != 0;
-}
-
 static void
 handle_toggle_down(GObject *gobj)
 {
@@ -1066,91 +971,20 @@ handle_toggle_up(GObject   *gobj)
     }
 }
 
-static gboolean
-idle_handle_toggle(gpointer data)
-{
-    ToggleRefNotifyOperation *operation = (ToggleRefNotifyOperation *) data;
-
-    if (!clear_toggle_idle_source(operation->gobj, operation->direction)) {
-        /* Already cleared, the JSObject is going away, abort mission */
-        goto out;
-    }
-
-    switch (operation->direction) {
-        case TOGGLE_UP:
-            handle_toggle_up(operation->gobj);
-            break;
-        case TOGGLE_DOWN:
-            handle_toggle_down(operation->gobj);
-            break;
-        default:
-            g_assert_not_reached();
-    }
-
-out:
-    return G_SOURCE_REMOVE;
-}
-
 static void
-toggle_ref_notify_operation_free(ToggleRefNotifyOperation *operation)
+toggle_handler(GObject               *gobj,
+               ToggleQueue::Direction direction)
 {
-    if (operation->needs_unref)
-        g_object_unref (operation->gobj);
-    g_slice_free(ToggleRefNotifyOperation, operation);
-    g_atomic_int_add(&pending_idle_toggles, -1);
-}
-
-static void
-queue_toggle_idle(GObject         *gobj,
-                  ToggleDirection  direction)
-{
-    ToggleRefNotifyOperation *operation;
-    GQuark qdata_key;
-    GSource *source;
-
-    operation = g_slice_new0(ToggleRefNotifyOperation);
-    operation->direction = direction;
-
     switch (direction) {
-        case TOGGLE_UP:
-            /* If we're toggling up we take a reference to the object now,
-             * so it won't toggle down before we process it. This ensures we
-             * only ever have at most two toggle notifications queued.
-             * (either only up, or down-up)
-             */
-            operation->gobj = (GObject*) g_object_ref(gobj);
-            operation->needs_unref = true;
+        case ToggleQueue::UP:
+            handle_toggle_up(gobj);
             break;
-        case TOGGLE_DOWN:
-            /* If we're toggling down, we don't need to take a reference since
-             * the associated JSObject already has one, and that JSObject won't
-             * get finalized until we've completed toggling (since it's rooted,
-             * until we unroot it when we dispatch the toggle down idle).
-             *
-             * Taking a reference now would be bad anyway, since it would force
-             * the object to toggle back up again.
-             */
-            operation->gobj = gobj;
+        case ToggleQueue::DOWN:
+            handle_toggle_down(gobj);
             break;
         default:
             g_assert_not_reached();
     }
-
-    qdata_key = get_qdata_key_for_toggle_direction(direction);
-
-    source = g_idle_source_new();
-    g_source_set_priority(source, G_PRIORITY_HIGH);
-    g_source_set_callback(source,
-                          idle_handle_toggle,
-                          operation,
-                          (GDestroyNotify) toggle_ref_notify_operation_free);
-
-    g_atomic_int_inc(&pending_idle_toggles);
-    g_object_set_qdata (gobj, qdata_key, source);
-    g_source_attach (source, NULL);
-
-    /* object qdata is piggy-backing off the main loop's ref of the source */
-    g_source_unref (source);
 }
 
 static void
@@ -1198,14 +1032,11 @@ wrapped_gobj_toggle_notify(gpointer      data,
      * visible to the refcounted C world), but because of weird
      * weak singletons like g_bus_get_sync() objects can see toggle-ups
      * from different threads too.
-     * We could lock the keep alive structure and avoid the idle maybe,
-     * but there aren't many peculiar objects like that and it's
-     * not a big deal.
      */
     is_main_thread = _gjs_context_get_is_owner_thread(context);
 
-    toggle_up_queued = toggle_idle_source_is_queued(gobj, TOGGLE_UP);
-    toggle_down_queued = toggle_idle_source_is_queued(gobj, TOGGLE_DOWN);
+    auto& toggle_queue = ToggleQueue::get_default();
+    std::tie(toggle_down_queued, toggle_up_queued) = toggle_queue.is_queued(gobj);
 
     if (is_last_ref) {
         /* We've transitions from 2 -> 1 references,
@@ -1222,7 +1053,7 @@ wrapped_gobj_toggle_notify(gpointer      data,
 
             handle_toggle_down(gobj);
         } else {
-            queue_toggle_idle(gobj, TOGGLE_DOWN);
+            toggle_queue.enqueue(gobj, ToggleQueue::DOWN, toggle_handler);
         }
     } else {
         /* We've transitioned from 1 -> 2 references.
@@ -1237,7 +1068,7 @@ wrapped_gobj_toggle_notify(gpointer      data,
             }
             handle_toggle_up(gobj);
         } else {
-            queue_toggle_idle(gobj, TOGGLE_UP);
+            toggle_queue.enqueue(gobj, ToggleQueue::UP, toggle_handler);
         }
     }
 }
@@ -1256,10 +1087,9 @@ release_native_object (ObjectInstance *priv)
 void
 gjs_object_clear_toggles(void)
 {
-    while (g_main_context_pending(NULL) &&
-           g_atomic_int_get(&pending_idle_toggles) > 0) {
-        g_main_context_iteration(NULL, false);
-    }
+    auto& toggle_queue = ToggleQueue::get_default();
+    while (toggle_queue.handle_toggle(toggle_handler))
+        ;
 }
 
 void
@@ -1391,8 +1221,10 @@ disassociate_js_gobject(GObject *gobj)
      * assertion.
      * https://bugzilla.gnome.org/show_bug.cgi?id=778862
      */
-    if (cancel_toggle_idle(gobj, TOGGLE_UP) ||
-        cancel_toggle_idle(gobj, TOGGLE_DOWN))
+    auto& toggle_queue = ToggleQueue::get_default();
+    auto had_toggles = toggle_queue.cancel(gobj);
+    if (std::get<ToggleQueue::UP>(had_toggles) ||
+        std::get<ToggleQueue::DOWN>(had_toggles))
         g_critical("JS object wrapper for GObject %p (%s) is being released "
                    "while toggle references are still pending. This may happen "
                    "on exit in Gio.Application.vfunc_dbus_unregister(). If you "
@@ -1590,8 +1422,8 @@ object_instance_finalize(JSFreeOp  *fop,
                     priv->info ? g_base_info_get_name((GIBaseInfo*) priv->info) : g_type_name(priv->gtype));
         }
 
-        had_toggle_up = cancel_toggle_idle(priv->gobj, TOGGLE_UP);
-        had_toggle_down = cancel_toggle_idle(priv->gobj, TOGGLE_DOWN);
+        auto& toggle_queue = ToggleQueue::get_default();
+        std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(priv->gobj);
 
         if (!had_toggle_up && had_toggle_down) {
             g_error("Finalizing proxy for an object that's scheduled to be unrooted: %s.%s\n",
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
new file mode 100644
index 0000000..2bc6268
--- /dev/null
+++ b/gi/toggle.cpp
@@ -0,0 +1,147 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2017 Endless Mobile, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authored by: Philip Chimento <philip endlessm com>, <philip chimento gmail com>
+ */
+
+#include <algorithm>
+#include <deque>
+#include <mutex>
+#include <glib-object.h>
+
+#include "toggle.h"
+
+std::deque<ToggleQueue::Item>::iterator
+ToggleQueue::find_operation_locked(GObject               *gobj,
+                                   ToggleQueue::Direction direction)
+{
+    return std::find_if(q.begin(), q.end(),
+        [gobj, direction](const Item& item)->bool {
+            return item.gobj == gobj && item.direction == direction;
+        });
+}
+
+bool
+ToggleQueue::find_and_erase_operation_locked(GObject               *gobj,
+                                             ToggleQueue::Direction direction)
+{
+    auto pos = find_operation_locked(gobj, direction);
+    bool had_toggle = (pos != q.end());
+    if (had_toggle)
+        q.erase(pos);
+    return had_toggle;
+}
+
+gboolean
+ToggleQueue::idle_handle_toggle(void *data)
+{
+    auto self = static_cast<ToggleQueue *>(data);
+    while (self->handle_toggle(self->m_toggle_handler))
+        ;
+
+    return G_SOURCE_REMOVE;
+}
+
+void
+ToggleQueue::idle_destroy_notify(void *data)
+{
+    auto self = static_cast<ToggleQueue *>(data);
+    std::lock_guard<std::mutex> hold(self->lock);
+    self->m_idle_id = 0;
+    self->m_toggle_handler = nullptr;
+}
+
+std::pair<bool, bool>
+ToggleQueue::is_queued(GObject *gobj)
+{
+    std::lock_guard<std::mutex> hold(lock);
+    bool has_toggle_down = find_operation_locked(gobj, DOWN) != q.end();
+    bool has_toggle_up = find_operation_locked(gobj, UP) != q.end();
+    return {has_toggle_down, has_toggle_up};
+}
+
+std::pair<bool, bool>
+ToggleQueue::cancel(GObject *gobj)
+{
+    std::lock_guard<std::mutex> hold(lock);
+    bool had_toggle_down = find_and_erase_operation_locked(gobj, DOWN);
+    bool had_toggle_up = find_and_erase_operation_locked(gobj, UP);
+    return {had_toggle_down, had_toggle_up};
+}
+
+bool
+ToggleQueue::handle_toggle(Handler handler)
+{
+    Item item;
+    {
+        std::lock_guard<std::mutex> hold(lock);
+        if (q.empty())
+            return false;
+
+        item = q.front();
+        handler(item.gobj, item.direction);
+        q.pop_front();
+    }
+    
+    if (item.needs_unref)
+        g_object_unref(item.gobj);
+    
+    return true;
+}
+
+void
+ToggleQueue::enqueue(GObject               *gobj,
+                     ToggleQueue::Direction direction,
+                     ToggleQueue::Handler   handler)
+{
+    Item item{gobj, direction};
+    /* If we're toggling up we take a reference to the object now,
+     * so it won't toggle down before we process it. This ensures we
+     * only ever have at most two toggle notifications queued.
+     * (either only up, or down-up)
+     */
+    if (direction == UP) {
+        g_object_ref(gobj);
+        item.needs_unref = true;
+    }
+    /* If we're toggling down, we don't need to take a reference since
+     * the associated JSObject already has one, and that JSObject won't
+     * get finalized until we've completed toggling (since it's rooted,
+     * until we unroot it when we dispatch the toggle down idle).
+     *
+     * Taking a reference now would be bad anyway, since it would force
+     * the object to toggle back up again.
+     */   
+
+    std::lock_guard<std::mutex> hold(lock);
+    q.push_back(item);
+    
+    if (m_idle_id) {
+        g_assert(((void) "Should always enqueue with the same handler",
+                  m_toggle_handler == handler));
+        return;
+    }
+
+    m_toggle_handler = handler;
+    m_idle_id = g_idle_add_full(G_PRIORITY_HIGH, idle_handle_toggle, this,
+                                idle_destroy_notify);
+}
diff --git a/gi/toggle.h b/gi/toggle.h
new file mode 100644
index 0000000..990d5fa
--- /dev/null
+++ b/gi/toggle.h
@@ -0,0 +1,88 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2017 Endless Mobile, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authored by: Philip Chimento <philip endlessm com>, <philip chimento gmail com>
+ */
+
+#ifndef GJS_TOGGLE_H
+#define GJS_TOGGLE_H
+
+#include <deque>
+#include <mutex>
+#include <glib-object.h>
+
+/* Thread-safe queue for enqueueing toggle-up or toggle-down events on GObjects
+ * from any thread. For more information, see object.cpp, comments near
+ * wrapped_gobj_toggle_notify(). */
+class ToggleQueue {
+public:
+    enum Direction {
+        DOWN,
+        UP
+    };
+
+    typedef void (*Handler)(GObject *, Direction);
+
+private:
+    struct Item {
+        GObject *gobj;
+        ToggleQueue::Direction direction;
+        unsigned needs_unref : 1;
+    };
+
+    std::mutex lock;
+    std::deque<Item> q;
+    unsigned m_idle_id;
+    Handler m_toggle_handler;
+
+    std::deque<Item>::iterator find_operation_locked(GObject  *gobj,
+                                                     Direction direction);
+    bool find_and_erase_operation_locked(GObject *gobj, Direction direction);
+
+    static gboolean idle_handle_toggle(void *data);
+    static void idle_destroy_notify(void *data);
+
+public:
+    /* These two functions return a pair DOWN, UP signifying whether toggles
+     * are / were queued. is_queued() just checks and does not modify. */
+    std::pair<bool, bool> is_queued(GObject *gobj);
+    /* Cancels pending toggles and returns whether any were queued. */
+    std::pair<bool, bool> cancel(GObject *gobj);
+
+    /* Pops a toggle from the queue and processes it. Call this if you don't
+     * want to wait for it to be processed in idle time. Returns false if queue
+     * is empty. */
+    bool handle_toggle(Handler handler);
+    
+    /* Queues a toggle to be processed in idle time. */
+    void enqueue(GObject  *gobj,
+                 Direction direction,
+                 Handler   handler);
+
+    static ToggleQueue&
+    get_default(void) {
+        static ToggleQueue the_singleton;
+        return the_singleton;
+    }
+};
+
+#endif  /* GJS_TOGGLE_H */
diff --git a/gjs-srcs.mk b/gjs-srcs.mk
index 2295bf8..0efad48 100644
--- a/gjs-srcs.mk
+++ b/gjs-srcs.mk
@@ -42,6 +42,8 @@ gjs_srcs =                            \
        gi/proxyutils.h                 \
        gi/repo.cpp                     \
        gi/repo.h                       \
+       gi/toggle.cpp                   \
+       gi/toggle.h                     \
        gi/union.cpp                    \
        gi/union.h                      \
        gi/value.cpp                    \
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]