Re: Changes to the GLib main loop

[Owen Taylor <otaylor redhat com>]

Sebastian Wilhelmi <wilhelmi ira uka de> writes:

> while trying to port ORBit-mt to the new GLib main loop I noticed, that the
> one context per thread concept really isn't of great use. Instead I would like
> the GMainContext to be a object of its own. There are several reasons:
> 
> 1) I can add sources to a context and only then create a thread, where a 
>    main loop for that context is running (not possible now).
> 2) I would want to be able to run two context's in the same thread, for
>    example, if my GTK/ORBit program is inactive the context with both the 
>    ORBit and the GTK fds is running, if a ORBit request is in progress
>    a context with only the ORBit fds is running to just wait for the reply 
>    and to avoid reentrance from GTK.

I'm glad you had a chance to try out the new main loop code in
detail. I basically wrote the multiple main loop support without
a real live example in mind, so some testing in practice is definitely
a good thing.

In concept, I'm not opposed to the idea of allowing GMainContext's
not to be restricted to running in a single thread; however,
there are some definite implementation challenges there that I'm
not sure that your code addresses.

> For multithreaded programs there is the added advantage, that you can use the
> usual idioms in other threads as well (though they will do nothing then, just
> wait, whenever may_block is true.):
> 
> while (g_main_context_pending(context))
>   g_main_context_iteration(context, may_block)

But the way you've written it, this doesn't do at all what people
expect.

What people will try to use this for is:

 - Main thread is blocking in a long computation
 - To continue processing events, iterate the main loop from
   a different thread.

But that doesn't work since g_main_context_iteration() will just wait
for the main thread. So allowing someone to write this is just
misleading them. 

If we actually want to allow people to call g_main_context_iteration()
from multiple threads, then we have to implement it so that if one
thread is stuck in dispatch() then another thread will recursively
iterate; if the first thread is however, blocking in poll(), or is
doing anything other than dispatching, then it has to simply
wait. 

This is conceptually possible, but quite a pain to implement, which is
one reason I didn't do it. (Remember, the context lock is _not_ held
over prepare/query/check.) There are also questions of what happens to
outstanding dispatches when multiple threads are iterating on the main
loop. A recursive invocation from a single loop simply discards
outstanding dispatches and starts over, but this clearly isn't the
right behavior for multiple threads.

Possibly the right behavior is for the threads to dispatch requests
round robin, and for the first free thread to start a new cycle,
ignoring sources currently being dispatched. This would need
_very_ careful definition, and the combination of this and
recursion within threads presents a highly complex situation.


The context->owner mechanism presents some problems with GMainLoop. 
Consider what happens when:

 - Thread A calls g_main_context_iteration()
 - Before that returns, thread B calls g_main_run() on the same thread
 - Thread A returns and starts a long computation

Becaus Thread A was the owner when the main loop started, thread B
will continue waiting forever for some other thread to run the
main loop. Since creation of a context doesn't identify the owner,
we introduce the potential of race conditions for ownership of
a thread.

Also, using g_main_loop_run() to wait for, say, a modal dialog,
in a second thread has become _much_ less efficient since you
are waking up the second thread via g_cond_broadcast() on
_every_ iteration. In face, I'd consider this to be an 
unacceptable performance hit. The reason why each GMainLoop 
currently has a separate condition variable is to avoid this.


A final general consideration with your changes is that you
have effectively made the public functions:

  gboolean g_main_context_prepare  (GMainContext *context,
                                    gint         *priority);
  gint     g_main_context_query    (GMainContext *context,
                                    gint          max_priority,
                                    gint         *timeout,
                                    GPollFD      *fds,
                                    gint          n_fds);
  gint     g_main_context_check    (GMainContext *context,
                                    gint          max_priority,
                                    GPollFD      *fds,
                                    gint          n_fds);
  void     g_main_context_dispatch (GMainContext *context);

Not usuable, because g_main_context_iterate() manipulates
the context structure directly in ways that external code
can't. If you look at the current implementation of
g_main_context_iterate(), you'll see that the only direct
use of the GMainContext structure is to cache the GPollFD
array.

One of things I wanted to achieve going into my main loop changes
for GLib-2.0 was that people would be able to integrate the GLib
main loop with other main loops that worked in different
ways. This is the reason for making these functions public,
and I think you're changes break this ability.

> - The loop is locked with the context lock always. This saves some
>   locks/unlocks and because the main loop has exactly one context
>   attached to it its whole lifetime it also works.

If you use private API's between GMainLoop and GMainContext,
yes, this is possible, and perhaps a good idea.

> - g_main_context_iterate handles cached_poll_array more inteligent. The
>   old solution looked quite doubios to me.

I'm not sure your handling is "more intelligent". It is certainly
simpler. My initial reasoning for the complexity was that the
fd array needed to remain the same over the entire 
prepare/query/check() sequence; the code was meant to ensure that.