Re: atomic reference counting for gmain.c

[Sebastian Wilhelmi <seppi seppi de>]

Hi,

> the attached patch uses the new g_atomic_int_(inc|dec_and_test)
> functions for atomic reference counting in gmain.c
> 
> GMainLoop and GMainContext are ported over. GSource can't be ported,
> beacuse it uses guint for the reference count (like GMainLoop and
> GMainContext) and is defined in a header file (unlike GMainLoop and
> GMainContext) and we can't change structures in headers (that would give
> us unstable ABI).
> 
> En passant I also fixed #134877.

New patch with g_atomic_int_get all over the place. 
While it can be argued, whether we need g_atomic_int_get in
g_return_if_fail (We could only possibly find a positive ref_count,
where indeed it is already zero), in the last usage in
child_watch_helper_thread in mandatory.

Ciao,
Sebastian
-- 
Sebastian Wilhelmi                 |            här ovanför alla molnen
mailto:seppi seppi de              |     är himmlen så förunderligt blå
http://seppi.de                    |

Index: glib/gmain.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gmain.c,v
retrieving revision 1.105
diff -u -r1.105 gmain.c
--- glib/gmain.c	25 Feb 2004 23:48:21 -0000	1.105
+++ glib/gmain.c	27 Feb 2004 17:05:45 -0000
@@ -103,7 +103,7 @@
   GSList *waiters;
 #endif  
 
-  guint ref_count;
+  gint32 ref_count;
 
   GPtrArray *pending_dispatches;
   gint timeout;			/* Timeout for current iteration */
@@ -153,7 +153,7 @@
 {
   GMainContext *context;
   gboolean is_running;
-  guint ref_count;
+  gint32 ref_count;
 };
 
 struct _GTimeoutSource
@@ -587,13 +587,9 @@
 g_main_context_ref (GMainContext *context)
 {
   g_return_if_fail (context != NULL);
-  g_return_if_fail (context->ref_count > 0); 
-
-  LOCK_CONTEXT (context);
-  
-  context->ref_count++;
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0); 
 
-  UNLOCK_CONTEXT (context);
+  g_atomic_int_inc (&context->ref_count);
 }
 
 /* If DISABLE_MEM_POOLS is defined, then freeing the
@@ -616,18 +612,22 @@
 }
 #endif /* DISABLE_MEM_POOLS */
 
-static void
-g_main_context_unref_and_unlock (GMainContext *context)
+/**
+ * g_main_context_unref:
+ * @context: a #GMainContext
+ * 
+ * Decreases the reference count on a #GMainContext object by one. If
+ * the result is zero, free the context and free all associated memory.
+ **/
+void
+g_main_context_unref (GMainContext *context)
 {
   GSource *source;
+  g_return_if_fail (context != NULL);
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0); 
 
-  context->ref_count--;
-
-  if (context->ref_count != 0)
-    {
-      UNLOCK_CONTEXT (context);
-      return;
-    }
+  if (!g_atomic_int_dec_and_test (&context->ref_count))
+    return;
 
   G_LOCK (main_context_list);
   main_context_list = g_slist_remove (main_context_list, context);
@@ -640,7 +640,6 @@
       g_source_destroy_internal (source, context, TRUE);
       source = next;
     }
-  UNLOCK_CONTEXT (context);
 
 #ifdef G_THREADS_ENABLED  
   g_static_mutex_free (&context->mutex);
@@ -675,23 +674,6 @@
   g_free (context);
 }
 
-/**
- * g_main_context_unref:
- * @context: a #GMainContext
- * 
- * Decreases the reference count on a #GMainContext object by one. If
- * the result is zero, free the context and free all associated memory.
- **/
-void
-g_main_context_unref (GMainContext *context)
-{
-  g_return_if_fail (context != NULL);
-  g_return_if_fail (context->ref_count > 0); 
-
-  LOCK_CONTEXT (context);
-  g_main_context_unref_and_unlock (context);
-}
-
 #ifdef G_THREADS_ENABLED
 static void 
 g_main_context_init_pipe (GMainContext *context)
@@ -2492,31 +2474,13 @@
 g_main_loop_ref (GMainLoop *loop)
 {
   g_return_val_if_fail (loop != NULL, NULL);
-  g_return_val_if_fail (loop->ref_count > 0, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&loop->ref_count) > 0, NULL);
 
-  LOCK_CONTEXT (loop->context);
-  loop->ref_count++;
-  UNLOCK_CONTEXT (loop->context);
+  g_atomic_int_inc (&loop->ref_count);
 
   return loop;
 }
 
-static void
-g_main_loop_unref_and_unlock (GMainLoop *loop)
-{
-  loop->ref_count--;
-  if (loop->ref_count == 0)
-    {
-      /* When the ref_count is 0, there can be nobody else using the
-       * loop, so it is safe to unlock before destroying.
-       */
-      g_main_context_unref_and_unlock (loop->context);
-  g_free (loop);
-    }
-  else
-    UNLOCK_CONTEXT (loop->context);
-}
-
 /**
  * g_main_loop_unref:
  * @loop: a #GMainLoop
@@ -2528,11 +2492,13 @@
 g_main_loop_unref (GMainLoop *loop)
 {
   g_return_if_fail (loop != NULL);
-  g_return_if_fail (loop->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&loop->ref_count) > 0);
 
-  LOCK_CONTEXT (loop->context);
-  
-  g_main_loop_unref_and_unlock (loop);
+  if (!g_atomic_int_dec_and_test (&loop->ref_count))
+    return;
+
+  g_main_context_unref (loop->context);
+  g_free (loop);
 }
 
 /**
@@ -2550,7 +2516,7 @@
   GThread *self = G_THREAD_SELF;
 
   g_return_if_fail (loop != NULL);
-  g_return_if_fail (loop->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&loop->ref_count) > 0);
 
 #ifdef G_THREADS_ENABLED
   if (!g_main_context_acquire (loop->context))
@@ -2567,7 +2533,7 @@
       
       LOCK_CONTEXT (loop->context);
 
-      loop->ref_count++;
+      g_atomic_int_inc (&loop->ref_count);
 
       if (!loop->is_running)
 	loop->is_running = TRUE;
@@ -2602,7 +2568,7 @@
       return;
     }
 
-  loop->ref_count++;
+  g_atomic_int_inc (&loop->ref_count);
   loop->is_running = TRUE;
   while (loop->is_running)
     g_main_context_iterate (loop->context, TRUE, TRUE, self);
@@ -2627,7 +2593,7 @@
 g_main_loop_quit (GMainLoop *loop)
 {
   g_return_if_fail (loop != NULL);
-  g_return_if_fail (loop->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&loop->ref_count) > 0);
 
   LOCK_CONTEXT (loop->context);
   loop->is_running = FALSE;
@@ -2653,7 +2619,7 @@
 g_main_loop_is_running (GMainLoop *loop)
 {
   g_return_val_if_fail (loop != NULL, FALSE);
-  g_return_val_if_fail (loop->ref_count > 0, FALSE);
+  g_return_val_if_fail (g_atomic_int_get (&loop->ref_count) > 0, FALSE);
 
   return loop->is_running;
 }
@@ -2670,7 +2636,7 @@
 g_main_loop_get_context (GMainLoop *loop)
 {
   g_return_val_if_fail (loop != NULL, NULL);
-  g_return_val_if_fail (loop->ref_count > 0, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&loop->ref_count) > 0, NULL);
  
   return loop->context;
 }
@@ -2776,7 +2742,7 @@
   if (!context)
     context = g_main_context_default ();
   
-  g_return_if_fail (context->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0);
   g_return_if_fail (fd);
 
   LOCK_CONTEXT (context);
@@ -2848,7 +2814,7 @@
   if (!context)
     context = g_main_context_default ();
   
-  g_return_if_fail (context->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0);
   g_return_if_fail (fd);
 
   LOCK_CONTEXT (context);
@@ -2950,7 +2916,7 @@
   if (!context)
     context = g_main_context_default ();
   
-  g_return_if_fail (context->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0);
 
   LOCK_CONTEXT (context);
   
@@ -2984,7 +2950,7 @@
   if (!context)
     context = g_main_context_default ();
   
-  g_return_val_if_fail (context->ref_count > 0, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&context->ref_count) > 0, NULL);
 
   LOCK_CONTEXT (context);
   result = context->poll_func;
@@ -3024,7 +2990,7 @@
   if (!context)
     context = g_main_context_default ();
   
-  g_return_if_fail (context->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0);
 
   LOCK_CONTEXT (context);
   g_main_context_wakeup_unlocked (context);
@@ -3392,15 +3358,20 @@
 #endif
 
       /* We were woken up.  Wake up all other contexts in all other threads */
-      G_UNLOCK (main_context_list);
+      G_LOCK (main_context_list);
       for (list = main_context_list; list; list = list->next)
 	{
 	  GMainContext *context;
 
 	  context = list->data;
-	  g_main_context_wakeup (context);
+	  if (g_atomic_int_get (&context->ref_count) > 0)
+	    /* Due to racing conditions we can find ref_count == 0, in
+	     * that case, however, the context is still not destroyed
+	     * and no poll can be active, otherwise the ref_count
+	     * wouldn't be 0 */
+	    g_main_context_wakeup (context);
 	}
-      G_LOCK (main_context_list);
+      G_UNLOCK (main_context_list);
     }
   return NULL;
 }