From a1e9284f4766cbe3247cb398f9756b8ab3f248e3 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Tue, 13 Aug 2024 21:18:02 +0200
Subject: [PATCH] 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;
 }