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

Subject: Re: [boost] [thread] Use of synchronized_value<> member in a constructor
From: Klaim - Joël Lamotte (mjklaim_at_[hidden])
Date: 2013-06-12 19:07:42

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

(note that it's just an example, in my real code Sequence is not a value
semantic type)

>             : m_info( std::move(info) )
>>      {
>>            m_info->id( new_id ); // mutex lock
>>
> You mean
>
>           m_info->id = new_id;
>
> ?
>

Yes, sorry, in my current code id is a (setter) function so I was a bit
confused but you got the idea correctly.

> 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, 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 assume the later, so the member don't need locking until the end of the
constructor of the owning object.

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

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

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

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

    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.

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.

Joel Lamotte