Re: [boost] [thread] Use of synchronized_value<> member in a constructor

Subject: Re: [boost] [thread] Use of synchronized_value<> member in a constructor
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2013-06-13 02:13:46

Le 13/06/13 01:07, Klaim - Joël Lamotte a écrit :
> Thanks for your patience.
>
>
> On Wed, Jun 12, 2013 at 10:47 PM, Vicente J. Botet Escriba <
> vicente.botet_at_[hidden]> wrote:
>
>>>       explicit Sequence( SequenceId new_id, SequenceInfo info )
>>>
>> explicit is not needed here.
>
> In C++11 it prevent returning a temporary with more than one attributes in
> the constructor without using
> the type name explicitly:
>
>      // returning a Sequence
>      return { sequence_id, some_info }; // error (in C++11): the constructor
> is explicit
>      return Sequence{ sequence_id, some_info }; // OK (C++11)
>
> I don't have the standard around to check but here is an explanation:
> http://stackoverflow.com/questions/12437241/c-always-use-explicit-constructor/12437453#12437453
I was not aware of this C++11 feature. Thanks.
>
> (note that it's just an example, in my real code Sequence is not a value
> semantic type)
>
>
>> I would define thesame kind of constructor for the class SequenceInfo .
>>
>>
>>      Sequence( SequenceId new_id, SequenceInfo info )
>>            : m_info( new_id, std::move(info) )
>>      {}
>>
> This example is a contrived/simplistic one. In my real code the member I'm
> setting is from generated code
> which I can't modify without important efforts.
> Assume that I'm not in the control of that member's type definition.
> The best I could do in this case, I suppose, would be a function wrapper:
>
>       Sequence( SequenceId new_id, SequenceInfo info )
>            : m_info( merge_info( new_id, info ) )         // no locking
>      {}
>
> Which should be equivalent to your suggestion I guess,
This seems a good alternative.
>   but it is not enough
> in all cases. See below.
>
>
>>> If I am correct, then the lock of the synchronized_value is unnecessary.
>>>
>> You don't need to lock at construction. but once constructed the lock is
>> imperative.
>>
>>
> Just to clarify, and make sure I understand 100% correctly,
> are you talking about the member's construction (m_info) or the owning
> object construction (Sequence)?
I meant the member.
> I assume the later, so the member don't need locking until the end of the
> constructor of the owning object.
No. Any access to the member would imply a lock after construction.

m_info-> returns a lock.

>
>
>>   I suspect that there might be other cases like this one where
>>> the object responsable for the synchronized object should be able, rarely,
>>> to access the object unsafely.
You are supposing that there is an object responsible for synchronized 
object, but I don't know what this means and how to ensure it.
If you want this pattern , you maybe need external locking. See the 
class externally_locked.
>>>
>> For example?
>>
>>
> This is just speculation. Maybe I'm just pessimistic.
> Currently I only have the case of the constructor and I don't think I will
> have others.
> I have another bigger code base which have more complex concurrent cases
> where
> I might find similar usage (access to a usually locked object in a case
> where I am 100% sure there can't be concurrent accesses).
Could you develop the case and explain how are you 100% sure that there 
is no concurrent access?
> I will search and report if I find something, I'm not sure I will.
>
>
>>> Suggestion: add an unsafe access to the object, in a very visible way.
>>>
>>>       m_info.unsafe_access().id( new_id );
>>>
>>> OR
>>>
>>>       m_info.unsafe_access()->id( new_id ); // using a smart pointer which
>>> checks if safe accesses have been done inbetween and throw if it is the
>>> case? or any other checks detecting potential error
BTW, I don't know what "checks if safe accesses have been done 
inbetween" could mean? Could you clarify?
>> I don't think this is needed for the use case presented. Just create the
>> appropiated constructors.
>
> As I was saying, in the real code I will have to use a builder function
> instead, I can't modify that type.
This seems a good alternative.
>
> However it is still not a perfect solution.
> In the real code I have some logic I need to do before manipulating the
> synchronized member.
> It should be done in this order, first some logic, then use thesynched
> object.
> But even if I use a function to create the member but still have to access
> it
> later because of that logic, there is no point in doing that.
You can create a unsynchronized SequenceInfo object, do whatever you 
want with and last create the synchronized SequenceInfo just by copying.
> To clarify what I'm trying to explain (this is fictive code close to mine,
> which is more complex):
>
> explicit Sequence( SequenceId new_id, SequenceInfo info )
>            : m_info(  merge_info( new_id, info ) )      // assuming I follow
> your suggestion
>      {
>            if( first_thing_to_do() )
>            {
>               second_thing_to_do( m_info->name ); // still need to lock
>            }
>      }
I recognize that the factory function could be less natural, but it works

SequenceInfo makeIt( SequenceId new_id, SequenceInfo info )
{
     
     info = merge_info( new_id, info );
     if( first_thing_to_do() )
           {
              second_thing_to_do( info->name ); // still need to lock
           }
     }
     return info

}

explicit Sequence( SequenceId new_id, SequenceInfo info )
           : m_info(  makeIt( new_id, info ) )
{}

> I'm using function name instead of real code just so you get the idea,
> sorry if it's not real code.
> I try to do minimum work in the constructor but I noticed the locking
> access when I couldn't avoid it.
>
> I will try to be clearer with some code, first this:
>
> class InfoProcessor;    // generate analytic data from info - non-copyable!
Is it movable?
>
> class Sequence
> {
>      boost::synchronized_value<InfoProcessor> m_infoproc;
>
> public:
>      explicit Sequence( SequenceInfo info )
>            : m_infoproc(  info )      // process info into the constructor,
> ready to be used
Isn't the parameter  InfoProcessor info?
>      {
>            m_infoproc->do_something(); // can't avoid a lock?
>      }
>
>      Sequence( const Sequence& ) = delete;
>      Sequence& operator=( const Sequence& ) = delete;
>
>
> Assuming I can't modify InfoProcessor, the only way I see how to do the
> call in the constructor
> without locking would be to encapsulate the member into a wrapper:
>
> class Sequence
> {
>      struct InfoProcWrapper {
>           InfoProcWrapper( SequenceInfo info )
>               : infoproc( info )
>           { infoproc.do_something(); }   // no locking
>
>           InfoProcessor infoproc;
>      };
>      boost::synchronized_value<InfoProcWrapper > m_infoproc;
>
> public:
>      explicit Sequence( SequenceInfo info )
>            : m_infoproc(  info )      // now any access will need locking
>      {
>            // OK: m_infoproc->infoproc.do_something() was already called
> without locking.
>      }
>
> Which would be fine if it wasn't so noisy and that's only for 1
> synchronized member...
I agree, creating these wrappers is too noisy but an alternative. I 
think the factory is a better choice. Note that you have lambdas in C++11.

>
> class Sequence
> {
>      struct InfoProcWrapper {
>           InfoProcWrapper( SequenceInfo info )
>               : infoproc( info )
>           { infoproc.do_something(); }   // no locking
>
>           InfoProcessor infoproc;
>      };
>      struct DataCruncherWrapper {
>           DataCruncherWrapper( SequenceInfo info )
>               : cruncher( info )
>           { cruncher.prepare( 42 ); }   // no locking
>
>           DataCruncher cruncher;
>      };
>      boost::synchronized_value<InfoProcWrapper> m_infoproc;
>      boost::synchronized_value<DataCruncher> m_cruncher; // should NOT be
> synched with the same mutex
>
> public:
>      explicit Sequence( SequenceInfo info )
>            : m_infoproc(  info )      // OK: setup with no locking
>            : m_cruncher(  info )      // OK: setup with no locking
>      {
>             m_cruncher->cruncher.configure( m_infoproc->infoproc ); // how
> to do that without locking?
>      }
Delaying the construction. This is similar to the problems raised for 
writing constexpr fuctions.

class Sequence
{
   struct X {
     InfoProcWrapper m_infoproc;
     DataCruncher m_cruncher;
     X(SequenceInfo info)
           : sx_(  info )
           : m_cruncher(  info )
     {
            m_cruncher->cruncher.configure( m_infoproc->infoproc );
     }
  };

   struct SX {
     boost::synchronized_value<InfoProcWrapper> m_infoproc;
     boost::synchronized_value<DataCruncher> m_cruncher; // should NOT be
synched with the same mutex
     SX(X) {...};
   };
   
   SX sx_;
  public:
     explicit Sequence( SequenceInfo info )
           : sx_(  X(info) )
     {
     }

>      void crunch( Data data ) { m_cruncher->cruncher.crunch( data ); } //
> OK: locks only one mutex
>      void add( Element element ) { m_infoproc->infoproc.add( element ); } //
> OK: locks only one mutex
>
> Anyway, I might be wrong but it seems to me that any attempt to combine use
> of several synched members in the constructor
> will imply locking OR adding several layers of abstractions, which if the
> members' types have value-semantic, can be easy,
> but if it's not the case, it becomes quickly very noisy.
I agree. We surely need something that make this easier and less noisy. 
I would prefer if we continue to see other alternatives to go directly 
to providing a bypass function.
>
> Obviously it is still just speculation but it don't seem unfamiliar to have
> this kind of work in a constructor.
>
>   It looks like it would allow avoiding cost where it could be avoided using
>>> a mutex manipulated manually.
>>>
>>> Any thoughts on this?
>>>
>> I'm open to your suggestion if you have other use cases that can not be
>> solved otherwise.
>>
> I will have to find a concrete and more convincing use case and report here
> then.
>
>
> Note that hopefully the cost of the lock isn't important so far in my
> specific use case of today,
> I was just pointing the constructor cost because in the other code bases I
> have
> I might have more performance-related concerns and I might not be the only
> one.
>
>
I would help you if I can.

HTH,
Vicente