Re: Changes to the GLib main loop

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi Owen,

> >  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.
> 
> Well, I don't see a problem with efficiency:
> 
>  - It's "wake one"
> 
>  - The number of entries is just the number of waiters, it's
>    not going to get large.
> 
> However, there is a fairly serious bug in the code as written
> currently it:
> 
>  - Removes a GMainWaiter from the list
>  - Calls g_cond_signal(), so one of the waiters for that context
>    is woken up.
> 
> But there is no guarantee that the waiter that gets woken up is the
> same one we removed from the list, so we might end up removing
> two GMainWaiter's from the list.
> 
> Using a cond variable per loop doesn't help, since the code (as
> written currently) allows multiple threads to call g_main_run()
> on the same loop, though I wouldn't expect that to be very
> common at all.
> 
> I believe that this bug can be simply solved by removing the
> call to g_slist_delete_link() from g_main_context_release().
> The logic here is [ please check() ]:
> 
>  * The only difference that this can make is if
>    g_main_context_release() is called again before the g_list_remove()
>    in g_main_context_wait().
> 
>  * If g_main_context_release() is called before the
>    g_list_remove() call in g_main_context_wait(), then the same
>    condition variable may be signaled again, even if there are
>    no longer any waiters for it; but this is harmless, because
>    g_main_context_wait() will attempt to get ownership, making sure
>    that g_main_context_release(), will be called, and a remaining
>    waiter for another condition variable will be woken up.

Seems to work.

> > 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]
> 
> Dropping the cond/mutex is, as far as I can see, impossible.
> 
> To reliably wait on a condition, you must check the condition
> with the mutex locked... but we do not, and should not expose the
> main context's condition through the public API.

Ah, I see. But actually it might be better to expose the mutex (as in
GAsyncQueue). Now we have a deadlock problem:

g_mutex_lock(external_mutex)
g_main_loop_wait (context, external_cond, external_mutex)
  /* This does g_mutex_lock(context->mutex) */

and in another thread

g_main_loop_release (context)
  /* This does g_mutex_lock(context->mutex) and then
     g_mutex_lock(external_mutex) */

So we could end up in dead-lock. Yes, I know, it would bloat the API, but in
the end it might be the best solution to add _lock and _unlock calls and
require the wait call to be done inside those two. We do not need to export
other _unlocked variants, even though it might make implementing own main
loops easier.

> > 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.
> 
> I don't see a big win in efficiency here.

You save a lot, I think, see attached patch. Yes I know, this is not fair
anymore, because we wake up GLib main loops first.
 
> > 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
> 
> This could be added ... I avoided doing this at the first step because
> I wanted to test the public API with the default implementation.

Ok, postponed.

> > -       waiter_to_notify = context->waiters;
> > +       GMainWaiter *waiter = context->waiters->data;

> This looks like mostly (an fine) cleanup change. If I'm correct, the
> one substantive change is to fix a problem where, if the mutex is
> not context->mutex, we leave the context locked? [ Just checking
> to make sure I understand the change ]

Yes, and also using context->waiters->data instead of context->waiters.

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

Index: glib/gmain.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gmain.c,v
retrieving revision 1.64
diff -u -b -B -r1.64 gmain.c
--- glib/gmain.c	2001/07/17 08:49:23	1.64
+++ glib/gmain.c	2001/07/17 09:12:49
@@ -94,6 +94,7 @@
   GThread *owner;
   guint owner_count;
   GSList *waiters;
+  guint internal_waiters;
 #endif  
 
   guint ref_count;
@@ -1714,19 +1715,15 @@
     {
       context->owner = NULL;
 
-      if (context->waiters)
+      if (context->internal_waiters)
 	{
+	  g_cond_signal (context->cond);
+	}
+      else if (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 (!loop_internal_waiter)
 	    g_mutex_lock (waiter->mutex);
-	  
 	  g_cond_signal (waiter->cond);
-	  
-	  if (!loop_internal_waiter)
 	    g_mutex_unlock (waiter->mutex);
 	}
     }
@@ -1773,6 +1770,14 @@
 
   if (context->owner && context->owner != self)
     {
+      if (loop_internal_waiter)
+	{
+	  context->internal_waiters++;
+	  g_cond_wait (cond, mutex);
+	  context->internal_waiters--;
+	}
+      else
+	{
       GMainWaiter waiter;
 
       waiter.cond = cond;
@@ -1780,13 +1785,12 @@
 
       context->waiters = g_slist_append (context->waiters, &waiter);
       
-      if (!loop_internal_waiter)
 	UNLOCK_CONTEXT (context);
       g_cond_wait (cond, mutex);
-      if (!loop_internal_waiter)      
 	LOCK_CONTEXT (context);
 
       context->waiters = g_slist_remove (context->waiters, &waiter);
+	}
     }
 
   if (!context->owner)