From 0c06a4b7a0b779cade6ba4040a1820d17951fc2c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 Jan 2024 20:46:27 +0100 Subject: [PATCH] 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 | 113 +++++++++++++++++++++++++++++++++++++++++ glib/gdatasetprivate.h | 23 +++++++++ glib/glib-private.c | 3 ++ glib/glib-private.h | 10 ++++ glib/tests/dataset.c | 55 ++++++++++++++++++++ 5 files changed, 204 insertions(+) diff --git a/glib/gdataset.c b/glib/gdataset.c index 12c77b959..cebca8fb6 100644 --- a/glib/gdataset.c +++ b/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. diff --git a/glib/gdatasetprivate.h b/glib/gdatasetprivate.h index f22cf381f..c53455a06 100644 --- a/glib/gdatasetprivate.h +++ b/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 diff --git a/glib/glib-private.c b/glib/glib-private.c index c2b68a060..27deba46a 100644 --- a/glib/glib-private.c +++ b/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 @@ -74,6 +75,8 @@ glib__private__ (void) g_uri_get_default_scheme_port, g_set_prgname_once, + + g_datalist_id_update_atomic, }; return &table; diff --git a/glib/glib-private.h b/glib/glib-private.h index 479ebb9df..cb656461a 100644 --- a/glib/glib-private.h +++ b/glib/glib-private.h @@ -23,6 +23,7 @@ #include #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__ */ diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index 7541268d6..9226b5132 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -1,6 +1,8 @@ #include #include +#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 (); }