Display server main control loop

Thu Apr 4 15:26:04 UTC 2013

On Thu, Apr 04, 2013 at 11:25:19AM +0200, Thomas Voß wrote:
> On 04.04.2013 10:41, Alexandros Frantzis wrote:
> > On Thu, Apr 04, 2013 at 08:32:05AM +0200, Thomas Voß wrote:
> >> On 03.04.2013 17:51, Alexandros Frantzis wrote:
> >>>
> >>> To get a better feeling of what's involved, I created a prototype
> >>> abstraction for a main loop and an implementation using boost asio.
> >>> Get it and use it with:
> >>>
> >>> $ bzr branch lp:~afrantzis/+junk/mainloop
> >>> $ cd mainloop && make
> >>> $ ./mainloop
> >>>
> >>> The main loop abstraction was designed so that event creation and
> >>> handling are completely dissociated. The display server (or other
> >>> consumer) can handle an event without caring if it is coming from a
> >>> signal, a read from a fd, or another mechanism.  So, we could have
> >>> something like this in the mir main loop:
> >>>
> >>
> >> This looks pretty good to me, some thoughts on the separation of
> >> concerns, though:
> >>> auto main_loop = main_loop_factory->create_main_loop();
> >>>
> >>> auto pause_event = platform->create_pause_event(main_loop_factory);
> >>
> >> I would rather prefer:
> >>
> >> PauseEvent pause_event(main_loop);
> >> pause_event.async_wait(pause_handler);
> >>
> >> QuitEvent quit_event(main_loop);
> >> quit_event.async_wait(quit_handler);
> >>
> >> DisplayConfigurationEvent disp_config_event(main_loop);
> >> disp_config_event.async_wait(disp_conf_handler);
> > 
> > The problem is that the way many events are triggered is platform
> > dependent. For example, a display configuration event may be triggered
> > by data being available on an FD on the desktop (from udev), and
> > something completely different on Android.  That's the reason I opted
> > for asking components to give us event object, since they have all the
> > information to set this up.
> > 
> 
> Okay, the idea here is: "Client" code expresses an interest in a certain
> event, say "DisplayReconfigurationEvent" and it's not a concern of the
> code expressing interest in the event to know where this event might
> originate from. That is, I would expect the respective Event classes
> itself to know how to set themselves up and talk to the
> platform-specific components. With that, we would achieve a strong
> separation of concerns, while keeping platform-specific tasks local to
> the Event classes.

I find that we are disconnecting related entities too much this way. If
a component is responsible for producing an event, it should be the one
giving out event handles. Having "free" event objects essentially means
hiding a dependency (between the producer of the event and the event
object) that should be explicit.

Also, with the "free" objects it's still not clear to me how we will
handle different implementations on different platforms. Would this be
handled at build time (if PLATFORM = "android" build
pause_event_android.cpp else build pause_event_gbm.cpp) ?

> > In the scheme you propose, would different triggers be handled by having
> > a different version of EventService for each platform?
> > 
> >> See boost::asio::signal_set as an example: Different parties interested
> >> in a certain set of signals just setup their respective signal set and
> >> execute an (async) wait on the respective set. Behind the scenes, the
> >> signal_set class sets up all of the handlers, and associates all of the
> >> operation with one io_service, which forces handler execution to the
> >> thread that the io_service.run method is executed on.
> >>
> >> To this end, a mir::EventService would need to be created, and the
> >> respective events, including custom handler types (not only void()), are
> >> known to the EventService. On startup, the service is registered with
> >> the io_service representing the main event loop with
> >> boost::asio::add_service<mir::EventService>(io_service).
> >>
> >> A class QuitEvent would then do:
> >>
> >> class QuitEvent
> >> {
> >> public:
> >> 	QuitEvent(const std::shared_ptr<mir::MainLoop>& main_loop) :
> >> io_service(main_loop->io_service()) {}
> >>
> >> 	void async_wait(QuitEventHandler handler)
> >> 	{
> >> 	
> >> boost::asio::use_service<mir::EventService>(io_service).enqueue_handler_for_quit_event(handler);
> >> 	}
> >> private:
> >> 	boost::asio::io_service& io_service;
> >> };
> >>
> >> From my perspective, keeping the handler definition local, without
> >> encoding everything within the main loop avoids tight coupling, and the
> >> main loop is stripped down to only the synchronization concern.
> > 
> > What you are proposing is more versatile, but I see tighter coupling as
> > something desirable in this case. The main control loop is not supposed
> > to be a generic event loop, it's only about events that need to change
> > the state of the main components of the display server in a coordinated
> > manner. As such, the handlers need to defined at a level where all the
> > information is available, and that is at the control loop level. I don't
> > want to allow event handler registration away from the main loop, since
> > this is not useful for our use cases, and will make the code more
> > difficult to reason about.
> > 
> 
> From my perspective, there is a difference between the concern of
> synchronization and the concern of communicating specific events to
> components interested in them. In this respect, I do agree with your
> intention to avoid a generic event loop. What I'm proposing is
> basically: We have a generic reactor (boost::asio::io_service) that is
> concerned with synchronization and we can add specific services to
> specific instances of the reactor, for example a
> mir::display::ConfigurationEventService.

I am not convinced that using this model would be an win in our case.
As noted before, it's versatile, but at the same time more indirect and
complicated that we need (IMO).

Furthermore, if we go for this approach, we are going to build too tightly
around the boost asio model, whereas I am going for a more abstracted
approach. If decide at some point to change to another main loop
mechanism, we will need to change all of our event classes. With the
approach in the branch (and the latest changes, see below) custom events
use the lower-level MainLoopFactory services, which can be changed
easily at any time.

>
> > The model I am proposing is that components offer interesting events
> > which are independent of any particular main loop, and at the main
> > control loop level we decide if/how to handle them. So, e.g., the
> > platforms tells us: "I am producing display configuration events, use
> > this event object if you want to handle them".
> > 
> > I would like to support events that provide more information (i.e. have
> > a custom handler) and I am currently thinking about how to go about
> > this, while still elegantly allowing different triggers depending on the
> > platform.
> 
> I think my proposal addresses your requirement quite elegantly in that
> the Event carries the platform-independent information but knows
> internally how to wire itself up to the platform-specific pieces.
> Components interested in the event then are able to say: I'm interested
> in this event and would like to wait for it, not knowing anything about
> something platform-specific.

I have updated lp:~afrantzis/+junk/mainloop to support custom events
(i.e. events with extra data, see TextEvent in component.*). The idea is
to build on the lower-level infrastructure offered by the
MainLoopFactory (signal and fd events) layer to provide custom events.

If we use custom events for everything then the lower-level event
infrastructure is not used at all in the main loop, and the code becomes
something like:

auto main_loop = main_loop_factory->create_main_loop();

StopEvent stop_event{main_loop_factory}; // StopEvent is local to the main loop
DispConfEvent disp_event = platform->create_disp_conf_event(main_loop_factory);
PauseResumeEvent pause_resume_event = platform->create_pause_resume_event(main_loop_factory);

stop_event.register_handler(main_loop, [main_loop]() { main_loop->stop(); });
disp_event->register_handler(main_loop, []() { ... });
pause_resume_event->register_handler(main_loop, [...](bool pause) { if (pause) ... else ... });

main_loop->run();

Thanks,
Alexandros