Display server main control loop

Fri Apr 5 06:53:19 UTC 2013

On 04.04.2013 17:26, Alexandros Frantzis wrote:
> 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.
>>>

I think we are saying the same here :) So I'm good with your proposal.

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

Fair enough, I'm mostly convinced :)

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

Fine with me, one niggle, though: Both DisplayConfigurationEvent and
PauseResumeEvent are constructed in a factory-style, whereas StopEvent
is not. Do you think it makes sense to unify the construction pattern here?

Thanks,

  Thomas

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