Re: Changes to the GLib main loop

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi Owen,

I'm back to normal service now. Thanks for applying the patch. That really
made my day.... (and that of ORBit-mt).

> > Yes, but having a condition variable for the loop does also mean,
> > that the context (in an iteration) would have to signal all loops for
> > that context. That sounds much more inefficient.
> 
> I guess your concern is that you replace 1 cond broadcast for N
> threads waiting with N cond broadcasts. I'm not sure there is
> a big difference in efficiency between these two options.
> 
> But actually, for the main loop case, we need only need "wake one"
> semantics not "wake all" semantics. Which could be accomplished:
> 
>  - By using g_cond_signal() on a single condition variable.
> 
>  - By picking one out of a list of condition variables and
>    calling signalling that.

That indeed should work (even though one should once more ensure that not one
thread can consume a signal and then not resend it later, leaving the waiting
threads livelocked, I'm not yet completly sure), but in the current version of
g_main_loop_run you just take the condition variable from the context meaning,
that there will be lots of GMainWaiter entries in the context which all have
the same mutex/cond combination, namely that of the context... That doesn't
sound very efficient neither.

There are 3 possibilities:

1. Drop GMainWaiter and cond/mutex from g_main_context_wait.
It still is possible to use g_main_context for your own main loops even after
that, I think. That would just be the solution, you disaproved of in the first
place. [I know, why you added that feature, such that external main loops can
use their own condition variables to be notified, so they can notify
themselfs, which wouldn't be possible otherwise, I'm not sure that is of big
use, but it might be]

2. If you want to retain that, then we should at least special case the normal
case of the context cond/mutex combination by just setting a flag and not
adding that cond to the list multiple times.

3. Most certainly your intention was to allocate a per-main-loop-condition-var
(as oposed to the current per-main-context-conditon-var). If that is the case,
just do it that way.


Also there are some typos/bugs, that I fixed and attached. Shall I commit?

Furthermore there would be the chance to make the whole thing faster by
providing more _unlocked function variants to avoid to lock/unlock to often:

These would be:

g_main_context_acquire_unlocked
g_main_context_wait_unlocked
g_main_context_release_unlocked
g_main_context_check_unlocked
g_main_context_dispatch_unlocked

Bye,
Sebastian
-- 
Sebastian Wilhelmi
mailto:wilhelmi ira uka de
http://goethe.ira.uka.de/~wilhelmi

Index: gmain.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gmain.c,v
retrieving revision 1.63
diff -u -b -B -r1.63 gmain.c
--- gmain.c	2001/07/11 20:08:48	1.63
+++ gmain.c	2001/07/16 13:51:01
@@ -1672,7 +1672,10 @@
   LOCK_CONTEXT (context);
 
   if (!context->owner)
+    {
     context->owner = self;
+      g_assert (context->owner_count == 0);
+    }
 
   if (context->owner == self)
     {
@@ -1701,8 +1704,6 @@
 g_main_context_release (GMainContext *context)
 {
 #ifdef G_THREAD_ENABLED
-  GMainWaiter *waiter_to_notify = NULL;
-
   if (context == NULL)
     context = g_main_context_default ();
   
@@ -1715,28 +1716,21 @@
 
       if (context->waiters)
 	{
-	  waiter_to_notify = context->waiters;
+	  GMainWaiter *waiter = context->waiters->data;
+	  gboolean loop_internal_waiter =
+	    (waiter->mutex == g_static_mutex_get_mutex (&context->mutex));
 	  context->waiters = g_slist_delete_link (context->waiters,
 						  context->waiters);
-	}
-    }
-
-  if (waiter_to_notify)
-    {
-      gboolean loop_internal_waiter =
-	(waiter_to_notify->mutex == g_static_mutex_get_mutex (&context->mutex));
-
       if (!loop_internal_waiter)
-	g_mutex_lock (waiter_to_notify->mutex);
+	    g_mutex_lock (waiter->mutex);
       
-      g_cond_signal (waiter_to_notify->cond);
+	  g_cond_signal (waiter->cond);
       
       if (!loop_internal_waiter)
-	g_mutex_unlock (waiter_to_notify->mutex);
-      else
-	UNLOCK_CONTEXT (context); 
+	    g_mutex_unlock (waiter->mutex);
     }
-  else
+    }
+
     UNLOCK_CONTEXT (context); 
 
   return result;
@@ -1796,7 +1790,10 @@
     }
 
   if (!context->owner)
+    {
     context->owner = self;
+      g_assert (context->owner_count == 0);
+    }
 
   if (context->owner == self)
     {
@@ -2384,7 +2381,6 @@
 	{
 	  g_warning ("g_main_loop_run() was called from second thread but"
 		     "g_thread_init() was never called.");
-	  UNLOCK_CONTEXT (loop->context);
 	  return;
 	}
       
@@ -2405,9 +2401,10 @@
       
       if (!loop->is_running)
 	{
+	  UNLOCK_CONTEXT (loop->context);
 	  if (got_ownership)
 	    g_main_context_release (loop->context);
-	  g_main_loop_unref_and_unlock (loop);
+	  g_main_loop_unref (loop);
 	  return;
 	}