glib: add internal g_datalist_id_update_atomic() function

GDataSet is mainly used by GObject. Usually, when we access the private
data there, we already hold another lock around the GObject.

For example, before accessing quark_toggle_refs, we take a
OPTIONAL_BIT_LOCK_TOGGLE_REFS lock. That makes sense, because we anyway
need to protect access to the ToggleRefStack. By holding such an
external mutex around several GData operations, we achieve atomic
updates.

However, there is a (performance) use case to update the qdata
atomically, without such additional lock. The GData already holds a lock
while updating the data. Add a new g_datalist_id_update_atomic()
function, that can invoke a callback while holding that lock.

This will be used by GObject. The benefit is that we can access the
GData atomically, without requiring another mutex around it.

For example, a common pattern is to request some GData entry, and if
it's not yet allocated, to allocate it. This requires to take the GData
bitlock twice. With this API, the callback can allocate the data if no
entry exists yet.

glib/gdataset.c

@@ -827,6 +827,119 @@ g_datalist_id_remove_no_notify (GData	**datalist,
   return ret_data;
 }
 
+/*< private >
+ * g_datalist_id_update_atomic:
+ * @datalist: the data list
+ * @key_id: the key to add.
+ * @callback: (scope call): callback to update (set, remove, steal, update) the
+ *   data.
+ * @user_data: the user data for @callback.
+ *
+ * Will call @callback while holding the lock on @datalist. Be careful to not
+ * end up calling into another data-list function, because the lock is not
+ * reentrant and deadlock will happen.
+ *
+ * The callback receives the current data and destroy function. If @key_id is
+ * currently not in @datalist, they will be %NULL. The callback can update
+ * those pointers, and @datalist will be updated with the result. Note that if
+ * callback modifies a received data, then it MUST steal it and take ownership
+ * on it. Possibly by freeing it with the provided destroy function.
+ *
+ * The point is to atomically access the entry, while holding a lock
+ * of @datalist. Without this, the user would have to hold their own mutex
+ * while handling @key_id entry.
+ *
+ * The return value of @callback is not used, except it becomes the return
+ * value of the function. This is an alternative to returning a result via
+ * @user_data.
+ *
+ * Returns: the value returned by @callback.
+ *
+ * Since: 2.80
+ */
+gpointer
+g_datalist_id_update_atomic (GData **datalist,
+                             GQuark key_id,
+                             GDataListUpdateAtomicFunc callback,
+                             gpointer user_data)
+{
+  GData *d;
+  GDataElt *data;
+  gpointer new_data;
+  gpointer result;
+  GDestroyNotify new_destroy;
+  guint32 idx;
+  gboolean to_unlock = TRUE;
+
+  d = g_datalist_lock_and_get (datalist);
+
+  data = datalist_find (d, key_id, &idx);
+
+  if (data)
+    {
+      new_data = data->data;
+      new_destroy = data->destroy;
+    }
+  else
+    {
+      new_data = NULL;
+      new_destroy = NULL;
+    }
+
+  result = callback (key_id, &new_data, &new_destroy, user_data);
+
+  if (data && !new_data)
+    {
+      /* Remove. The callback indicates to drop the entry.
+       *
+       * The old data->data was stolen by callback(). */
+      d->len--;
+
+      /* We don't bother to shrink, but if all data are now gone
+       * we at least free the memory
+       */
+      if (d->len == 0)
+        {
+          g_datalist_unlock_and_set (datalist, NULL);
+          g_free (d);
+          to_unlock = FALSE;
+        }
+      else
+        {
+          if (idx != d->len)
+            *data = d->data[d->len];
+        }
+    }
+  else if (data)
+    {
+      /* Update. The callback may have provided new pointers to an existing
+       * entry.
+       *
+       * The old data was stolen by callback(). We only update the pointers and
+       * are done. */
+      data->data = new_data;
+      data->destroy = new_destroy;
+    }
+  else if (!data && !new_data)
+    {
+      /* Absent. No change. The entry didn't exist and still does not. */
+    }
+  else
+    {
+      /* Add. Add a new entry that didn't exist previously. */
+      if (datalist_append (&d, key_id, new_data, new_destroy))
+        {
+          g_datalist_unlock_and_set (datalist, d);
+          to_unlock = FALSE;
+        }
+    }
+
+  if (to_unlock)
+    g_datalist_unlock (datalist);
+
+  return result;
+}
+
 /**
  * g_dataset_id_get_data:
  * @dataset_location: (not nullable): the location identifying the dataset.

glib/gdatasetprivate.h

@@ -38,6 +38,29 @@ G_BEGIN_DECLS
 #define G_DATALIST_GET_FLAGS(datalist)				\
   ((gsize) g_atomic_pointer_get (datalist) & G_DATALIST_FLAGS_MASK)
 
+/*< private >
+ * GDataListUpdateAtomicFunc:
+ * @key_id: ID of the entry to update
+ * @data: (inout) (nullable) (not optional): the existing data corresponding
+ *   to @key_id, and return location for the new value for it
+ * @destroy_notify: (inout) (nullable) (not optional): the existing destroy
+ *   notify function for @data, and return location for the destroy notify
+ *   function for the new value for it
+ * @user_data: user data passed in to [func@GLib.datalist_id_update_atomic]
+ *
+ * Callback from [func@GLib.datalist_id_update_atomic].
+ *
+ * Since: 2.80
+ */
+typedef gpointer (*GDataListUpdateAtomicFunc) (GQuark key_id,
+                                               gpointer *data,
+                                               GDestroyNotify *destroy_notify,
+                                               gpointer user_data);
+
+gpointer g_datalist_id_update_atomic (GData **datalist,
+                                      GQuark key_id,
+                                      GDataListUpdateAtomicFunc callback,
+                                      gpointer user_data);
 
 G_END_DECLS
 

glib/glib-private.c

@@ -24,6 +24,7 @@
 #include "glib-private.h"
 #include "glib-init.h"
 #include "gutilsprivate.h"
+#include "gdatasetprivate.h"
 
 #ifdef USE_INVALID_PARAMETER_HANDLER
 #include <crtdbg.h>
@@ -74,6 +75,8 @@ glib__private__ (void)
     g_uri_get_default_scheme_port,
 
     g_set_prgname_once,
+
+    g_datalist_id_update_atomic,
   };
 
   return &table;

glib/glib-private.h

@@ -23,6 +23,7 @@
 #include <glib.h>
 #include "gwakeup.h"
 #include "gstdioprivate.h"
+#include "gdatasetprivate.h"
 
 /* gcc defines __SANITIZE_ADDRESS__, clang sets the address_sanitizer
  * feature flag.
@@ -291,6 +292,11 @@ typedef struct {
   /* See gutils.c */
   gboolean (* g_set_prgname_once) (const gchar *prgname);
 
+  gpointer (*g_datalist_id_update_atomic) (GData **datalist,
+                                           GQuark key_id,
+                                           GDataListUpdateAtomicFunc callback,
+                                           gpointer user_data);
+
   /* Add other private functions here, initialize them in glib-private.c */
 } GLibPrivateVTable;
 
@@ -327,4 +333,8 @@ guint g_uint_hash (gconstpointer v);
 #undef G_THREAD_LOCAL
 #endif
 
+/* Convenience wrapper to call private g_datalist_id_update_atomic() function. */
+#define _g_datalist_id_update_atomic(datalist, key_id, callback, user_data) \
+  (GLIB_PRIVATE_CALL (g_datalist_id_update_atomic) ((datalist), (key_id), (callback), (user_data)))
+
 #endif /* __GLIB_PRIVATE_H__ */

glib/tests/dataset.c

@@ -1,6 +1,8 @@
 #include <glib.h>
 #include <stdlib.h>
 
+#include "glib/glib-private.h"
+
 static void
 test_quark_basic (void)
 {
@@ -319,6 +321,58 @@ test_datalist_id_remove_multiple_destroy_order (void)
   g_assert_cmpint (destroy_count, ==, 3);
 }
 
+static gpointer
+_update_atomic_cb (GQuark key_id,
+                   gpointer *data,
+                   GDestroyNotify *destroy_notify,
+                   gpointer user_data)
+{
+  const char *op = user_data;
+  char *data_entry = *data;
+
+  g_assert_nonnull (op);
+
+  if (strcmp (op, "create") == 0)
+    {
+      g_assert_cmpstr (data_entry, ==, NULL);
+      g_assert_null (*destroy_notify);
+
+      *data = g_strdup ("hello");
+      *destroy_notify = g_free;
+    }
+  else if (strcmp (op, "remove") == 0)
+    {
+      g_assert_cmpstr (data_entry, ==, "hello");
+      g_assert_true (*destroy_notify == g_free);
+
+      g_free (data_entry);
+      *data = NULL;
+    }
+  else
+    g_assert_not_reached ();
+
+  return "result";
+}
+
+static void
+test_datalist_update_atomic (void)
+{
+  GQuark one = g_quark_from_static_string ("one");
+  GData *list = NULL;
+  const char *result;
+
+  result = _g_datalist_id_update_atomic (&list, one, _update_atomic_cb, "create");
+  g_assert_cmpstr (result, ==, "result");
+  g_assert_cmpstr ((const char *) g_datalist_id_get_data (&list, one), ==, "hello");
+
+  g_datalist_id_set_data_full (&list, one, g_strdup ("hello"), g_free);
+
+  result = _g_datalist_id_update_atomic (&list, one, _update_atomic_cb, "remove");
+  g_assert_cmpstr (result, ==, "result");
+
+  g_assert_null (list);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -337,6 +391,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/datalist/id-remove-multiple", test_datalist_id_remove_multiple);
   g_test_add_func ("/datalist/id-remove-multiple-destroy-order",
                    test_datalist_id_remove_multiple_destroy_order);
+  g_test_add_func ("/datalist/update-atomic", test_datalist_update_atomic);
 
   return g_test_run ();
 }