From 7743c7aaa2699cbffaabb65c651a46546ccab469 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Apr 2025 10:59:53 +0200 Subject: [PATCH 1/6] gobject/tests: add test for g_object_weak_ref() --- gobject/tests/references.c | 68 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/gobject/tests/references.c b/gobject/tests/references.c index 2eb994cf4..c7f3891d4 100644 --- a/gobject/tests/references.c +++ b/gobject/tests/references.c @@ -282,6 +282,73 @@ test_references (void) g_assert_true (object_destroyed); } +/*****************************************************************************/ + +static guint weak_ref4_notified; +static guint weak_ref4_has_stable_order; + +static void +weak_ref4 (gpointer data, + GObject *object) +{ + static gint last = -1; + gint idx; + + if (weak_ref4_has_stable_order) + { + idx = (gint) GPOINTER_TO_UINT (data); + g_assert_cmpint (last, <, idx); + last = idx; + } + + weak_ref4_notified++; +} + +static void +test_references_many (void) +{ + GObject *object; + guint *indexes; + guint i; + guint n; + guint m; + + /* Test subscribing a (random) number of weak references. */ + object = g_object_new (TEST_TYPE_OBJECT, NULL); + n = g_random_int () % 1000; + indexes = g_new (guint, MAX (n, 1)); + for (i = 0; i < n; i++) + { + g_object_weak_ref (object, weak_ref4, GUINT_TO_POINTER (i)); + indexes[i] = i; + } + for (i = 0; i < n; i++) + { + guint tmp, j; + + j = (guint) g_random_int_range ((gint) i, (gint) n); + tmp = indexes[i]; + indexes[i] = indexes[j]; + indexes[j] = tmp; + } + if (g_random_boolean ()) + { + m = 0; + } + else + { + m = g_random_int () % (n + 1u); + for (i = 0; i < m; i++) + g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (indexes[i])); + } + weak_ref4_has_stable_order = (m == 0); + g_object_unref (object); + g_assert_cmpint (weak_ref4_notified, ==, n - m); + g_free (indexes); +} + +/*****************************************************************************/ + int main (int argc, char *argv[]) @@ -293,6 +360,7 @@ main (int argc, g_test_init (&argc, &argv, NULL); g_test_add_func ("/gobject/references", test_references); + g_test_add_func ("/gobject/references-many", test_references_many); return g_test_run (); } From d2e08b7dfe568498b07bf1960c0757be32ab6ca2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Apr 2025 07:52:34 +0200 Subject: [PATCH 2/6] gobject: preserve order of weak notifications in g_object_weak_unref() g_object_weak_unref() would have done a fast-removal of the entry, which messes up the order of the weak notifications. During destruction of the object we emit the weak notifications. They are emitted in the order in which they were registered (FIFO). Except, when a g_object_weak_unref() messes up the order. Avoid that and preserve the order. Now, do a memmove(), which is O(n). But note that we already track weak references in a flat array that requires a O(n) linear search. Thus, g_object_weak_unref() was already O(n) and that didn't change. More importantly, users are well advised to limit themselves to a reasonably small number of weak notifications. And for small n, the linear search and the memmove() is an efficient solution. --- gobject/gobject.c | 6 +++++- gobject/tests/references.c | 24 ++++++------------------ 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 3305abcf6..3a9ff87b5 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3827,7 +3827,11 @@ g_object_weak_unref_cb (gpointer *data, *data = NULL; } else if (i != wstack->n_weak_refs) - wstack->weak_refs[i] = wstack->weak_refs[wstack->n_weak_refs]; + { + memmove (&wstack->weak_refs[i], + &wstack->weak_refs[i + 1], + sizeof (wstack->weak_refs[i]) * (wstack->n_weak_refs - i)); + } found_one = TRUE; break; diff --git a/gobject/tests/references.c b/gobject/tests/references.c index c7f3891d4..ebd27f170 100644 --- a/gobject/tests/references.c +++ b/gobject/tests/references.c @@ -285,7 +285,6 @@ test_references (void) /*****************************************************************************/ static guint weak_ref4_notified; -static guint weak_ref4_has_stable_order; static void weak_ref4 (gpointer data, @@ -294,12 +293,9 @@ weak_ref4 (gpointer data, static gint last = -1; gint idx; - if (weak_ref4_has_stable_order) - { - idx = (gint) GPOINTER_TO_UINT (data); - g_assert_cmpint (last, <, idx); - last = idx; - } + idx = (gint) GPOINTER_TO_UINT (data); + g_assert_cmpint (last, <, idx); + last = idx; weak_ref4_notified++; } @@ -331,17 +327,9 @@ test_references_many (void) indexes[i] = indexes[j]; indexes[j] = tmp; } - if (g_random_boolean ()) - { - m = 0; - } - else - { - m = g_random_int () % (n + 1u); - for (i = 0; i < m; i++) - g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (indexes[i])); - } - weak_ref4_has_stable_order = (m == 0); + m = g_random_int () % (n + 1u); + for (i = 0; i < m; i++) + g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (indexes[i])); g_object_unref (object); g_assert_cmpint (weak_ref4_notified, ==, n - m); g_free (indexes); From af508f91b1e2b3eeaf9eb3126c6592c998dc3730 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Apr 2025 09:41:50 +0200 Subject: [PATCH 3/6] gobject: add internal WeakRefTuple helper structure This is already useful and will be more useful later. --- gobject/gobject.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 3a9ff87b5..4e6a65c51 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3715,13 +3715,17 @@ g_object_disconnect (gpointer _object, va_end (var_args); } -typedef struct { +typedef struct +{ + GWeakNotify notify; + gpointer data; +} WeakRefTuple; + +typedef struct +{ GObject *object; guint n_weak_refs; - struct { - GWeakNotify notify; - gpointer data; - } weak_refs[1]; /* flexible array */ + WeakRefTuple weak_refs[1]; /* flexible array */ } WeakRefStack; static void @@ -3806,8 +3810,7 @@ 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]; + WeakRefTuple *tuple = user_data; WeakRefStack *wstack = *data; gboolean found_one = FALSE; guint i; @@ -3816,8 +3819,8 @@ g_object_weak_unref_cb (gpointer *data, { for (i = 0; i < wstack->n_weak_refs; i++) { - if (wstack->weak_refs[i].notify != notify || - wstack->weak_refs[i].data != notify_data) + if (wstack->weak_refs[i].notify != tuple->notify || + wstack->weak_refs[i].data != tuple->data) continue; wstack->n_weak_refs -= 1; @@ -3839,7 +3842,7 @@ g_object_weak_unref_cb (gpointer *data, } if (!found_one) - g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, notify, notify_data); + g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, tuple->notify, tuple->data); return NULL; } @@ -3863,7 +3866,10 @@ g_object_weak_unref (GObject *object, _g_datalist_id_update_atomic (&object->qdata, quark_weak_notifies, g_object_weak_unref_cb, - ((gpointer[]){ notify, data })); + &((WeakRefTuple){ + .notify = notify, + .data = data, + })); } /** From dadb759c652a8dfa9a86a11598fa05c00d273cd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Apr 2025 11:00:49 +0200 Subject: [PATCH 4/6] gobject: invoke g_object_weak_ref() one-by-one during destruction Previously, at two places (in g_object_real_dispose() and shortly before finalize()), we would call g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); This clears @quark_weak_notifies at once and then invokes all notifications. This means, if you were inside a notification callback and called g_object_weak_unref() on the object for *another* weak-reference, then an exception failed: GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0) Granted, maybe inside a GWeakNotify you shouldn't call much of anything on where_the_object_was. However, unregistering things (like calling g_object_weak_unref()) should still reasonably work. Instead, now remove each weak notification one by one and invoke it. As we now invoke the callbacks in a loop, if a callee registers a new callback, then that one gets unregistered right away too. Previously, we would during g_object_real_dispose() only notify the notifications that were present when the loop starts. This is similar to what happens in closure_array_destroy_all(). This is a change in behavior, but it will be fixed in a separate follow-up commit. https://gitlab.gnome.org/GNOME/glib/-/issues/1002 --- gobject/gobject.c | 97 +++++++++++++++++++++++++++++--------- gobject/tests/references.c | 32 +++++++++++++ 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 4e6a65c51..07018494d 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -207,6 +207,7 @@ static guint object_floating_flag_handler (GObject *object, gint job); static inline void object_set_optional_flags (GObject *object, guint flags); +static void g_object_weak_release_all (GObject *object); static void object_interface_check_properties (gpointer check_data, gpointer g_iface); @@ -1827,7 +1828,7 @@ g_object_real_dispose (GObject *object) g_signal_handlers_destroy (object); /* GWeakNotify and GClosure can call into user code */ - g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); + g_object_weak_release_all (object); closure_array_destroy_all (object); } @@ -3723,41 +3724,33 @@ typedef struct typedef struct { - GObject *object; guint n_weak_refs; WeakRefTuple weak_refs[1]; /* flexible array */ } WeakRefStack; -static void -weak_refs_notify (gpointer data) -{ - WeakRefStack *wstack = data; - guint i; - - for (i = 0; i < wstack->n_weak_refs; i++) - wstack->weak_refs[i].notify (wstack->weak_refs[i].data, wstack->object); - 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]; + WeakRefTuple *tuple = user_data; 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; + /* We don't set a @destroy_notify. Shortly before finalize(), we call + * g_object_weak_release_all(), which frees the WeakRefStack. At that + * point the ref-count is already at zero and g_object_weak_ref() will + * assert against being called. This means, we expect that there is + * never anything to destroy. */ +#ifdef G_ENABLE_DEBUG + *destroy_notify = g_destroy_notify_assert_not_reached; +#endif } else { @@ -3767,8 +3760,7 @@ g_object_weak_ref_cb (gpointer *data, *data = wstack; - wstack->weak_refs[i].notify = notify; - wstack->weak_refs[i].data = notify_data; + wstack->weak_refs[i] = *tuple; return NULL; } @@ -3802,7 +3794,10 @@ g_object_weak_ref (GObject *object, _g_datalist_id_update_atomic (&object->qdata, quark_weak_notifies, g_object_weak_ref_cb, - ((gpointer[]){ object, notify, data })); + &((WeakRefTuple){ + .notify = notify, + .data = data, + })); } static gpointer @@ -3872,6 +3867,64 @@ g_object_weak_unref (GObject *object, })); } +static gpointer +g_object_weak_release_all_cb (gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + WeakRefTuple *tuple = user_data; + WeakRefStack *wstack = *data; + + if (!wstack) + return NULL; + +#ifdef G_ENABLE_DEBUG + g_assert (wstack->n_weak_refs > 0); +#endif + + wstack->n_weak_refs--; + + /* Emit the notifications in FIFO order. */ + *tuple = wstack->weak_refs[0]; + + if (wstack->n_weak_refs == 0) + { + g_free (wstack); + *data = NULL; + } + else + { + memmove (&wstack->weak_refs[0], + &wstack->weak_refs[1], + sizeof (wstack->weak_refs[0]) * wstack->n_weak_refs); + } + + return tuple; +} + +static void +g_object_weak_release_all (GObject *object) +{ + WeakRefTuple tuple; + + /* We notify weak references in a loop. As this emits external callbacks, a + * callee could register another weak reference, which the loop would notify + * right away. This means, registering new weak references during dispose + * does not work well, which you might want to do when resurrecting the + * object under destruction. + * + * This is an intentional choice. It would be complicated to keep track of + * the tuples that were present when the loop starts, and only notify those. + * + * You are advised to not register new weak references while handling a weak + * notification. */ + while (_g_datalist_id_update_atomic (&object->qdata, + quark_weak_notifies, + g_object_weak_release_all_cb, + &tuple)) + tuple.notify (tuple.data, object); +} + /** * g_object_add_weak_pointer: (skip) * @object: The object that should be weak referenced. @@ -4613,7 +4666,7 @@ retry_decrement: closure_array_destroy_all (object); g_signal_handlers_destroy (object); - g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); + g_object_weak_release_all (object); TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object))); G_OBJECT_GET_CLASS (object)->finalize (object); diff --git a/gobject/tests/references.c b/gobject/tests/references.c index ebd27f170..69e1984e9 100644 --- a/gobject/tests/references.c +++ b/gobject/tests/references.c @@ -337,6 +337,37 @@ test_references_many (void) /*****************************************************************************/ +static void notify_two (gpointer data, GObject *former_object); + +static void +notify_one (gpointer data, GObject *former_object) +{ + g_object_weak_unref (data, notify_two, former_object); +} + +static void +notify_two (gpointer data, GObject *former_object) +{ + g_assert_not_reached (); +} + +static void +test_references_two (void) +{ + GObject *obj; + + /* https://gitlab.gnome.org/GNOME/gtk/-/issues/5542#note_1688809 */ + + obj = g_object_new (G_TYPE_OBJECT, NULL); + + g_object_weak_ref (obj, notify_one, obj); + g_object_weak_ref (obj, notify_two, obj); + + g_object_unref (obj); +} + +/*****************************************************************************/ + int main (int argc, char *argv[]) @@ -349,6 +380,7 @@ main (int argc, g_test_add_func ("/gobject/references", test_references); g_test_add_func ("/gobject/references-many", test_references_many); + g_test_add_func ("/gobject/references_two", test_references_two); return g_test_run (); } From 1312ec6d0b41595dde76b106b29327184bf12662 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Apr 2025 21:10:05 +0200 Subject: [PATCH 5/6] gobject: grow buffers for weak notifications exponentially It seems bad style, to use a naive realloc() +1 increment each time a new element gets added. Instead, remember the allocation size and double the buffer size on buffer grow. This way we get linear amortized runtime complexity for buffer growth. Well, WeakRefStack uses a flat array for tracking the entires. We anyway need to search and memmove() the entries and are thus O(n) anyway. We do that, because it allows for relatively simple code while being memory efficient. Also, we do expect only a reasonably small number of weak notifications in the first place. I still think it makes sense to avoid the O(n) number of realloc() calls on top of that. Note that we do this while holding the (per-object) lock. It's one thing to do a linear search or a memmove(). It's another to do a (more expensive) realloc(). Also, shrink the buffer during g_object_weak_unref() to get rid of excess memory. Also, note that the initial allocation only allocates space for the first item. I think that makes sense, because I expect that many objects will only get a single weak notification registered. So this allocation should not yet have excess memory allocated. Also, note that the "flexible" array member WeakRefStack.weak_refs has a length of 1. Maybe we should use C99 flexible array members ([]) or the pre-C99 workaround ([0]). Anyway. Previously, we would always allocate space for that extra one tuple, but never use it. Fix that too. --- gobject/gobject.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 07018494d..b80c6b2b9 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3725,9 +3725,12 @@ typedef struct typedef struct { guint n_weak_refs; + guint alloc_size; WeakRefTuple weak_refs[1]; /* flexible array */ } WeakRefStack; +#define WEAK_REF_STACK_ALLOC_SIZE(alloc_size) (G_STRUCT_OFFSET (WeakRefStack, weak_refs) + sizeof (WeakRefTuple) * (alloc_size)) + static gpointer g_object_weak_ref_cb (gpointer *data, GDestroyNotify *destroy_notify, @@ -3739,10 +3742,12 @@ g_object_weak_ref_cb (gpointer *data, if (!wstack) { - wstack = g_new (WeakRefStack, 1); + wstack = g_malloc (WEAK_REF_STACK_ALLOC_SIZE (1)); + wstack->alloc_size = 1; wstack->n_weak_refs = 1; i = 0; + *data = wstack; /* We don't set a @destroy_notify. Shortly before finalize(), we call * g_object_weak_release_all(), which frees the WeakRefStack. At that * point the ref-count is already at zero and g_object_weak_ref() will @@ -3755,10 +3760,17 @@ g_object_weak_ref_cb (gpointer *data, else { i = wstack->n_weak_refs++; - wstack = g_realloc (wstack, sizeof (*wstack) + sizeof (wstack->weak_refs[0]) * i); - } - *data = wstack; + if (G_UNLIKELY (wstack->n_weak_refs > wstack->alloc_size)) + { + if (G_UNLIKELY (wstack->alloc_size >= (G_MAXUINT / 2u + 1u))) + g_error ("g_object_weak_ref(): cannot register more than 2^31 references"); + wstack->alloc_size = wstack->alloc_size * 2u; + + wstack = g_realloc (wstack, WEAK_REF_STACK_ALLOC_SIZE (wstack->alloc_size)); + *data = wstack; + } + } wstack->weak_refs[i] = *tuple; @@ -3824,11 +3836,21 @@ g_object_weak_unref_cb (gpointer *data, g_free (wstack); *data = NULL; } - else if (i != wstack->n_weak_refs) + else { - memmove (&wstack->weak_refs[i], - &wstack->weak_refs[i + 1], - sizeof (wstack->weak_refs[i]) * (wstack->n_weak_refs - i)); + if (i != wstack->n_weak_refs) + { + memmove (&wstack->weak_refs[i], + &wstack->weak_refs[i + 1], + sizeof (wstack->weak_refs[i]) * (wstack->n_weak_refs - i)); + } + + if (G_UNLIKELY (wstack->n_weak_refs <= wstack->alloc_size / 4u)) + { + wstack->alloc_size = wstack->alloc_size / 2u; + wstack = g_realloc (wstack, WEAK_REF_STACK_ALLOC_SIZE (wstack->alloc_size)); + *data = wstack; + } } found_one = TRUE; @@ -3897,6 +3919,9 @@ g_object_weak_release_all_cb (gpointer *data, memmove (&wstack->weak_refs[0], &wstack->weak_refs[1], sizeof (wstack->weak_refs[0]) * wstack->n_weak_refs); + + /* Don't bother shrinking the buffer. The caller will loop until all + * elements are removed. */ } return tuple; From 0be672e1e0653386c20cdcab1c9af651bbd199fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Apr 2025 08:59:29 +0200 Subject: [PATCH 6/6] gobject: preserve weak notifications registered during dispose We call g_object_weak_release_all() at two places. Once right before finalize(). At this point, the object is definitely going to be destroyed, and the user must no longer resurrect it or subscribe new weak notifications. In that case, we really want to notify/release all weak notifications. However, we also call it from g_object_real_dispose(). During dispose, the API allows the user to resurrect an object. Granted, that is probably not something anybody should do, but GObject makes a reasonable attempt to support that. A possible place to resurrect (and subscribe new weak notifications) is when GObject calls g_object_real_dispose(). static void g_object_real_dispose (GObject *object) { g_signal_handlers_destroy (object); /* GWeakNotify and GClosure can call into user code */ g_object_weak_release_all (object); closure_array_destroy_all (object); } But previously, g_object_weak_release_all() would continue iterating until there are no more weak notifications left. So while the user can take a strong reference and resurrect the object, their attempts to register new weak notifications are thwarted. Instead, when the loop in g_object_weak_release_all() starts, remember the initial number of weak notifications, and don't release more than that. Note that WeakRefStack preserves the order of entries, so by maintaining the "remaining_to_notify" counter we know when to stop. Note that this brings also an earlier behavior back, where we would call g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); This would take out the entire WeakRefStack at once and notify the weak notifications registered at the time. But subsequent registrations would not be released/notified yet. --- gobject/gobject.c | 197 +++++++++++++++++++++++++++++++------ gobject/tests/references.c | 190 ++++++++++++++++++++++++++++++++++- 2 files changed, 355 insertions(+), 32 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index b80c6b2b9..1abeeca48 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -207,7 +207,7 @@ static guint object_floating_flag_handler (GObject *object, gint job); static inline void object_set_optional_flags (GObject *object, guint flags); -static void g_object_weak_release_all (GObject *object); +static void g_object_weak_release_all (GObject *object, gboolean release_all); static void object_interface_check_properties (gpointer check_data, gpointer g_iface); @@ -1828,7 +1828,7 @@ g_object_real_dispose (GObject *object) g_signal_handlers_destroy (object); /* GWeakNotify and GClosure can call into user code */ - g_object_weak_release_all (object); + g_object_weak_release_all (object, FALSE); closure_array_destroy_all (object); } @@ -3722,15 +3722,85 @@ typedef struct gpointer data; } WeakRefTuple; +struct _WeakRefReleaseAllState; + +typedef struct _WeakRefReleaseAllState +{ + guint remaining_to_notify; + struct _WeakRefReleaseAllState *release_all_next; +} WeakRefReleaseAllState; + typedef struct { guint n_weak_refs; guint alloc_size; + WeakRefReleaseAllState *release_all_states; WeakRefTuple weak_refs[1]; /* flexible array */ } WeakRefStack; #define WEAK_REF_STACK_ALLOC_SIZE(alloc_size) (G_STRUCT_OFFSET (WeakRefStack, weak_refs) + sizeof (WeakRefTuple) * (alloc_size)) +G_GNUC_UNUSED G_ALWAYS_INLINE static inline gboolean +_weak_ref_release_all_state_contains (WeakRefReleaseAllState *release_all_state, WeakRefReleaseAllState *needle) +{ + for (; release_all_state; release_all_state = release_all_state->release_all_next) + { + if (release_all_state == needle) + return TRUE; + } + return FALSE; +} + +G_ALWAYS_INLINE static inline void +_weak_ref_stack_free (WeakRefStack *wstack) +{ +#ifdef G_ENABLE_DEBUG + g_assert (!wstack->release_all_states); +#endif + g_free (wstack); +} + +G_ALWAYS_INLINE static inline void +_weak_ref_stack_update_release_all_state (WeakRefStack *wstack, guint idx) +{ + WeakRefReleaseAllState **previous_ptr; + WeakRefReleaseAllState *release_all_state; + +#ifdef G_ENABLE_DEBUG + g_assert (idx < wstack->n_weak_refs); +#endif + + previous_ptr = &wstack->release_all_states; + + while (G_UNLIKELY ((release_all_state = *previous_ptr))) + { + if (idx >= release_all_state->remaining_to_notify) + { +#ifdef G_ENABLE_DEBUG + g_assert (release_all_state->remaining_to_notify <= wstack->n_weak_refs); +#endif + /* We removed an index higher than the "remaining_to_notify" count. */ + goto next; + } + + /* Lower the "remaining_to_notify" bar of the entries we consider, as we + * just removed an entry at index @idx (below that bar). */ + release_all_state->remaining_to_notify--; + + if (release_all_state->remaining_to_notify > 0) + goto next; + + /* Remove the entry from the linked list. No need to reset + * release_all_state->release_all_next pointer to NULL as it has no + * purpose when not being linked. */ + *previous_ptr = release_all_state->release_all_next; + continue; + + next: + previous_ptr = &release_all_state->release_all_next; + } +} + static gpointer g_object_weak_ref_cb (gpointer *data, GDestroyNotify *destroy_notify, @@ -3745,6 +3815,7 @@ g_object_weak_ref_cb (gpointer *data, wstack = g_malloc (WEAK_REF_STACK_ALLOC_SIZE (1)); wstack->alloc_size = 1; wstack->n_weak_refs = 1; + wstack->release_all_states = NULL; i = 0; *data = wstack; @@ -3830,10 +3901,12 @@ g_object_weak_unref_cb (gpointer *data, wstack->weak_refs[i].data != tuple->data) continue; + _weak_ref_stack_update_release_all_state (wstack, i); + wstack->n_weak_refs -= 1; if (wstack->n_weak_refs == 0) { - g_free (wstack); + _weak_ref_stack_free (wstack); *data = NULL; } else @@ -3889,13 +3962,21 @@ g_object_weak_unref (GObject *object, })); } +typedef struct +{ + WeakRefReleaseAllState *const release_all_state; + WeakRefTuple tuple; + gboolean release_all_done; +} WeakRefReleaseAllData; + static gpointer g_object_weak_release_all_cb (gpointer *data, GDestroyNotify *destroy_notify, gpointer user_data) { - WeakRefTuple *tuple = user_data; WeakRefStack *wstack = *data; + WeakRefReleaseAllData *wdata = user_data; + WeakRefReleaseAllState *release_all_state = wdata->release_all_state; if (!wstack) return NULL; @@ -3904,15 +3985,72 @@ g_object_weak_release_all_cb (gpointer *data, g_assert (wstack->n_weak_refs > 0); #endif + if (release_all_state) + { + if (release_all_state->remaining_to_notify == G_MAXUINT) + { + if (wstack->n_weak_refs == 1u) + { + /* We only pop the single entry. */ + wdata->release_all_done = TRUE; + release_all_state = NULL; + } + else + { + release_all_state->remaining_to_notify = wstack->n_weak_refs; + + /* Prepend to linked list. */ + release_all_state->release_all_next = wstack->release_all_states; + wstack->release_all_states = release_all_state; + } + } + else + { + if (release_all_state->remaining_to_notify == 0u) + { +#ifdef G_ENABLE_DEBUG + g_assert (!_weak_ref_release_all_state_contains (wstack->release_all_states, release_all_state)); +#endif + return NULL; + } +#ifdef G_ENABLE_DEBUG + g_assert (release_all_state->remaining_to_notify <= wstack->n_weak_refs); + g_assert (_weak_ref_release_all_state_contains (wstack->release_all_states, release_all_state)); +#endif + } + } + + _weak_ref_stack_update_release_all_state (wstack, 0); + + if (release_all_state && release_all_state->remaining_to_notify == 0) + wdata->release_all_done = TRUE; + wstack->n_weak_refs--; /* Emit the notifications in FIFO order. */ - *tuple = wstack->weak_refs[0]; + wdata->tuple = wstack->weak_refs[0]; if (wstack->n_weak_refs == 0) { - g_free (wstack); + _weak_ref_stack_free (wstack); *data = NULL; + + /* Also set release_all_done. + * + * If g_object_weak_release_all() was called during dispose (with + * release_all FALSE), we anyway have an upper limit of how many + * notifications we want to pop. We only pop the notifications that were + * registered when the loop initially starts. In that case, we surely + * don't want the caller to call back. + * + * g_object_weak_release_all() is also being called before finalize. At + * that point, the ref count is already at zero, and g_object_weak_ref() + * asserts against being called. So nobody can register a new weak ref + * anymore. + * + * In both cases, we don't require the calling loop to call back. This + * saves an additional GData lookup. */ + wdata->release_all_done = TRUE; } else { @@ -3920,34 +4058,37 @@ g_object_weak_release_all_cb (gpointer *data, &wstack->weak_refs[1], sizeof (wstack->weak_refs[0]) * wstack->n_weak_refs); - /* Don't bother shrinking the buffer. The caller will loop until all - * elements are removed. */ + /* Don't bother to shrink the buffer. Most likely the object gets + * destroyed soon after. */ } - return tuple; + return wdata; } static void -g_object_weak_release_all (GObject *object) +g_object_weak_release_all (GObject *object, gboolean release_all) { - WeakRefTuple tuple; + WeakRefReleaseAllState release_all_state = { + .remaining_to_notify = G_MAXUINT, + }; + WeakRefReleaseAllData wdata = { + .release_all_state = release_all ? NULL : &release_all_state, + .release_all_done = FALSE, + }; - /* We notify weak references in a loop. As this emits external callbacks, a - * callee could register another weak reference, which the loop would notify - * right away. This means, registering new weak references during dispose - * does not work well, which you might want to do when resurrecting the - * object under destruction. - * - * This is an intentional choice. It would be complicated to keep track of - * the tuples that were present when the loop starts, and only notify those. - * - * You are advised to not register new weak references while handling a weak - * notification. */ - while (_g_datalist_id_update_atomic (&object->qdata, - quark_weak_notifies, - g_object_weak_release_all_cb, - &tuple)) - tuple.notify (tuple.data, object); + while (TRUE) + { + if (!_g_datalist_id_update_atomic (&object->qdata, + quark_weak_notifies, + g_object_weak_release_all_cb, + &wdata)) + break; + + wdata.tuple.notify (wdata.tuple.data, object); + + if (wdata.release_all_done) + break; + } } /** @@ -4691,7 +4832,7 @@ retry_decrement: closure_array_destroy_all (object); g_signal_handlers_destroy (object); - g_object_weak_release_all (object); + g_object_weak_release_all (object, TRUE); TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object))); G_OBJECT_GET_CLASS (object)->finalize (object); diff --git a/gobject/tests/references.c b/gobject/tests/references.c index 69e1984e9..e0cf540dd 100644 --- a/gobject/tests/references.c +++ b/gobject/tests/references.c @@ -285,19 +285,80 @@ test_references (void) /*****************************************************************************/ static guint weak_ref4_notified; +static guint weak_ref4_finalizing_notified; +static guint weak_ref4_finalizing_notified_skipped; +static gint weak_ref4_finalizing_resurrected = -1; +static guint weak_ref4_indexes_n; +static const guint *weak_ref4_indexes; + +static void +weak_ref4_finalizing (gpointer data, + GObject *object) +{ + static gint last = -1; + gint idx; + + g_assert_true (weak_ref4_finalizing_resurrected == FALSE || weak_ref4_finalizing_resurrected == 4); + + idx = (gint) GPOINTER_TO_UINT (data); + g_assert_cmpint (last, <, idx); + last = idx; + + weak_ref4_finalizing_notified++; +} static void weak_ref4 (gpointer data, GObject *object) { static gint last = -1; - gint idx; + guint idx; - idx = (gint) GPOINTER_TO_UINT (data); - g_assert_cmpint (last, <, idx); - last = idx; + g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0); + + idx = GPOINTER_TO_UINT (data); + g_assert_cmpint (last, <, (gint) idx); + last = (gint) idx; weak_ref4_notified++; + + if (weak_ref4_finalizing_resurrected == -1) + { + if (g_random_boolean ()) + { + /* Resurrect the object. */ + weak_ref4_finalizing_resurrected = TRUE; + g_object_ref (object); + } + else + weak_ref4_finalizing_resurrected = FALSE; + } + + g_object_weak_ref (object, weak_ref4_finalizing, GUINT_TO_POINTER (idx)); + + if (weak_ref4_indexes_n > 0 && (g_random_int () % 4 == 0)) + { + + while (weak_ref4_indexes_n > 0 && weak_ref4_indexes[weak_ref4_indexes_n - 1] <= idx) + { + /* already invoked. Skip. */ + weak_ref4_indexes_n--; + } + + /* From the callback, we unregister another callback that we know is + * still registered. */ + if (weak_ref4_indexes_n > 0) + { + guint new_idx = weak_ref4_indexes[weak_ref4_indexes_n - 1]; + + weak_ref4_indexes_n--; + g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (new_idx)); + + /* Behave as if we got this callback invoked still, so that the remainder matches up. */ + weak_ref4_finalizing_notified_skipped++; + weak_ref4_notified++; + } + } } static void @@ -330,8 +391,29 @@ test_references_many (void) m = g_random_int () % (n + 1u); for (i = 0; i < m; i++) g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (indexes[i])); + weak_ref4_indexes_n = n - m; + weak_ref4_indexes = &indexes[m]; g_object_unref (object); g_assert_cmpint (weak_ref4_notified, ==, n - m); + if (n - m == 0) + { + g_assert_cmpint (weak_ref4_finalizing_resurrected, ==, -1); + g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0); + g_assert_cmpint (weak_ref4_finalizing_notified_skipped, ==, 0); + } + else if (weak_ref4_finalizing_resurrected == TRUE) + { + weak_ref4_finalizing_resurrected = 4; + g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0); + g_object_unref (object); + g_assert_cmpint (weak_ref4_notified, ==, n - m); + g_assert_cmpint (weak_ref4_finalizing_notified + weak_ref4_finalizing_notified_skipped, ==, n - m); + } + else + { + g_assert_cmpint (weak_ref4_finalizing_resurrected, ==, FALSE); + g_assert_cmpint (weak_ref4_finalizing_notified + weak_ref4_finalizing_notified_skipped, ==, n - m); + } g_free (indexes); } @@ -368,6 +450,105 @@ test_references_two (void) /*****************************************************************************/ +#define RUN_DISPOSE_N_THREADS 5 + +GThread *run_dispose_threads[RUN_DISPOSE_N_THREADS]; +GObject *run_dispose_obj; +gint run_dispose_thread_ready_count; +const guint RUN_DISPOSE_N_ITERATIONS = 5000; + +gint run_dispose_weak_notify_pending; +gint run_dispose_weak_notify_pending_nested; + +static void +_run_dispose_weak_notify_nested (gpointer data, + GObject *where_the_object_was) +{ + g_assert_true (run_dispose_obj == where_the_object_was); + g_assert_cmpint (g_atomic_int_add (&run_dispose_weak_notify_pending_nested, -1), >, 0); +} + +static void +_run_dispose_weak_notify (gpointer data, + GObject *where_the_object_was) +{ + g_assert_true (run_dispose_obj == where_the_object_was); + g_assert_cmpint (g_atomic_int_add (&run_dispose_weak_notify_pending, -1), >, 0); + + if (g_random_int () % 10 == 0) + { + g_object_run_dispose (run_dispose_obj); + } + + if (g_random_int () % 10 == 0) + { + g_atomic_int_inc (&run_dispose_weak_notify_pending_nested); + g_object_weak_ref (run_dispose_obj, _run_dispose_weak_notify_nested, NULL); + } +} + +static gpointer +_run_dispose_thread_fcn (gpointer thread_data) +{ + guint i; + + g_atomic_int_inc (&run_dispose_thread_ready_count); + + while (g_atomic_int_get (&run_dispose_thread_ready_count) != RUN_DISPOSE_N_THREADS) + g_usleep (10); + + for (i = 0; i < RUN_DISPOSE_N_ITERATIONS; i++) + { + g_atomic_int_inc (&run_dispose_weak_notify_pending); + g_object_weak_ref (run_dispose_obj, _run_dispose_weak_notify, NULL); + if (g_random_int () % 10 == 0) + g_object_run_dispose (run_dispose_obj); + } + + g_object_run_dispose (run_dispose_obj); + + return NULL; +} + +static void +test_references_run_dispose (void) +{ + GQuark quark_weak_notifies = g_quark_from_static_string ("GObject-weak-notifies"); + guint i; + + run_dispose_obj = g_object_new (G_TYPE_OBJECT, NULL); + + for (i = 0; i < RUN_DISPOSE_N_THREADS; i++) + { + run_dispose_threads[i] = g_thread_new ("run-dispose", _run_dispose_thread_fcn, GINT_TO_POINTER (i)); + } + for (i = 0; i < RUN_DISPOSE_N_THREADS; i++) + { + g_thread_join (run_dispose_threads[i]); + } + + g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending), ==, 0); + + if (g_atomic_int_get (&run_dispose_weak_notify_pending_nested) == 0) + { + g_assert_null (g_object_get_qdata (run_dispose_obj, quark_weak_notifies)); + } + else + { + g_assert_nonnull (g_object_get_qdata (run_dispose_obj, quark_weak_notifies)); + } + + g_object_run_dispose (run_dispose_obj); + + g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending), ==, 0); + g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending_nested), ==, 0); + g_assert_null (g_object_get_qdata (run_dispose_obj, quark_weak_notifies)); + + g_clear_object (&run_dispose_obj); +} + +/*****************************************************************************/ + int main (int argc, char *argv[]) @@ -381,6 +562,7 @@ main (int argc, g_test_add_func ("/gobject/references", test_references); g_test_add_func ("/gobject/references-many", test_references_many); g_test_add_func ("/gobject/references_two", test_references_two); + g_test_add_func ("/gobject/references_run_dispose", test_references_run_dispose); return g_test_run (); }