Merge branch 'th/optimize-weak-ref-list' into 'main'

[th/optimize-weak-ref-list] rework GObject's `WeakRefData` to track references in an array instead of GSList

See merge request GNOME/glib!3869

gobject/gobject.c

@@ -262,20 +262,61 @@ object_get_optional_flags_p (GObject *object)
  * and has a separate lifetime from the object. */
 typedef struct
 {
-  gatomicrefcount ref_count;
-  gint lock_flags; /* (atomic) */
-  GSList *list;    /* (element-type GWeakRef) (owned) */
+  /* This is both an atomic ref-count and bit 30 (WEAK_REF_DATA_LOCK_BIT) is
+   * used for g_bit_lock(). */
+  gint atomic_field;
+
+  guint16 len;
+
+  /* Only relevant when len > 1. In that case, it's the allocated size of
+   * "list.many" array.  */
+  guint16 alloc;
+
+  /* Only relevant when len > 0. In that case, either "one" or "many" union
+   * field is in use. */
+  union
+  {
+    GWeakRef *one;
+    GWeakRef **many;
+  } list;
 } WeakRefData;
 
+/* We choose bit 30, and not bit 31. Bit 31 would be the sign for gint, so it
+ * a bit awkward to use. Note that it probably also would work fine.
+ *
+ * But 30 is ok, because it still leaves us space for 2^30-1 references, which
+ * is more than we ever need. */
+#define WEAK_REF_DATA_LOCK_BIT 30
+
 static void weak_ref_data_clear_list (WeakRefData *wrdata, GObject *object);
 
 static WeakRefData *
 weak_ref_data_ref (WeakRefData *wrdata)
 {
+  gint ref;
+
 #if G_ENABLE_DEBUG
   g_assert (wrdata);
 #endif
-  g_atomic_ref_count_inc (&wrdata->ref_count);
+
+  ref = g_atomic_int_add (&wrdata->atomic_field, 1);
+
+#if G_ENABLE_DEBUG
+  /* Overflow is almost impossible to happen, because the user would need to
+   * spawn that many operating system threads, that all call
+   * g_weak_ref_{set,get}() in parallel.
+   *
+   * Still, assert in debug mode. */
+  g_assert (ref < G_MAXINT32);
+
+  /* the real ref-count would be the following: */
+  ref = (ref + 1) & ~(1 << WEAK_REF_DATA_LOCK_BIT);
+
+  /* assert that the ref-count is still in the valid range. */
+  g_assert (ref > 0 && ref < (1 << WEAK_REF_DATA_LOCK_BIT));
+#endif
+  (void) ref;
+
   return wrdata;
 }
 
@@ -285,7 +326,16 @@ weak_ref_data_unref (WeakRefData *wrdata)
   if (!wrdata)
     return;
 
-  if (!g_atomic_ref_count_dec (&wrdata->ref_count))
+  /* Note that we also use WEAK_REF_DATA_LOCK_BIT on "atomic_field" as a bit
+   * lock. However, we will always keep the @wrdata alive (having a reference)
+   * while holding a lock (otherwise, we couldn't unlock anymore). Thus, at the
+   * point when we decrement the ref-count to zero, we surely also have the
+   * @wrdata unlocked.
+   *
+   * This means, using "aomit_field" both as ref-count and the lock bit is
+   * fine. */
+
+  if (!g_atomic_int_dec_and_test (&wrdata->atomic_field))
     return;
 
 #if G_ENABLE_DEBUG
@@ -294,7 +344,7 @@ weak_ref_data_unref (WeakRefData *wrdata)
    * should have been cleared.
    *
    * Calling weak_ref_data_clear_list() should be unnecessary. */
-  g_assert (!wrdata->list);
+  g_assert (wrdata->len == 0);
 #endif
 
   g_free_sized (wrdata, sizeof (WeakRefData));
@@ -306,14 +356,14 @@ weak_ref_data_lock (WeakRefData *wrdata)
   /* Note that while holding a _weak_ref_lock() on the @weak_ref, we MUST not acquire a
    * weak_ref_data_lock() on the @wrdata. The other way around! */
   if (wrdata)
-    g_bit_lock (&wrdata->lock_flags, 0);
+    g_bit_lock (&wrdata->atomic_field, WEAK_REF_DATA_LOCK_BIT);
 }
 
 static void
 weak_ref_data_unlock (WeakRefData *wrdata)
 {
   if (wrdata)
-    g_bit_unlock (&wrdata->lock_flags, 0);
+    g_bit_unlock (&wrdata->atomic_field, WEAK_REF_DATA_LOCK_BIT);
 }
 
 static gpointer
@@ -328,13 +378,15 @@ weak_ref_data_get_or_create_cb (GQuark key_id,
   if (!wrdata)
     {
       wrdata = g_new (WeakRefData, 1);
-      *wrdata = (WeakRefData){
+
       /* The initial ref-count is 1. This one is owned by the GData until the
-         * object gets destroyed. */
-        .ref_count = 1,
-        .lock_flags = 0,
-        .list = NULL,
-      };
+       * object gets destroyed.
+       *
+       * The WEAK_REF_DATA_LOCK_BIT bit is of course initially unset.  */
+      wrdata->atomic_field = 1;
+      wrdata->len = 0;
+      /* Other fields are left uninitialized. They are only considered with a positive @len. */
+
       *data = wrdata;
       *destroy_notify = (GDestroyNotify) weak_ref_data_unref;
 
@@ -387,6 +439,110 @@ weak_ref_data_get_surely (GObject *object)
   return wrdata;
 }
 
+static gint32
+weak_ref_data_list_find (WeakRefData *wrdata, GWeakRef *weak_ref)
+{
+  if (wrdata->len == 1u)
+    {
+      if (wrdata->list.one == weak_ref)
+        return 0;
+    }
+  else
+    {
+      guint16 i;
+
+      for (i = 0; i < wrdata->len; i++)
+        {
+          if (wrdata->list.many[i] == weak_ref)
+            return i;
+        }
+    }
+
+  return -1;
+}
+
+static gboolean
+weak_ref_data_list_add (WeakRefData *wrdata, GWeakRef *weak_ref)
+{
+  if (wrdata->len == 0u)
+    wrdata->list.one = weak_ref;
+  else
+    {
+      if (wrdata->len == 1u)
+        {
+          GWeakRef *weak_ref2 = wrdata->list.one;
+
+          wrdata->alloc = 4u;
+          wrdata->list.many = g_new (GWeakRef *, wrdata->alloc);
+          wrdata->list.many[0] = weak_ref2;
+        }
+      else if (wrdata->len == wrdata->alloc)
+        {
+          guint16 alloc;
+
+          alloc = wrdata->alloc * 2u;
+          if (G_UNLIKELY (alloc < wrdata->len))
+            {
+              if (wrdata->len == G_MAXUINT16)
+                return FALSE;
+              alloc = G_MAXUINT16;
+            }
+          wrdata->list.many = g_renew (GWeakRef *, wrdata->list.many, alloc);
+          wrdata->alloc = alloc;
+        }
+
+      wrdata->list.many[wrdata->len] = weak_ref;
+    }
+
+  wrdata->len++;
+  return TRUE;
+}
+
+static GWeakRef *
+weak_ref_data_list_remove (WeakRefData *wrdata, guint16 idx, gboolean allow_shrink)
+{
+  GWeakRef *weak_ref;
+
+#if G_ENABLE_DEBUG
+  g_assert (idx < wrdata->len);
+#endif
+
+  wrdata->len--;
+
+  if (wrdata->len == 0u)
+    {
+      weak_ref = wrdata->list.one;
+    }
+  else
+    {
+      weak_ref = wrdata->list.many[idx];
+
+      if (wrdata->len == 1u)
+        {
+          GWeakRef *weak_ref2 = wrdata->list.many[idx == 0 ? 1 : 0];
+
+          g_free (wrdata->list.many);
+          wrdata->list.one = weak_ref2;
+        }
+      else
+        {
+          wrdata->list.many[idx] = wrdata->list.many[wrdata->len];
+
+          if (allow_shrink && G_UNLIKELY (wrdata->len <= wrdata->alloc / 4u))
+            {
+              /* Shrink the buffer. When 75% are empty, shrink it to 50%. */
+              if (wrdata->alloc == G_MAXUINT16)
+                wrdata->alloc = ((guint32) G_MAXUINT16 + 1u) / 2u;
+              else
+                wrdata->alloc /= 2u;
+              wrdata->list.many = g_renew (GWeakRef *, wrdata->list.many, wrdata->alloc);
+            }
+        }
+    }
+
+  return weak_ref;
+}
+
 static gboolean
 weak_ref_data_has (GObject *object, WeakRefData *wrdata, WeakRefData **out_new_wrdata)
 {
@@ -5290,12 +5446,14 @@ _weak_ref_unlock_and_set (GWeakRef *weak_ref, GObject *object)
 static void
 weak_ref_data_clear_list (WeakRefData *wrdata, GObject *object)
 {
-  while (wrdata->list)
+  while (wrdata->len > 0u)
     {
-      GWeakRef *weak_ref = wrdata->list->data;
+      GWeakRef *weak_ref;
       gpointer ptr;
 
-      wrdata->list = g_slist_remove (wrdata->list, weak_ref);
+      /* pass "allow_shrink=FALSE", so we don't reallocate needlessly. We
+       * anyway are about to clear the entire list. */
+      weak_ref = weak_ref_data_list_remove (wrdata, wrdata->len - 1u, FALSE);
 
       /* Fast-path. Most likely @weak_ref is currently not locked, so we can
        * just atomically set the pointer to NULL. */
@@ -5407,17 +5565,13 @@ _weak_ref_set (GWeakRef *weak_ref,
 
   if (old_object)
     {
-#if G_ENABLE_DEBUG
-      g_assert (g_slist_find (old_wrdata->list, weak_ref));
-#endif
-      if (!old_wrdata->list)
-        {
+      gint32 idx;
+
+      idx = weak_ref_data_list_find (old_wrdata, weak_ref);
+      if (idx < 0)
         g_critical ("unexpected missing GWeakRef data");
-        }
       else
-        {
-          old_wrdata->list = g_slist_remove (old_wrdata->list, weak_ref);
-        }
+        weak_ref_data_list_remove (old_wrdata, idx, TRUE);
     }
 
   weak_ref_data_unlock (old_wrdata);
@@ -5425,7 +5579,7 @@ _weak_ref_set (GWeakRef *weak_ref,
   if (new_object)
     {
 #if G_ENABLE_DEBUG
-      g_assert (!g_slist_find (new_wrdata->list, weak_ref));
+      g_assert (weak_ref_data_list_find (new_wrdata, weak_ref) < 0);
 #endif
       if (g_atomic_int_get (&new_object->ref_count) < 1)
         {
@@ -5434,7 +5588,11 @@ _weak_ref_set (GWeakRef *weak_ref,
         }
       else
         {
-          new_wrdata->list = g_slist_prepend (new_wrdata->list, weak_ref);
+          if (!weak_ref_data_list_add (new_wrdata, weak_ref))
+            {
+              g_critical ("Too many GWeakRef registered");
+              new_object = NULL;
+            }
         }
     }
 

gobject/tests/reference.c

@@ -767,6 +767,59 @@ test_weak_ref_in_toggle_notify (void)
   g_assert_null (g_weak_ref_get (&weak));
 }
 
+static void
+test_weak_ref_many (void)
+{
+  const guint N = g_test_slow ()
+                      ? G_MAXUINT16
+                      : 211;
+  const guint PRIME = 1048583;
+  GObject *obj;
+  GWeakRef *weak_refs;
+  GWeakRef weak_ref1;
+  guint j;
+  guint n;
+  guint i;
+
+  obj = g_object_new (G_TYPE_OBJECT, NULL);
+
+  weak_refs = g_new (GWeakRef, N);
+
+  /* We register them in a somewhat juggled order. That's because below, we will clear them
+   * again, and we don't want to always clear them in the same order as they were registered.
+   * For that, we calculate the actual index by jumping around by adding a prime number. */
+  j = (g_test_rand_int () % (N + 1));
+  for (i = 0; i < N; i++)
+    {
+      j = (j + PRIME) % N;
+      g_weak_ref_init (&weak_refs[j], obj);
+    }
+
+  if (N == G_MAXUINT16)
+    {
+      g_test_expect_message ("GLib-GObject", G_LOG_LEVEL_CRITICAL, "*Too many GWeakRef registered");
+      g_weak_ref_init (&weak_ref1, obj);
+      g_test_assert_expected_messages ();
+      g_assert_null (g_weak_ref_get (&weak_ref1));
+    }
+
+  n = g_test_rand_int () % (N + 1u);
+  for (i = 0; i < N; i++)
+    g_weak_ref_set (&weak_refs[i], i < n ? NULL : obj);
+
+  g_object_unref (obj);
+
+  for (i = 0; i < N; i++)
+    g_assert_null (g_weak_ref_get (&weak_refs[i]));
+
+  /* The API would expect us to also call g_weak_ref_clear() on all references
+   * to clean up.  In practice, they are already all NULL, so we don't need
+   * that (it would have no effect, with the current implementation of
+   * GWeakRef). */
+
+  g_free (weak_refs);
+}
+
 /*****************************************************************************/
 
 #define CONCURRENT_N_OBJS 5
@@ -1584,6 +1637,7 @@ main (int argc, char **argv)
   g_test_add_func ("/object/weak-ref/on-run-dispose", test_weak_ref_on_run_dispose);
   g_test_add_func ("/object/weak-ref/on-toggle-notify", test_weak_ref_on_toggle_notify);
   g_test_add_func ("/object/weak-ref/in-toggle-notify", test_weak_ref_in_toggle_notify);
+  g_test_add_func ("/object/weak-ref/many", test_weak_ref_many);
   g_test_add_data_func ("/object/weak-ref/concurrent/0", GINT_TO_POINTER (0), test_weak_ref_concurrent);
   g_test_add_data_func ("/object/weak-ref/concurrent/1", GINT_TO_POINTER (1), test_weak_ref_concurrent);
   g_test_add_func ("/object/toggle-ref", test_toggle_ref);