rfc: alleviate GType write locking on ref/unref

[Jindrich Makovicka <jindrich makovicka itonis tv>]

Hello,

we are currently working on a multithreaded application using GStreamer,
and one of the most annoying performance issues we have is the lock
contention of the GType write lock during creation and deletion of an
instance. As the GStreamer buffers are GType instances, and most of them
are only temporary, created at the beginning of the pipeline and
destroyed when arriving at the end, it means these rw locks are under a
heavy load.

The attached patch attempts to relieve this lock pressure by allowing
atomic modifications of class ref_count only with a read lock. I am
however not sure if it doesn't fatally break things. Can someone with a
better knowledge of GType/GObject internals comment on it?

Best regards,
-- 
Jindrich Makovicka

--- gtype.c.orig	2005-12-01 17:33:51.000000000 +0100
+++ gtype.c	2006-05-25 08:50:00.000000000 +0200
@@ -237,7 +237,7 @@
 };
 struct _CommonData
 {
-  guint             ref_count;
+  gint              ref_count;
   GTypeValueTable  *value_table;
 };
 struct _IFaceData
@@ -1944,7 +1944,7 @@
   GTypeClass *class = cdata->class;
   TypeNode *bnode;
   
-  g_assert (cdata->class && cdata->common.ref_count == 0);
+  g_assert (cdata->class && g_atomic_int_get(&cdata->common.ref_count) == 0);
   
   if (cdata->class_finalize)
     cdata->class_finalize (class, (gpointer) cdata->class_data);
@@ -1991,7 +1991,7 @@
 	  G_READ_UNLOCK (&type_rw_lock);
 	  need_break = cache_func (cache_data, node->data->class.class);
 	  G_READ_LOCK (&type_rw_lock);
-	  if (!node->data || node->data->common.ref_count == 0)
+	  if (!node->data || g_atomic_int_get(&node->data->common.ref_count) == 0)
 	    INVALID_RECURSION ("GType class cache function ", cache_func, NODE_NAME (node));
 	  if (need_break)
 	    break;
@@ -2000,15 +2000,11 @@
       G_WRITE_LOCK (&type_rw_lock);
     }
   
-  if (node->data->common.ref_count > 1)	/* may have been re-referenced meanwhile */
-    node->data->common.ref_count -= 1;
-  else
+  if (g_atomic_int_dec_and_test(&node->data->common.ref_count))
     {
       GType ptype = NODE_PARENT_TYPE (node);
       TypeData *tdata;
       
-      node->data->common.ref_count = 0;
-      
       if (node->is_instantiatable)
 	{
 	  /* destroy node->data->instance.mem_chunk */
@@ -2327,17 +2323,17 @@
   
   /* optimize for common code path
    */
-  G_WRITE_LOCK (&type_rw_lock);
+  G_READ_LOCK (&type_rw_lock);
   node = lookup_type_node_I (type);
-  if (node && node->is_classed && node->data &&
-      node->data->class.class && node->data->common.ref_count > 0)
+  if (node && node->is_classed && node->data && node->data->class.class)
     {
-      type_data_ref_Wm (node);
-      G_WRITE_UNLOCK (&type_rw_lock);
-      
+      g_atomic_int_inc(&node->data->common.ref_count);
+      G_READ_UNLOCK (&type_rw_lock);
       return node->data->class.class;
     }
-  
+  G_READ_UNLOCK (&type_rw_lock);
+
+  G_WRITE_LOCK (&type_rw_lock);
   if (!node || !node->is_classed ||
       (node->data && node->data->common.ref_count < 1))
     {
@@ -2379,14 +2375,40 @@
   g_return_if_fail (g_class != NULL);
   
   node = lookup_type_node_I (class->g_type);
-  G_WRITE_LOCK (&type_rw_lock);
-  if (node && node->is_classed && node->data &&
-      node->data->class.class == class && node->data->common.ref_count > 0)
-    type_data_unref_Wm (node, FALSE);
+  G_READ_LOCK (&type_rw_lock);
+  if (node && node->is_classed && node->data && node->data->class.class == class)
+    {
+      if (g_atomic_int_dec_and_test(&node->data->common.ref_count))
+        {
+          /* we decremented 1->0 ; revert the counter for last_unref */
+          g_atomic_int_inc(&node->data->common.ref_count);
+          G_READ_UNLOCK (&type_rw_lock);
+          G_WRITE_LOCK (&type_rw_lock);
+          if (g_atomic_int_get(&node->data->common.ref_count) == 1)
+            {
+              if (!node->plugin)
+                {
+                  g_warning ("static type `%s' unreferenced too often",
+                             NODE_NAME (node));
+                  return;
+                }
+              type_data_last_unref_Wm (NODE_TYPE (node), FALSE);
+              G_WRITE_UNLOCK (&type_rw_lock);
+              return;
+            }
+          /* someone referenced the class meanwhile - re-decrement;
+             here's no need to test, we hold the write lock */
+          g_atomic_int_dec_and_test(&node->data->common.ref_count);
+          G_WRITE_UNLOCK (&type_rw_lock);
+          return;
+        }
+    }
   else
-    g_warning ("cannot unreference class of invalid (unclassed) type `%s'",
-	       type_descriptive_name_I (class->g_type));
-  G_WRITE_UNLOCK (&type_rw_lock);
+    {
+      g_warning ("cannot unreference class of invalid (unclassed) type `%s'",
+                 type_descriptive_name_I (class->g_type));
+    }
+  G_READ_UNLOCK (&type_rw_lock);
 }
 
 void
@@ -2586,7 +2608,7 @@
   G_WRITE_LOCK (&type_rw_lock);
   if (node && NODE_IS_IFACE (node) &&
       node->data->iface.dflt_vtable == g_iface &&
-      node->data->common.ref_count > 0)
+      g_atomic_int_get(&node->data->common.ref_count) > 0)
     type_data_unref_Wm (node, FALSE);
   else
     g_warning ("cannot unreference invalid interface default vtable for '%s'",
@@ -3215,7 +3237,7 @@
  restart_check:
   if (node)
     {
-      if (node->data && node->data->common.ref_count > 0 &&
+      if (node->data && g_atomic_int_get(&node->data->common.ref_count) > 0 &&
 	  node->data->common.value_table->value_init)
 	tflags = GPOINTER_TO_UINT (type_get_qdata_L (node, static_quark_type_flags));
       else if (NODE_IS_IFACE (node))
@@ -3278,7 +3300,7 @@
   G_READ_LOCK (&type_rw_lock);
   
  restart_table_peek:
-  has_refed_data = node && node->data && node->data->common.ref_count;
+  has_refed_data = node && node->data && g_atomic_int_get(&node->data->common.ref_count);
   has_table = has_refed_data && node->data->common.value_table->value_init;
   if (has_refed_data)
     {
@@ -3528,7 +3550,7 @@
   if (NODE_PARENT_TYPE (private_node))
     {
       parent_node = lookup_type_node_I (NODE_PARENT_TYPE (private_node));
-      g_assert (parent_node->data && parent_node->data->common.ref_count);
+      g_assert (parent_node->data && g_atomic_int_get(&parent_node->data->common.ref_count));
 
       if (G_UNLIKELY (private_node->data->instance.private_size == parent_node->data->instance.private_size))
 	{