I vote to reject the FSM library because I find the rationale for not providing better performance insufficient. Specifically, I am not convinced that the framework could not have been implemented to use static dispatch, constant-time dynamic dispatch or some combination of these and the current approach. Normally, before voting to reject a library I like to study the implementation thoroughly so I can make concrete proposals about how it might be improved. I haven't had time to do this with the FSM library, but I would still like my view to be known. I'll start by going through the relevant portions of rationale.html; then I'll answer the review questions. ------- * Introduction. The nine requirements are well-stated, and I accept them. * Dynamic configurability. I agree with the argument for not making all state machines dynamically-configurable. However, the documentation does not explain why it would not be possible to support both dynamically-configurable and non-dynamically-configurable state machines in the same framework, the way xpressive supports both dynamically and statically specified regular expressions. However, I don't consider dynamically-configurability essential, so I'm satisfied with the library in this respect. * Speed versus scalability tradeoffs. Here there is a big problem. I accept that there must be a speed versus scalability tradeoff; in fact, I'd be slightly surprised if there weren't. However, I would normally expect a slow-down of at most a few percentage points when using a framework; I find it stunning that the difference in performance between hand-written machines and framework-generated machines is so significant (10ns/10ns vs. 130ns/220ns), even in a "somewhat unrealistic" test case. In order to accept this performance gap I would need a very convincing explanation. Instead, the explanation seems to be of this form: "The requirements are X; I tried hard to satisfy X without compromising performance, but I couldn't; therefore, it can't be done." Even if we accept that the author really has tried all known techniques, it is important to document what was tried and why it doesn't work, since many library users are sure to be surprised by the performance difference and wonder whether it represents a flawed design. The explanation of why the performance difference shouldn't matter is also unconvincing. I'm particularly disturbed by the implication that FMSs should not be used for parsing. Why not? I have a candidate application of FMSs which is very similar to parsing. I'm currently modifying the Iostreams library to accommodate non-blocking i/o, and I have come to see that many filters can be thought of as state machines. The fact that a dispatch may involve a linear serach involving an RTTI check at each step immediately rules out use of the FSM framework to implement filters, since this search would have to be performed for each character. So if I want to benefit from the FSM abstraction, I would have to write my own framework. I don't see why this should be necessary. * Double dispatch. This seems like it should be a subsection of "Speed versus scalability tradeoffs" - Acyclic visitor: I accept this explanation. - GOF Visitor: It's natural to ask why a complex state machine can't be broken up into several parts which don't know much about each other, each of which uses a more efficient form of dispatch. Less efficient dispatch would be needed only at junctions between the separate parts. Dave Gomboc raised a similar point, and I don't think it has been adequately addressed. - Two-dimensional array of function pointers. This explanation makes no sense to me, even as supplemented by Andreas's reply to Iain Hanson this morning. It's common for a composite component to make use of several components implemented in different TUs. For instance, the composite commonent may "know about" the other components by derivation, by containment, typelists, etc. When it needs to assemble the array of pointers, it can simply ask each of them for their contribution. If there are order of initialization problems, the arrays can be allocated on first use. ------- In my experience, C++ provides extremely fine-grained control over which parts of a computation are performed at compile time and which at runtime. I have a strong suspicion that a FSM library could be written which satisfies the nine requirements and also provides ruthless efficiencies, at least in many common cases. It may turn out to be necessary to sacrifice performance for some FSMs, but I believe I hybrid approach may be possible which uses fast dispatch in the majority of cases. I'm willing to be convinced that it is impossible, but I haven't seen a satisfying explanation yet. Pavel Vozenilek wrote:
Here are some questions you might want to answer in your review:
* What is your evaluation of the documentation? How easy (or hard) it is to understand library features? What can be improved?
The documentation is quite good, except that the Rationale is insufficient.
* What is your evaluation of the design? What features are supported better by alternative FSMs? What could be added (or removed) from the library?
Since I've already discussed my dissatisfaction with performance, I'll just address the library's user interface. Overall, I find the design very good. Others have complained about use of CRTP or having to forward to declare states, but to me it looks pretty easy to use. I only have a few comments: - The documentation warns that executing code after calling simple_state::transit (and several other functions) can lead to undefined behavior. I believe the library should prevent this from occurring. One approach would be to have the function specify a result which is stored until react has finished executing, and then acted upon. Another possibility would be to give result move semantics, so result instances can't be stored in local variables and returned later. - The restriction on event deferral -- one must pass an event pointed to by an instance of intrusive_ptr -- is extremely inelegant: int main() { myMachine.process_event( *boost::intrusive_ptr<Event>( new Event() ) ); } I'm sure there is a good reason for this restriction, but there must be a cleaner way to accomplish the same thing. - The procedure for posting events from inside entry actions looks messy; this is noted in the docs, and it's not nearly as bad as the syntax for event deferral, but I'd like to see it improved. - I don't understand why it is necessary to pass a type as a length-one mpl::list simply because it is a class template instantiation. The docs say it is "due to some template-related implementation details", but this sounds fishy to me.
* The library documentation contains few not yet solved issues (name, separating the library into two parts, exception handling). What is you opinion here?
I have no 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?
Unfortunately, I only had time to glance at the implementation.
* 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?
See above.
* What is your evaluation of the potential usefulness of the library?
Useful, judging by the comments of reviewers who are actually using the library.
Can you compare this FSM to other implementations?
No.
* 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?
I ran the regression tests on seven compilers, with the following results: 1. VC7.1 -- passed all tests 2. VC8.0 -- passed all tests 3. VC7.0 -- ...failed updating 36 targets.. ...skipped 144 targets... ...updated 506 targets... 4. GCC 3.4.1 (Cygwin) -- passed all tests 4. GCC 3.2 (MinGW) -- passed all tests 5. Intel 8.0 ...failed updating 4 targets... ...skipped 4 targets... ...updated 678 targets... 6. CW 8.3 ...failed updating 28 targets... ...skipped 112 targets... ...updated 546 targets... 7. Borland 5.6.4 ...failed updating 36 targets... ...skipped 144 targets... ...updated 506 targets... I'd be happy to send the full error messages.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Probably about six or seven hours studying the documentation, reading the review comments and preparing my review.
* Are you knowledgeable about the problem domain?
I'm not very familiar with FMSs, but know quite a bit about other formal models of computation such as DFAs and Abstract State Machines.
Pavel Vozenilek FSM review manager
Jonathan