From 465c11f94b1dd43e9614ff6d1b8ba0d96f08f1e2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jan 2024 11:38:40 +0100 Subject: [PATCH 1/7] gobject: rework g_object_notify_queue_freeze() to use g_datalist_id_update_atomic() A common pattern is to look whether a GData entry exists, and if it doesn't, add it. For that, we currently always must take a OPTIONAL_BIT_LOCK_NOTIFY lock. This can be avoided, because GData already uses an internal mutex. By using g_datalist_id_update_atomic(), we can perform all relevant operations while holding that mutex. Move functionality from g_object_notify_queue_freeze() inside g_datalist_id_update_atomic(). The goal will be to drop the OPTIONAL_BIT_LOCK_NOTIFY lock in a later commit. --- gobject/gobject.c | 63 ++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 7ec5902b6e..d78e718108 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -658,46 +658,61 @@ g_object_notify_queue_free (gpointer data) } static GObjectNotifyQueue * -g_object_notify_queue_create_queue_frozen (GObject *object) +g_object_notify_queue_new (void) { GObjectNotifyQueue *nqueue; - nqueue = g_new0 (GObjectNotifyQueue, 1); - + nqueue = g_new (GObjectNotifyQueue, 1); *nqueue = (GObjectNotifyQueue){ .freeze_count = 1, }; - g_datalist_id_set_data_full (&object->qdata, quark_notify_queue, - nqueue, g_object_notify_queue_free); - return nqueue; } -static GObjectNotifyQueue * -g_object_notify_queue_freeze (GObject *object) +static gpointer +g_object_notify_queue_freeze_cb (GQuark key_id, + gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) { - GObjectNotifyQueue *nqueue; + GObject *object = user_data; + GObjectNotifyQueue *nqueue = *data; - object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); - nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue); if (!nqueue) { - nqueue = g_object_notify_queue_create_queue_frozen (object); - goto out; + /* The nqueue doesn't exist yet. We create it, and freeze thus 1 time. */ + nqueue = g_object_notify_queue_new (); + *data = nqueue; + *destroy_notify = g_object_notify_queue_free; } - - if (nqueue->freeze_count >= 65535) - g_critical("Free queue for %s (%p) is larger than 65535," - " called g_object_freeze_notify() too often." - " Forgot to call g_object_thaw_notify() or infinite loop", - G_OBJECT_TYPE_NAME (object), object); else - nqueue->freeze_count++; + { + if (nqueue->freeze_count >= 65535) + { + g_critical ("Free queue for %s (%p) is larger than 65535," + " called g_object_freeze_notify() too often." + " Forgot to call g_object_thaw_notify() or infinite loop", + G_OBJECT_TYPE_NAME (object), object); + } + else + nqueue->freeze_count++; + } -out: - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); + return nqueue; +} + +static GObjectNotifyQueue * +g_object_notify_queue_freeze (GObject *object) +{ + GObjectNotifyQueue *nqueue; + object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); + nqueue = _g_datalist_id_update_atomic (&object->qdata, + quark_notify_queue, + g_object_notify_queue_freeze_cb, + object); + object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); return nqueue; } @@ -788,7 +803,9 @@ g_object_notify_queue_add (GObject *object, * Note that this freeze will be balanced at the end of object * initialization. */ - nqueue = g_object_notify_queue_create_queue_frozen (object); + nqueue = g_object_notify_queue_new (); + g_datalist_id_set_data_full (&object->qdata, quark_notify_queue, + nqueue, g_object_notify_queue_free); } } -- GitLab From c4efff53bbb054f8e62b42b5b48ab8aff0d2f282 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 28 Jan 2024 12:56:47 +0100 Subject: [PATCH 2/7] gobject: rework g_object_notify_queue_thaw() to use g_datalist_id_update_atomic() The goal is to drop OPTIONAL_BIT_LOCK_NOTIFY lock. This is one step. Move code inside g_datalist_id_update_atomic(). --- gobject/gobject.c | 99 ++++++++++++++++++++++++++++++----------------- 1 file changed, 64 insertions(+), 35 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index d78e718108..2fd0265a4b 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -716,60 +716,89 @@ g_object_notify_queue_freeze (GObject *object) return nqueue; } +static gpointer +g_object_notify_queue_thaw_cb (GQuark key_id, + gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + GObject *object = ((gpointer *) user_data)[0]; + GObjectNotifyQueue *nqueue0 = ((gpointer *) user_data)[1]; + GObjectNotifyQueue *nqueue = *data; + +#if G_ENABLE_DEBUG + g_assert (!nqueue0 || nqueue0 == nqueue); +#endif + (void) nqueue0; + + if (G_UNLIKELY (!nqueue || nqueue->freeze_count == 0)) + { + g_critical ("%s: property-changed notification for %s(%p) is not frozen", + G_STRFUNC, G_OBJECT_TYPE_NAME (object), object); + return NULL; + } + + nqueue->freeze_count--; + + if (nqueue->freeze_count > 0) + return NULL; + + *data = NULL; + return nqueue; +} + static void -g_object_notify_queue_thaw (GObject *object, +g_object_notify_queue_thaw (GObject *object, GObjectNotifyQueue *nqueue, gboolean take_ref) { - GParamSpec *pspecs_mem[16], **pspecs, **free_me = NULL; + GParamSpec *pspecs_stack[16]; + GParamSpec **pspecs_heap = NULL; + GParamSpec **pspecs = pspecs_stack; + guint n_pspecs; GSList *slist; - guint n_pspecs = 0; object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); + nqueue = _g_datalist_id_update_atomic (&object->qdata, + quark_notify_queue, + g_object_notify_queue_thaw_cb, + ((gpointer[]){ object, nqueue })); + + object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); + if (!nqueue) - { - /* Caller didn't look up the queue yet. Do it now. */ - nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue); - } + return; - /* Just make sure we never get into some nasty race condition */ - if (G_UNLIKELY (!nqueue || nqueue->freeze_count == 0)) - { - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - g_critical ("%s: property-changed notification for %s(%p) is not frozen", - G_STRFUNC, G_OBJECT_TYPE_NAME (object), object); - return; - } + if (nqueue->n_pspecs == 0) + goto out; - nqueue->freeze_count--; - if (nqueue->freeze_count) + if (nqueue->n_pspecs > G_N_ELEMENTS (pspecs_stack)) { - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - return; + pspecs_heap = g_new (GParamSpec *, nqueue->n_pspecs); + pspecs = pspecs_heap; } - pspecs = nqueue->n_pspecs > 16 ? free_me = g_new (GParamSpec*, nqueue->n_pspecs) : pspecs_mem; - + n_pspecs = 0; for (slist = nqueue->pspecs; slist; slist = slist->next) - { - pspecs[n_pspecs++] = slist->data; - } - g_datalist_id_set_data (&object->qdata, quark_notify_queue, NULL); + pspecs[n_pspecs++] = slist->data; +#if G_ENABLE_DEBUG + g_assert (n_pspecs == nqueue->n_pspecs); +#endif - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); + if (take_ref) + g_object_ref (object); - if (n_pspecs) - { - if (take_ref) - g_object_ref (object); + G_OBJECT_GET_CLASS (object)->dispatch_properties_changed (object, n_pspecs, pspecs); - G_OBJECT_GET_CLASS (object)->dispatch_properties_changed (object, n_pspecs, pspecs); + if (take_ref) + g_object_unref (object); - if (take_ref) - g_object_unref (object); - } - g_free (free_me); + if (pspecs_heap) + g_free (pspecs_heap); + +out: + g_object_notify_queue_free (nqueue); } static gboolean -- GitLab From 94c5282ec96ca6fa1fe8074d90784eebe05ac31f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 10:20:13 +0100 Subject: [PATCH 3/7] gobject: rework g_object_notify_queue_add() to use g_datalist_id_update_atomic() The goal is to drop OPTIONAL_BIT_LOCK_NOTIFY lock. This is one step. Move code inside g_datalist_id_update_atomic(). --- gobject/gobject.c | 94 +++++++++++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 2fd0265a4b..f6cbe08226 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -801,54 +801,84 @@ out: g_object_notify_queue_free (nqueue); } -static gboolean -g_object_notify_queue_add (GObject *object, - GObjectNotifyQueue *nqueue, - GParamSpec *pspec, - gboolean in_init) +typedef struct { - object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); + GObject *object; + GObjectNotifyQueue *nqueue; + GParamSpec *pspec; + gboolean in_init; +} NotifyQueueAddData; + +static gpointer +g_object_notify_queue_add_cb (GQuark key_id, + gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + NotifyQueueAddData *nqdata = user_data; + GObjectNotifyQueue *nqueue = *data; + +#if G_ENABLE_DEBUG + g_assert (!nqdata->nqueue || nqdata->nqueue == nqueue); +#endif if (!nqueue) { - /* We are called without an nqueue. Figure out whether a notification - * should be queued. */ - nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue); - - if (!nqueue) + if (!nqdata->in_init) { - if (!in_init) - { - /* We don't have a notify queue and are not in_init. The event - * is not to be queued. The caller will dispatch directly. */ - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - return FALSE; - } - - /* We are "in_init", but did not freeze the queue in g_object_init - * yet. Instead, we gained a notify handler in instance init, so now - * we need to freeze just-in-time. - * - * Note that this freeze will be balanced at the end of object - * initialization. - */ - nqueue = g_object_notify_queue_new (); - g_datalist_id_set_data_full (&object->qdata, quark_notify_queue, - nqueue, g_object_notify_queue_free); + /* We are not in-init and are currently not frozen. There is nothing + * to do. We return FALSE to the caller, which then will dispatch + * the event right away. */ + return GINT_TO_POINTER (FALSE); } + + /* If we are "in_init", we always want to create a queue now. + * + * Note in that case, the freeze will be balanced at the end of object + * initialization. + * + * We only ensure that a nqueue exists. If it doesn't exist, we create + * it (and freeze once). If it already exists (and is frozen), we don't + * freeze an additional time. */ + nqueue = g_object_notify_queue_new (); + *data = nqueue; + *destroy_notify = g_object_notify_queue_free; } g_assert (nqueue->n_pspecs < 65535); - if (g_slist_find (nqueue->pspecs, pspec) == NULL) + if (!g_slist_find (nqueue->pspecs, nqdata->pspec)) { - nqueue->pspecs = g_slist_prepend (nqueue->pspecs, pspec); + nqueue->pspecs = g_slist_prepend (nqueue->pspecs, nqdata->pspec); nqueue->n_pspecs++; } + return GINT_TO_POINTER (TRUE); +} + +static gboolean +g_object_notify_queue_add (GObject *object, + GObjectNotifyQueue *nqueue, + GParamSpec *pspec, + gboolean in_init) +{ + gpointer result; + + object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); + + result = _g_datalist_id_update_atomic (&object->qdata, + quark_notify_queue, + g_object_notify_queue_add_cb, + &((NotifyQueueAddData){ + .object = object, + .nqueue = nqueue, + .pspec = pspec, + .in_init = in_init, + })); + object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - return TRUE; + return GPOINTER_TO_INT (result); } #ifdef G_ENABLE_DEBUG -- GitLab From 97dc5f42e29ae098387c5bb0617a34a3896b5970 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 10:28:26 +0100 Subject: [PATCH 4/7] gobject: drop OPTIONAL_BIT_LOCK_NOTIFY lock Now all accesses to quark_notify_queue are guarded by the GData lock. Several non-trivial operations are implemented via g_datalist_id_update_atomic(). The OPTIONAL_BIT_LOCK_NOTIFY lock is thus unnecessary and can be dropped. Note that with the move to g_datalist_id_update_atomic(), we now potentially do more work while holding the GData lock (e.g. some code paths allocation additional memory). But note that g_datalist_id_set_data() already has code paths where it must allocate memory to track the GDataElt. Also, most objects are not used in parallel, so holding the per-object (per-GData) lock longer does not affect them. Also, many operations also require a object_bit_lock(), so it's unlikely that you really could achieve higher parallelism by taking more locks (and minimizing the time to hold the GData lock). --- gobject/gobject.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index f6cbe08226..bf90534ad6 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -125,9 +125,8 @@ enum { * integers to GObjectPrivate. But increasing memory usage for more parallelism * (per-object!) is not worth it. */ #define OPTIONAL_BIT_LOCK_WEAK_REFS 1 -#define OPTIONAL_BIT_LOCK_NOTIFY 2 -#define OPTIONAL_BIT_LOCK_TOGGLE_REFS 3 -#define OPTIONAL_BIT_LOCK_CLOSURE_ARRAY 4 +#define OPTIONAL_BIT_LOCK_TOGGLE_REFS 2 +#define OPTIONAL_BIT_LOCK_CLOSURE_ARRAY 3 #if SIZEOF_INT == 4 && GLIB_SIZEOF_VOID_P >= 8 #define HAVE_OPTIONAL_FLAGS_IN_GOBJECT 1 @@ -705,15 +704,10 @@ g_object_notify_queue_freeze_cb (GQuark key_id, static GObjectNotifyQueue * g_object_notify_queue_freeze (GObject *object) { - GObjectNotifyQueue *nqueue; - - object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); - nqueue = _g_datalist_id_update_atomic (&object->qdata, - quark_notify_queue, - g_object_notify_queue_freeze_cb, - object); - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - return nqueue; + return _g_datalist_id_update_atomic (&object->qdata, + quark_notify_queue, + g_object_notify_queue_freeze_cb, + object); } static gpointer @@ -758,15 +752,11 @@ g_object_notify_queue_thaw (GObject *object, guint n_pspecs; GSList *slist; - object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); - nqueue = _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_thaw_cb, ((gpointer[]){ object, nqueue })); - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - if (!nqueue) return; @@ -864,8 +854,6 @@ g_object_notify_queue_add (GObject *object, { gpointer result; - object_bit_lock (object, OPTIONAL_BIT_LOCK_NOTIFY); - result = _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_add_cb, @@ -876,8 +864,6 @@ g_object_notify_queue_add (GObject *object, .in_init = in_init, })); - object_bit_unlock (object, OPTIONAL_BIT_LOCK_NOTIFY); - return GPOINTER_TO_INT (result); } -- GitLab From e767cb91c23ef241b296e42e702cf62aff3d97f3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 10:55:41 +0100 Subject: [PATCH 5/7] gobject: rework GObjectNotifyQueue to not use GSList GSList is almost in all use cases a bad choice. It's bad for locality and requires a heap allocation per entry. Instead, use an array, and grow the buffer exponentially via realloc(). Now, that we use g_datalist_id_update_atomic(), it is also easy to update the pointer. Hence, the GObjectNotifyQueue struct does not point to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets reallocated, thus saving one heap allocation for the separate head structure. --- gobject/gobject.c | 145 ++++++++++++++++++++-------------------------- 1 file changed, 63 insertions(+), 82 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index bf90534ad6..542b8d5433 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -213,14 +213,14 @@ static void object_interface_check_properties (gpointer check_d gpointer g_iface); /* --- typedefs --- */ -typedef struct _GObjectNotifyQueue GObjectNotifyQueue; -struct _GObjectNotifyQueue +typedef struct { - GSList *pspecs; - guint16 n_pspecs; - guint16 freeze_count; -}; + guint16 freeze_count; + guint16 len; + guint16 alloc; + GParamSpec *pspecs[1]; +} GObjectNotifyQueue; /* --- variables --- */ static GQuark quark_closure_array = 0; @@ -647,24 +647,17 @@ object_bit_unlock (GObject *object, guint lock_bit) } /* --- functions --- */ -static void -g_object_notify_queue_free (gpointer data) -{ - GObjectNotifyQueue *nqueue = data; - - g_slist_free (nqueue->pspecs); - g_free_sized (nqueue, sizeof (GObjectNotifyQueue)); -} static GObjectNotifyQueue * g_object_notify_queue_new (void) { GObjectNotifyQueue *nqueue; - nqueue = g_new (GObjectNotifyQueue, 1); - *nqueue = (GObjectNotifyQueue){ - .freeze_count = 1, - }; + nqueue = g_malloc (G_STRUCT_OFFSET (GObjectNotifyQueue, pspecs) + (4 * sizeof (GParamSpec *))); + + nqueue->freeze_count = 1; + nqueue->alloc = 4; + nqueue->len = 0; return nqueue; } @@ -683,11 +676,11 @@ g_object_notify_queue_freeze_cb (GQuark key_id, /* The nqueue doesn't exist yet. We create it, and freeze thus 1 time. */ nqueue = g_object_notify_queue_new (); *data = nqueue; - *destroy_notify = g_object_notify_queue_free; + *destroy_notify = g_free; } else { - if (nqueue->freeze_count >= 65535) + if (nqueue->freeze_count == G_MAXUINT16) { g_critical ("Free queue for %s (%p) is larger than 65535," " called g_object_freeze_notify() too often." @@ -716,15 +709,9 @@ g_object_notify_queue_thaw_cb (GQuark key_id, GDestroyNotify *destroy_notify, gpointer user_data) { - GObject *object = ((gpointer *) user_data)[0]; - GObjectNotifyQueue *nqueue0 = ((gpointer *) user_data)[1]; + GObject *object = user_data; GObjectNotifyQueue *nqueue = *data; -#if G_ENABLE_DEBUG - g_assert (!nqueue0 || nqueue0 == nqueue); -#endif - (void) nqueue0; - if (G_UNLIKELY (!nqueue || nqueue->freeze_count == 0)) { g_critical ("%s: property-changed notification for %s(%p) is not frozen", @@ -746,75 +733,55 @@ g_object_notify_queue_thaw (GObject *object, GObjectNotifyQueue *nqueue, gboolean take_ref) { - GParamSpec *pspecs_stack[16]; - GParamSpec **pspecs_heap = NULL; - GParamSpec **pspecs = pspecs_stack; - guint n_pspecs; - GSList *slist; - nqueue = _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_thaw_cb, - ((gpointer[]){ object, nqueue })); + object); if (!nqueue) return; - if (nqueue->n_pspecs == 0) - goto out; - - if (nqueue->n_pspecs > G_N_ELEMENTS (pspecs_stack)) + if (nqueue->len > 0) { - pspecs_heap = g_new (GParamSpec *, nqueue->n_pspecs); - pspecs = pspecs_heap; - } + guint16 i; + guint16 j; - n_pspecs = 0; - for (slist = nqueue->pspecs; slist; slist = slist->next) - pspecs[n_pspecs++] = slist->data; -#if G_ENABLE_DEBUG - g_assert (n_pspecs == nqueue->n_pspecs); -#endif + /* Reverse the list. This is the order that we historically had. */ + for (i = 0, j = nqueue->len - 1u; i < j; i++, j--) + { + GParamSpec *tmp; - if (take_ref) - g_object_ref (object); + tmp = nqueue->pspecs[i]; + nqueue->pspecs[i] = nqueue->pspecs[j]; + nqueue->pspecs[j] = tmp; + } - G_OBJECT_GET_CLASS (object)->dispatch_properties_changed (object, n_pspecs, pspecs); + if (take_ref) + g_object_ref (object); - if (take_ref) - g_object_unref (object); + G_OBJECT_GET_CLASS (object)->dispatch_properties_changed (object, nqueue->len, nqueue->pspecs); - if (pspecs_heap) - g_free (pspecs_heap); + if (take_ref) + g_object_unref (object); + } -out: - g_object_notify_queue_free (nqueue); + g_free (nqueue); } -typedef struct -{ - GObject *object; - GObjectNotifyQueue *nqueue; - GParamSpec *pspec; - gboolean in_init; -} NotifyQueueAddData; - static gpointer g_object_notify_queue_add_cb (GQuark key_id, gpointer *data, GDestroyNotify *destroy_notify, gpointer user_data) { - NotifyQueueAddData *nqdata = user_data; + GParamSpec *pspec = ((gpointer *) user_data)[0]; + gboolean in_init = GPOINTER_TO_INT (((gpointer *) user_data)[1]); GObjectNotifyQueue *nqueue = *data; - -#if G_ENABLE_DEBUG - g_assert (!nqdata->nqueue || nqdata->nqueue == nqueue); -#endif + guint16 i; if (!nqueue) { - if (!nqdata->in_init) + if (!in_init) { /* We are not in-init and are currently not frozen. There is nothing * to do. We return FALSE to the caller, which then will dispatch @@ -832,17 +799,36 @@ g_object_notify_queue_add_cb (GQuark key_id, * freeze an additional time. */ nqueue = g_object_notify_queue_new (); *data = nqueue; - *destroy_notify = g_object_notify_queue_free; + *destroy_notify = g_free; } + else + { + for (i = 0; i < nqueue->len; i++) + { + if (nqueue->pspecs[i] == pspec) + goto out; + } - g_assert (nqueue->n_pspecs < 65535); + if (G_UNLIKELY (nqueue->len == nqueue->alloc)) + { + guint32 alloc; - if (!g_slist_find (nqueue->pspecs, nqdata->pspec)) - { - nqueue->pspecs = g_slist_prepend (nqueue->pspecs, nqdata->pspec); - nqueue->n_pspecs++; + alloc = ((guint32) nqueue->alloc) * 2u; + if (alloc >= G_MAXUINT16) + { + g_assert (nqueue->len < G_MAXUINT16); + alloc = G_MAXUINT16; + } + nqueue = g_realloc (nqueue, G_STRUCT_OFFSET (GObjectNotifyQueue, pspecs) + (alloc * sizeof (GParamSpec *))); + nqueue->alloc = alloc; + + *data = nqueue; + } } + nqueue->pspecs[nqueue->len++] = pspec; + +out: return GINT_TO_POINTER (TRUE); } @@ -857,12 +843,7 @@ g_object_notify_queue_add (GObject *object, result = _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_add_cb, - &((NotifyQueueAddData){ - .object = object, - .nqueue = nqueue, - .pspec = pspec, - .in_init = in_init, - })); + ((gpointer[]){ pspec, GINT_TO_POINTER (!!in_init) })); return GPOINTER_TO_INT (result); } -- GitLab From b667a6887d2269731446649619963b46a6a9ce9a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 11:13:13 +0100 Subject: [PATCH 6/7] gobject: don't pass around the GObjectNotifyQueue instance By now, GObjectNotifyQueue gets reallocated. So quite possibly if we keep the queue, it is a dangling pointer. That is error prone, but it's also unnecessary. All we need to know is whether we bumped the freeze count and need to unfreeze. The queue itself was not useful, because we anyway must take a lock (via g_datalist_id_update_atomic()) to do anything with it. Instead, use a nqueue_is_frozen boolean variable. --- gobject/gobject.c | 104 ++++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 542b8d5433..05246be218 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -691,16 +691,16 @@ g_object_notify_queue_freeze_cb (GQuark key_id, nqueue->freeze_count++; } - return nqueue; + return NULL; } -static GObjectNotifyQueue * +static void g_object_notify_queue_freeze (GObject *object) { - return _g_datalist_id_update_atomic (&object->qdata, - quark_notify_queue, - g_object_notify_queue_freeze_cb, - object); + _g_datalist_id_update_atomic (&object->qdata, + quark_notify_queue, + g_object_notify_queue_freeze_cb, + object); } static gpointer @@ -729,10 +729,10 @@ g_object_notify_queue_thaw_cb (GQuark key_id, } static void -g_object_notify_queue_thaw (GObject *object, - GObjectNotifyQueue *nqueue, - gboolean take_ref) +g_object_notify_queue_thaw (GObject *object, gboolean take_ref) { + GObjectNotifyQueue *nqueue; + nqueue = _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_thaw_cb, @@ -834,7 +834,6 @@ out: static gboolean g_object_notify_queue_add (GObject *object, - GObjectNotifyQueue *nqueue, GParamSpec *pspec, gboolean in_init) { @@ -1947,7 +1946,7 @@ g_object_notify_by_spec_internal (GObject *object, if (pspec != NULL && needs_notify) { - if (!g_object_notify_queue_add (object, NULL, pspec, in_init)) + if (!g_object_notify_queue_add (object, pspec, in_init)) { /* * Coverity doesn’t understand the paired ref/unref here and seems to @@ -2100,7 +2099,7 @@ g_object_thaw_notify (GObject *object) } #endif - g_object_notify_queue_thaw (object, NULL, TRUE); + g_object_notify_queue_thaw (object, TRUE); } static void @@ -2184,7 +2183,7 @@ static inline void object_set_property (GObject *object, GParamSpec *pspec, const GValue *value, - GObjectNotifyQueue *nqueue, + gboolean nqueue_is_frozen, gboolean user_specified) { GTypeInstance *inst = (GTypeInstance *) object; @@ -2243,8 +2242,8 @@ object_set_property (GObject *object, } if ((pspec->flags & (G_PARAM_EXPLICIT_NOTIFY | G_PARAM_READABLE)) == G_PARAM_READABLE && - nqueue != NULL) - g_object_notify_queue_add (object, nqueue, pspec, FALSE); + nqueue_is_frozen) + g_object_notify_queue_add (object, pspec, FALSE); } static void @@ -2486,7 +2485,7 @@ g_object_new_with_custom_constructor (GObjectClass *class, GObjectConstructParam *params, guint n_params) { - GObjectNotifyQueue *nqueue = NULL; + gboolean nqueue_is_frozen = FALSE; gboolean newly_constructed; GObjectConstructParam *cparams; gboolean free_cparams = FALSE; @@ -2609,9 +2608,9 @@ g_object_new_with_custom_constructor (GObjectClass *class, /* This may or may not have been setup in g_object_init(). * If it hasn't, we do it now. */ - nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue); - if (!nqueue) - nqueue = g_object_notify_queue_freeze (object); + if (!g_datalist_id_get_data (&object->qdata, quark_notify_queue)) + g_object_notify_queue_freeze (object); + nqueue_is_frozen = TRUE; } } @@ -2622,11 +2621,10 @@ g_object_new_with_custom_constructor (GObjectClass *class, /* set remaining properties */ for (i = 0; i < n_params; i++) if (!(params[i].pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY))) - object_set_property (object, params[i].pspec, params[i].value, nqueue, TRUE); + object_set_property (object, params[i].pspec, params[i].value, nqueue_is_frozen, TRUE); - /* If nqueue is non-NULL then we are frozen. Thaw it. */ - if (nqueue) - g_object_notify_queue_thaw (object, nqueue, FALSE); + if (nqueue_is_frozen) + g_object_notify_queue_thaw (object, FALSE); return object; } @@ -2636,7 +2634,7 @@ g_object_new_internal (GObjectClass *class, GObjectConstructParam *params, guint n_params) { - GObjectNotifyQueue *nqueue = NULL; + gboolean nqueue_is_frozen = FALSE; GObject *object; guint i; @@ -2658,9 +2656,9 @@ g_object_new_internal (GObjectClass *class, /* This may or may not have been setup in g_object_init(). * If it hasn't, we do it now. */ - nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue); - if (!nqueue) - nqueue = g_object_notify_queue_freeze (object); + if (!g_datalist_id_get_data (&object->qdata, quark_notify_queue)) + g_object_notify_queue_freeze (object); + nqueue_is_frozen = TRUE; } /* We will set exactly n_construct_properties construct @@ -2688,7 +2686,7 @@ g_object_new_internal (GObjectClass *class, if (value == NULL) value = g_param_spec_get_default_value (pspec); - object_set_property (object, pspec, value, nqueue, user_specified); + object_set_property (object, pspec, value, nqueue_is_frozen, user_specified); } } @@ -2701,10 +2699,10 @@ g_object_new_internal (GObjectClass *class, */ for (i = 0; i < n_params; i++) if (!(params[i].pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY))) - object_set_property (object, params[i].pspec, params[i].value, nqueue, TRUE); + object_set_property (object, params[i].pspec, params[i].value, nqueue_is_frozen, TRUE); - if (nqueue) - g_object_notify_queue_thaw (object, nqueue, FALSE); + if (nqueue_is_frozen) + g_object_notify_queue_thaw (object, FALSE); return object; } @@ -3023,7 +3021,7 @@ g_object_constructor (GType type, /* set construction parameters */ if (n_construct_properties) { - GObjectNotifyQueue *nqueue = g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object); /* set construct properties */ while (n_construct_properties--) @@ -3032,9 +3030,10 @@ g_object_constructor (GType type, GParamSpec *pspec = construct_params->pspec; construct_params++; - object_set_property (object, pspec, value, nqueue, TRUE); + object_set_property (object, pspec, value, TRUE, TRUE); } - g_object_notify_queue_thaw (object, nqueue, FALSE); + + g_object_notify_queue_thaw (object, FALSE); /* the notification queue is still frozen from g_object_init(), so * we don't need to handle it here, g_object_newv() takes * care of that @@ -3097,7 +3096,7 @@ g_object_setv (GObject *object, const GValue values[]) { guint i; - GObjectNotifyQueue *nqueue = NULL; + gboolean nqueue_is_frozen = FALSE; GParamSpec *pspec; GObjectClass *class; @@ -3111,7 +3110,10 @@ g_object_setv (GObject *object, class = G_OBJECT_GET_CLASS (object); if (_g_object_has_notify_handler (object)) - nqueue = g_object_notify_queue_freeze (object); + { + g_object_notify_queue_freeze (object); + nqueue_is_frozen = TRUE; + } for (i = 0; i < n_properties; i++) { @@ -3120,11 +3122,11 @@ g_object_setv (GObject *object, if (!g_object_set_is_valid_property (object, pspec, names[i])) break; - object_set_property (object, pspec, &values[i], nqueue, TRUE); + object_set_property (object, pspec, &values[i], nqueue_is_frozen, TRUE); } - if (nqueue) - g_object_notify_queue_thaw (object, nqueue, FALSE); + if (nqueue_is_frozen) + g_object_notify_queue_thaw (object, FALSE); g_object_unref (object); } @@ -3143,7 +3145,7 @@ g_object_set_valist (GObject *object, const gchar *first_property_name, va_list var_args) { - GObjectNotifyQueue *nqueue = NULL; + gboolean nqueue_is_frozen = FALSE; const gchar *name; GObjectClass *class; @@ -3152,7 +3154,10 @@ g_object_set_valist (GObject *object, g_object_ref (object); if (_g_object_has_notify_handler (object)) - nqueue = g_object_notify_queue_freeze (object); + { + g_object_notify_queue_freeze (object); + nqueue_is_frozen = TRUE; + } class = G_OBJECT_GET_CLASS (object); @@ -3178,7 +3183,7 @@ g_object_set_valist (GObject *object, break; } - object_set_property (object, pspec, &value, nqueue, TRUE); + object_set_property (object, pspec, &value, nqueue_is_frozen, TRUE); /* We open-code g_value_unset() here to avoid the * cost of looking up the GTypeValueTable again. @@ -3189,8 +3194,8 @@ g_object_set_valist (GObject *object, name = va_arg (var_args, gchar*); } - if (nqueue) - g_object_notify_queue_thaw (object, nqueue, FALSE); + if (nqueue_is_frozen) + g_object_notify_queue_thaw (object, FALSE); g_object_unref (object); } @@ -4323,7 +4328,7 @@ g_object_unref (gpointer _object) gint old_ref; GToggleNotify toggle_notify; gpointer toggle_data; - GObjectNotifyQueue *nqueue; + gboolean nqueue_is_frozen; gboolean do_retry; GType obj_gtype; @@ -4418,7 +4423,8 @@ retry_beginning: * notification queue gets automatically drained when g_object_finalize() is * reached and the qdata is cleared. */ - nqueue = g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object); + nqueue_is_frozen = TRUE; TRACE (GOBJECT_OBJECT_DISPOSE (object, G_TYPE_FROM_INSTANCE (object), 1)); G_OBJECT_GET_CLASS (object)->dispose (object); @@ -4432,12 +4438,12 @@ retry_decrement: /* Here, old_ref is 1 if we just come from dispose(). If the object was resurrected, * we can hit `goto retry_decrement` and be here with a larger old_ref. */ - if (old_ref > 1 && nqueue) + if (old_ref > 1 && nqueue_is_frozen) { /* If the object was resurrected, we need to unfreeze the notify * queue. */ - g_object_notify_queue_thaw (object, nqueue, FALSE); - nqueue = NULL; + g_object_notify_queue_thaw (object, FALSE); + nqueue_is_frozen = FALSE; /* Note at this point, @old_ref might be wrong. * -- GitLab From 4e1bde652d52c08b374c863dde5de68a5d860cee Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 11:30:21 +0100 Subject: [PATCH 7/7] gobject: rework freezing once during object initialization During object initialization, we may want to freeze the notifications, but only do so once (and once unfreeze at the end). Rework how that was done. We can avoid an additional GData lookup. --- gobject/gobject.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 05246be218..5b1b108d3d 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -668,7 +668,8 @@ g_object_notify_queue_freeze_cb (GQuark key_id, GDestroyNotify *destroy_notify, gpointer user_data) { - GObject *object = user_data; + GObject *object = ((gpointer *) user_data)[0]; + gboolean freeze_always = GPOINTER_TO_INT (((gpointer *) user_data)[1]); GObjectNotifyQueue *nqueue = *data; if (!nqueue) @@ -678,6 +679,13 @@ g_object_notify_queue_freeze_cb (GQuark key_id, *data = nqueue; *destroy_notify = g_free; } + else if (!freeze_always) + { + /* The caller only wants to ensure we are frozen once. If we are already frozen, + * don't freeze another time. + * + * This is only relevant during the object initialization. */ + } else { if (nqueue->freeze_count == G_MAXUINT16) @@ -695,12 +703,12 @@ g_object_notify_queue_freeze_cb (GQuark key_id, } static void -g_object_notify_queue_freeze (GObject *object) +g_object_notify_queue_freeze (GObject *object, gboolean freeze_always) { _g_datalist_id_update_atomic (&object->qdata, quark_notify_queue, g_object_notify_queue_freeze_cb, - object); + ((gpointer[]){ object, GINT_TO_POINTER (!!freeze_always) })); } static gpointer @@ -1742,7 +1750,7 @@ g_object_init (GObject *object, if (CLASS_HAS_PROPS (class) && CLASS_NEEDS_NOTIFY (class)) { /* freeze object's notification queue, g_object_new_internal() preserves pairedness */ - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); } /* mark object in-construction for notify_queue_thaw() and to allow construct-only properties */ @@ -1922,7 +1930,7 @@ g_object_freeze_notify (GObject *object) } #endif - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); } static inline void @@ -2608,8 +2616,7 @@ g_object_new_with_custom_constructor (GObjectClass *class, /* This may or may not have been setup in g_object_init(). * If it hasn't, we do it now. */ - if (!g_datalist_id_get_data (&object->qdata, quark_notify_queue)) - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, FALSE); nqueue_is_frozen = TRUE; } } @@ -2656,8 +2663,7 @@ g_object_new_internal (GObjectClass *class, /* This may or may not have been setup in g_object_init(). * If it hasn't, we do it now. */ - if (!g_datalist_id_get_data (&object->qdata, quark_notify_queue)) - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, FALSE); nqueue_is_frozen = TRUE; } @@ -3021,7 +3027,7 @@ g_object_constructor (GType type, /* set construction parameters */ if (n_construct_properties) { - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); /* set construct properties */ while (n_construct_properties--) @@ -3111,7 +3117,7 @@ g_object_setv (GObject *object, if (_g_object_has_notify_handler (object)) { - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); nqueue_is_frozen = TRUE; } @@ -3155,7 +3161,7 @@ g_object_set_valist (GObject *object, if (_g_object_has_notify_handler (object)) { - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); nqueue_is_frozen = TRUE; } @@ -4423,7 +4429,7 @@ retry_beginning: * notification queue gets automatically drained when g_object_finalize() is * reached and the qdata is cleared. */ - g_object_notify_queue_freeze (object); + g_object_notify_queue_freeze (object, TRUE); nqueue_is_frozen = TRUE; TRACE (GOBJECT_OBJECT_DISPOSE (object, G_TYPE_FROM_INSTANCE (object), 1)); -- GitLab