From a4a20a101be621830d5b28ef0b44ff0f7e647e7e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Feb 2025 17:29:12 +0100 Subject: [PATCH 1/3] gdataset: mark freeing of d_to_free buffer as unlikely This only happens during a shrinking of the buffer, which only happens after a remove causes the array to become only 25% filled. Mark those code paths as unlikely. --- glib/gdataset.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index c18e13f06..b4f60f78b 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -636,7 +636,7 @@ g_data_set_internal (GData **datalist, if (dataset && !d) g_dataset_destroy_internal (dataset); - if (d_to_free) + if (G_UNLIKELY (d_to_free)) g_free (d_to_free); } else @@ -782,7 +782,7 @@ g_datalist_id_remove_multiple (GData **datalist, if (found_keys > 0 && datalist_shrink (&d, &d_to_free)) { g_datalist_unlock_and_set (datalist, d); - if (d_to_free) + if (G_UNLIKELY (d_to_free)) g_free (d_to_free); } else @@ -1127,7 +1127,7 @@ g_datalist_id_update_atomic (GData **datalist, if (datalist_shrink (&d, &d_to_free)) { g_datalist_unlock_and_set (datalist, d); - if (d_to_free) + if (G_UNLIKELY (d_to_free)) g_free (d_to_free); to_unlock = FALSE; } @@ -1376,7 +1376,7 @@ g_datalist_id_replace_data (GData **datalist, else g_datalist_unlock (datalist); - if (d_to_free) + if (G_UNLIKELY (d_to_free)) g_free (d_to_free); return val == oldval; From a1e9284f4766cbe3247cb398f9756b8ab3f248e3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Aug 2024 21:18:02 +0200 Subject: [PATCH 2/3] gdataset: reorder epilogue code path in g_datalist_id_update_atomic() The most common case is that the callback does not change the data/destroy pointers. Reorder the "if" checks to favor those cases. g_datalist_id_update_atomic() makes sense to track some data structure, and atomically update it. But in that case, we usually update the values inside the structure, and don't reset the pointer to the structure itself. That means, if old data was present, then new_data is likley unchanged. With this change, we instead check whether the values are unchanged and avoid resetting the (same) value. The idea is in the common case to avoid writing the same value to memory to avoid false sharing or invalidating CPU caches. --- glib/gdataset.c | 75 +++++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index b4f60f78b..b29042c54 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -1097,7 +1097,6 @@ g_datalist_id_update_atomic (GData **datalist, gpointer result; GDestroyNotify new_destroy; guint32 idx; - gboolean to_unlock = TRUE; d = g_datalist_lock_and_get (datalist); @@ -1116,49 +1115,59 @@ g_datalist_id_update_atomic (GData **datalist, result = callback (&new_data, &new_destroy, user_data); - if (data && !new_data) + if (G_LIKELY (data)) { - GData *d_to_free; - - /* Remove. The callback indicates to drop the entry. - * - * The old data->data was stolen by callback(). */ - datalist_remove (d, idx); - if (datalist_shrink (&d, &d_to_free)) + if (G_LIKELY (data->data == new_data && data->destroy == new_destroy)) { - g_datalist_unlock_and_set (datalist, d); - if (G_UNLIKELY (d_to_free)) - g_free (d_to_free); - to_unlock = FALSE; + /* No change. */ + } + else if (!new_data) + { + GData *d_to_free; + + /* Remove. The callback indicates to drop the entry. + * + * The old data->data was stolen by callback(). */ + datalist_remove (d, idx); + if (datalist_shrink (&d, &d_to_free)) + { + g_datalist_unlock_and_set (datalist, d); + if (G_UNLIKELY (d_to_free)) + g_free (d_to_free); + goto return_without_unlock; + } + } + else + { + /* 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) - { - /* 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)) + if (G_LIKELY (!new_data)) { - g_datalist_unlock_and_set (datalist, d); - to_unlock = FALSE; + /* 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); + goto return_without_unlock; + } } } - if (to_unlock) - g_datalist_unlock (datalist); + g_datalist_unlock (datalist); +return_without_unlock: return result; } From 4a9d93d7eb9029c704bb7557acb8f1584e8521c3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Feb 2025 17:34:22 +0100 Subject: [PATCH 3/3] gdataset: refactor check for old value in g_datalist_id_replace_data() GData does not support to track NULL values. That means, the existing value is NULL if-and-only-if the GDataElt is NULL. Thus the check for `if (val == NULL && ...` is already covered by `if (data)` above, and we can move that to the else branch. --- glib/gdataset.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index b29042c54..66b50e22e 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -1371,12 +1371,12 @@ g_datalist_id_replace_data (GData **datalist, } } } - - if (val == NULL && oldval == NULL && newval != NULL) + else { - if (datalist_append (&d, key_id, newval, destroy)) + if (oldval == NULL && newval != NULL) { - set_d = TRUE; + if (datalist_append (&d, key_id, newval, destroy)) + set_d = TRUE; } }