From 4e1039ea76e7079aa9c121cc94248364b1007064 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 5 Apr 2025 00:22:59 +0200 Subject: [PATCH 1/3] gobject: rework g_object_weak_unref() to use _g_datalist_id_update_atomic() This is a step step to drop OPTIONAL_BIT_LOCK_WEAK_REFS lock. See the next commits why that is done. Also, free the WeakRefStack, if there are no more references. Previously, it was never freed. --- gobject/gobject.c | 65 +++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index a5e88b424..0b483a116 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3721,6 +3721,45 @@ g_object_weak_ref (GObject *object, object_bit_unlock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); } +static gpointer +g_object_weak_unref_cb (gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + GWeakNotify notify = ((gpointer *) user_data)[0]; + gpointer notify_data = ((gpointer *) user_data)[1]; + WeakRefStack *wstack = *data; + gboolean found_one = FALSE; + guint i; + + if (wstack) + { + for (i = 0; i < wstack->n_weak_refs; i++) + { + if (wstack->weak_refs[i].notify != notify || + wstack->weak_refs[i].data != notify_data) + continue; + + wstack->n_weak_refs -= 1; + if (wstack->n_weak_refs == 0) + { + g_free (wstack); + *data = NULL; + } + else if (i != wstack->n_weak_refs) + wstack->weak_refs[i] = wstack->weak_refs[wstack->n_weak_refs]; + + found_one = TRUE; + break; + } + } + + if (!found_one) + g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, notify, notify_data); + + return NULL; +} + /** * g_object_weak_unref: (skip) * @object: #GObject to remove a weak reference from @@ -3734,33 +3773,15 @@ g_object_weak_unref (GObject *object, GWeakNotify notify, gpointer data) { - WeakRefStack *wstack; - gboolean found_one = FALSE; - g_return_if_fail (G_IS_OBJECT (object)); g_return_if_fail (notify != NULL); object_bit_lock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); - wstack = g_datalist_id_get_data (&object->qdata, quark_weak_notifies); - if (wstack) - { - guint i; - - for (i = 0; i < wstack->n_weak_refs; i++) - if (wstack->weak_refs[i].notify == notify && - wstack->weak_refs[i].data == data) - { - found_one = TRUE; - wstack->n_weak_refs -= 1; - if (i != wstack->n_weak_refs) - wstack->weak_refs[i] = wstack->weak_refs[wstack->n_weak_refs]; - - break; - } - } + _g_datalist_id_update_atomic (&object->qdata, + quark_weak_notifies, + g_object_weak_unref_cb, + ((gpointer[]){ notify, data })); object_bit_unlock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); - if (!found_one) - g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, notify, data); } /** From 5da7ed2bc9a89c380f0245b0537022dfea24437b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 5 Apr 2025 00:22:59 +0200 Subject: [PATCH 2/3] gobject: rework g_object_weak_ref() to use _g_datalist_id_update_atomic() This is a step step to drop OPTIONAL_BIT_LOCK_WEAK_REFS lock. See the next commits why that is done. --- gobject/gobject.c | 57 +++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 0b483a116..bc9f4d4f2 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3672,6 +3672,40 @@ weak_refs_notify (gpointer data) g_free (wstack); } +static gpointer +g_object_weak_ref_cb (gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + GObject *object = ((gpointer *) user_data)[0]; + GWeakNotify notify = ((gpointer *) user_data)[1]; + gpointer notify_data = ((gpointer *) user_data)[2]; + WeakRefStack *wstack = *data; + guint i; + + if (!wstack) + { + wstack = g_new (WeakRefStack, 1); + wstack->object = object; + wstack->n_weak_refs = 1; + i = 0; + + *destroy_notify = weak_refs_notify; + } + else + { + i = wstack->n_weak_refs++; + wstack = g_realloc (wstack, sizeof (*wstack) + sizeof (wstack->weak_refs[0]) * i); + } + + *data = wstack; + + wstack->weak_refs[i].notify = notify; + wstack->weak_refs[i].data = notify_data; + + return NULL; +} + /** * g_object_weak_ref: (skip) * @object: #GObject to reference weakly @@ -3694,30 +3728,15 @@ g_object_weak_ref (GObject *object, GWeakNotify notify, gpointer data) { - WeakRefStack *wstack; - guint i; - g_return_if_fail (G_IS_OBJECT (object)); g_return_if_fail (notify != NULL); g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1); object_bit_lock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); - wstack = g_datalist_id_remove_no_notify (&object->qdata, quark_weak_notifies); - if (wstack) - { - i = wstack->n_weak_refs++; - wstack = g_realloc (wstack, sizeof (*wstack) + sizeof (wstack->weak_refs[0]) * i); - } - else - { - wstack = g_renew (WeakRefStack, NULL, 1); - wstack->object = object; - wstack->n_weak_refs = 1; - i = 0; - } - wstack->weak_refs[i].notify = notify; - wstack->weak_refs[i].data = data; - g_datalist_id_set_data_full (&object->qdata, quark_weak_notifies, wstack, weak_refs_notify); + _g_datalist_id_update_atomic (&object->qdata, + quark_weak_notifies, + g_object_weak_ref_cb, + ((gpointer[]){ object, notify, data })); object_bit_unlock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); } From 93b5b8a05104e39f6a2334042d40f516a75f25f3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jan 2024 15:41:40 +0100 Subject: [PATCH 3/3] gobject: drop OPTIONAL_BIT_LOCK_WEAK_REFS bit lock With the previous changes, all accesses to the WeakRefStack go through g_datalist_id_update_atomic() and are guarded by the GData's bit lock. At this point, the OPTIONAL_BIT_LOCK_WEAK_REFS lock is unnecessary and can be dropped. A minor benefit is that g_object_weak_{ref,unref}() needs now one lock less. Also note that this rework fixes a potential race for weak refs. Note that we have two calls g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); that don't take OPTIONAL_BIT_LOCK_WEAK_REFS lock. One of these calls is right before finalize(). At that point, no other thread can hold a reference to the object to race and we are good. However, the other call is from g_object_real_dispose(). At that point, theoretically the object could have been resurrected and a pointer could have been passed to another thread. Calling then g_object_weak_ref()/g_object_weak_unref() will race. We would have required a OPTIONAL_BIT_LOCK_WEAK_REFS lock around those g_datalist_id_set_data(,,NULL) calls. Instead, this is now also fixed, because every update to the WeakRefStack happens while holding the GData lock. So if you call g_datalist_id_set_data(,,NULL), the WeakRefStack is removed from the GData (and later freed by weak_refs_notify() and can no longer concurrently updated by g_object_weak_{ref,unref}(). --- gobject/gobject.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index bc9f4d4f2..cd4769ec0 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -124,9 +124,8 @@ enum { * parallel as possible. The alternative would be to add individual locking * 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_NOTIFY 1 +#define OPTIONAL_BIT_LOCK_TOGGLE_REFS 2 #if SIZEOF_INT == 4 && GLIB_SIZEOF_VOID_P >= 8 #define HAVE_OPTIONAL_FLAGS_IN_GOBJECT 1 @@ -3732,12 +3731,10 @@ g_object_weak_ref (GObject *object, g_return_if_fail (notify != NULL); g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1); - object_bit_lock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); _g_datalist_id_update_atomic (&object->qdata, quark_weak_notifies, g_object_weak_ref_cb, ((gpointer[]){ object, notify, data })); - object_bit_unlock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); } static gpointer @@ -3795,12 +3792,10 @@ g_object_weak_unref (GObject *object, g_return_if_fail (G_IS_OBJECT (object)); g_return_if_fail (notify != NULL); - object_bit_lock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); _g_datalist_id_update_atomic (&object->qdata, quark_weak_notifies, g_object_weak_unref_cb, ((gpointer[]){ notify, data })); - object_bit_unlock (object, OPTIONAL_BIT_LOCK_WEAK_REFS); } /**