Thanks for your patience. On Wed, Jun 12, 2013 at 10:47 PM, Vicente J. Botet Escriba < vicente.botet@wanadoo.fr> 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-constructo... (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