[boost] ANN: review of FSM library starts

From: Alexander Nasonov (alnsn_at_[hidden])
Date: 2005-03-09 18:49:06

Pavel Vozenilek wrote:
> Review of Finite State Machine library written
> by Andreas Huber starts today, February 21,
> and lasts 10 days.

I hope it's not too late to submit a review.

>     * What is your evaluation of the documentation?
>       How easy (or hard) it is to understand library
>       features? What can be improved?

First time I read tutorial long before the review. The tutorial
helped me a lot to enter an area of statecharts quickly.
I'm still fsm/statecharts newbie though.

Most things are quite understandable. However, code snipsets are quite
complex. On my first reading, I stopped trying to understand them after
a second of third sample.
May be it's only me who reads complex stuff in a such way but the fact
that simple_state class accepts up to five parameters gives a hint to
a user that there shall be not_so_simple_state class that accepts
6+ or is more complex in other ways.
This appears at the beginning and I suspect some will stop reading
the tutorial right at that point.
I'd suggest using some smart intendation at least.

>     * What is your evaluation of the design?
>       What features are supported better by
>       alternative FSMs? What could be added
>       (or removed) from the library?

I don't like the design. Primary goal of the library is to be
close to UML and the secondary goal is performance. UML
conformance is pushed so strongly that any attempt to improve
performance makes design worse. For example, deferral<XXX>
requires that event is allocated using new and pointed by
intrusive_ptr. Ordinary events can be allocated differently.
If performance was out of library scope, the library could copy
_all_ events into heap-allocated buffer. From user point of view,
all event kinds looked the same, then.
I believe that performance is not so important because even optimized,
the library is too slow for FSM. According to docs, process_event
function has 10 steps and there is a search in a couple of steps.
This means loops and branching. I always thought that process_event
in typical FSM is single jump followed by indirect function call.

There are other things I don't like. One example is that transit call
in react is similar to "delete this;"
The  "Unstable state" and "Unstable state machine" sections are too
among things I don't like. They exist because transition is a complex
process which involves multiple destructions and constructions.
Not only it affects performance but it complicates recovering
from exception.

>     * The library documentation contains
>       few not yet solved issues (name,
>       separating the library into two parts,
>       exception handling). What is you opinion here?

I'm stongly against name FSM because the library doesn't give enough
guarantees common in FSM world.
Because the library is not predicitable in real-time terms and because
FSM and RT systems often come together, this name can confuse some users.
Boost.Statecharts would be much better, in my opinion.

>     * What is your evaluation of the implementation?
>       Are there parts of code too obscure or
>       duplicating exiting Boost functionality?
>       Can something be factored out to standalone
>       library or among general utilities?

I only searched for words virtual, typeid and throw. In the end
I understood that code is as complex as I thought yet managable.

> 
>     * Are there performance bottlenecks?
>       Does the library design fit requirements
>       of real-time systems? How is it useable
>       for embedded systems?
>       Is the documentation clear on performance
>       tradeoffs?

I don't think it's acceptable for real-time systems.

>     * What is your evaluation of the potential
>       usefulness of the library? Can you compare
>       this FSM to other implementations?

It's definitely useful as a library for representing
UML statechart but it covers only subset of FSM.

I can compare it briefly with my experimental example
which I use to verify a power of *overloads* library.

It has very simple model:

1. Every state has a type that identifies this state
2. State machine stores current state by value => state-locale storage
3. Each transition is a function that returns new state by value,
   accepts current state in a first argument and processed event
   in a second argument. void return means no transition, only
   in-state reaction.
4. One state type can be derived from another and several transition
   functions that lead to one state from different states with common
   base can be replaced with one tranition function which lead to the
   same state from common base of those states.
5. All transitions are functions defined in state machine.
6. State machine change state always like this:

    template<class Event>
    void process_event(Event const& event)
    {
        // m_current_state is a member of state machine,
        // it behaves as variant<State1,State2,...>

        m_current_state = transition(m_current_state, event);
    }

The model is simple, state changing is atomic, more complex
functionality can be built on top either by user or by other
layer of the framework.
Extensibility is not yet solved but I'm sure the solution exists.

Sample code:
http://tinyurl.com/3swzq
or
http://cvs.sourceforge.net/viewcvs.py/cpp-experiment/overloads/libs/overloads/example/fsm.cpp?rev=1.4&view=auto

>     * Did you try to use the library?  With what
>       compiler? Did you have any problems?
>       Do you have tips for better support of older
>       compilers? What are possible portability problems?

Never had neither chance nor time.

>     * How much effort did you put into your
>       evaluation?
>       A glance? A quick reading? In-depth study?

Quick reading a couple of times and few hours of analysis.

>     * Are you knowledgeable about the problem domain?

Only common knowledge.

> 
> And finally, every reviewer should answer this question:
> 
> 
>     * Do you think the library should be accepted as a
>       Boost library?  Be sure to say this explicitly so that
>       your other comments don't obscure your overall opinion.

I'm afraid not. Either it should be repositioned as Boost.Statecharts
or redesigned for better real-time support.

-- 
Alexander Nasonov