Re: Changes to the GLib main loop

[Owen Taylor <otaylor redhat com>]

Sebastian Wilhelmi <wilhelmi ira uka de> writes:

> 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)

This should be safe - the thread that got woken up will always
try to claim ownership of the loop... whether this succeeds
or fails, we are guaranteed that there is a new owner for
the loop. When the new owner releases ownership, then another
waiter will be woken up... and so forth.

>  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.

> 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.
 
> 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.
 
> 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.
 
No that wasn't my intention. :-) I originally did write it that way
before your patch, but I figured I might as well try to preserve
your optimization of sharing the same condition/mutex for all
main loops and the main context.

> 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

This could be added ... I avoided doing this at the first step because
I wanted to test the public API with the default implementation.

Note that g_main_context_wait() special cases being called with
the context's mutex, realizes that the lock is already held
in this case, and acts like a _unlocked() variant.

The fixes below look OK to commit.

Regards,
                                        Owen

> --- 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);
> +    }

Well, harmless, anyways.

> @@ -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); 

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 ]

> @@ -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;
>  	}

Yep, looks right.

>        
> @@ -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;
>  	}

Yes, also something that needs fixing.