Re: [boost] [FSM] Review library version update

From: Andrey Semashev (andysem_at_[hidden])
Date: 2007-02-03 16:27:41

Hello Steven,

Saturday, February 3, 2007, 10:05:52 PM, you wrote:

> Andrey Semashev <andysem <at> mail.ru> writes:

>> 
>> Hello Steven,
>> 
>> Wednesday, January 31, 2007, 10:02:24 PM, you wrote:
>> 
>> > AMDG
>> 
>> > Andrey Semashev wrote:
>> >> - A new example "BitMachine" added. It is quite similar to same-named
>> >> example of Boost.Statechart and may be used as a performance test.
>> >> - The Performance section of the docs updated with some test results.
>> 
>> > First of all I think that the elapsed time is more useful than the
>> > number of iterations per second.  I know that one is just the
>> > inverse of the other but the elapsed time has a linear correlation
>> > to the amount of work being done.
>> 
>> The elapsed time doesn't give you an absolute view of performance
>> since it also depends on number of iterations. But I think I may
>> present both figures in the docs.

> Sorry. I wasn't clear.  I meant time per iteration.

I see. Will change it.

> I looked through state_machine.hpp again

> You changed "very base class" to "most base class."
> What you mean is ultimate base class I think.

Will change the comment.

> basic_state_machine::m_StatesInfo needs to be volatile.
> At the very least least cast it to volatile for
> initialization.  This has nothing to do with sequence
> points; It has everything to do with observable behavior.

But it is volatile as it's being passed to the init_states_info
function which does the initialization. It's argument type is
volatile.
I'd really hate to make it volatile for the rest of the machine's
life, it only needs to be initialized properly.

> line 912
>     template< typename EventT >
>     static state_dispatcher< EventT > const& get_state_dispatcher()
>     {
>       static const state_dispatcher< EventT > dispatcher;
>       return dispatcher;
>     }
> function static is not thread safe.
> in pseudo-code this is
> {
>     static no_initialize const state_dispatcher< EventT > dispatcher;
>     static preinitialize bool dispatcher_initialized = false;
>     if(!dispatcher_initialized) {
>         dispatcher_initialized = true;
>         dispatcher.dispatcher();
>     }
>     return(dispatcher);
> }

It doesn't need to be. The state_machine class is intended to be used
in single-thread context. Once you have to access the machine from
multiple threads you should use locking_state_machine which
synchronizes on "process" function calls which is the only way
"get_state_dispatcher" gets called.

> locking_state_machine.hpp
> dynamic_locker is unnecessary
> you can use
>     scoped_lock this_lock(false);
> and
>     this_lock.lock();
> to do the same thing.

Actually, I can't - the lightweight mutex doesn't have this. The only
thing the Boost.FSM requires from the locker class is the
construction/destruction locking.

> Also for the comparisons of mutex addresses you should
> use std::less<void*> instead of operator< because operator<
> does not guarantee a strict weak ordering for pointers.
> see 5.9/2 and 20.3.3/8

Thanks, will fix.

-- 
Best regards,
 Andrey                            mailto:andysem_at_[hidden]