Re: [boost] [signals2][review] The review of the signals2library(formerly thread_safe_signals) begins today, Nov 1st

Subject: Re: [boost] [signals2][review] The review of the signals2library(formerly thread_safe_signals) begins today, Nov 1st
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-11-04 18:57:39

----- Original Message ----- 
From: "Frank Mori Hess" <frank.hess_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Tuesday, November 04, 2008 11:47 PM
Subject: Re: [boost] [signals2][review] The review of the 
signals2library(formerly thread_safe_signals) begins today, Nov 1st

>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Tuesday 04 November 2008 10:14 am, vicente.botet wrote:
>> - The number of parameters of the signal class starts to be too high.
>
> I agree.  My inclination is to remove the SlotFunction and
> ExtendedSlotFunction parameters, but I'm not going to do it based only on 
> my
> personal inclination unless something like a consensus arises that it is a
> good idea.
>
>> The use of the Boost.Parameter library could simplify this.
>
> I've never used Boost.Parameter.  It does seem useful for this kind of
> situation, I guess my question would be: is there any reason _not_ to use
> Boost.Parameter?  It doesn't seem to currently be used by other boost
> libraries except for some test programs.  Is that just because it is
> relatively new?

Maybe. Accumulators use it already and also the forthcomming Flyweigth 
library.
If you are interested I have designed a class that make eassier to do this 
task based on how the Flyweigth library use it (look for for template 
parameter expression on the ML or 
http://www.nabble.com/-flyweight--parameter--is-there-an-interest-in-this-template-parameter-expresion-td18454951.html#a18454951).

>> - I don't like too much the Mutex template parameter, I would prefer a 
>> more
>> declarative template parameter as single_threaded|multi_threaded<>.
>
> It seems like mostly a matter of taste, so I don't expect to convince you 
> of
> anything, but I'll just summarize why I prefer Mutex.  1) It is a very 
> simple
> concept. 2) Users of Boost.Thread are already familiar with it.  3) It is 
> in
> the same spirit as the SlotFunction parameter from Boost.Signals.  4) 
> There
> is precendence in at least one other Boost library, pool_alloc does 
> something
> very similar:
>
> http://www.boost.org/doc/libs/1_36_0/libs/pool/doc/implementation/pool_alloc.html
>
> Maybe I'm missing your point though, as I see now below your 
> multi_threaded<>
> parameter itself takes a Mutex parameter in turn.
>
>>   The fact that the library adds a new mutex class for performance issues
>> must documented.
>
> Are you talking about signals2::dummy_mutex or signals2::mutex?  And can 
> you
> be more specific about the holes in their documentation?

>From the documentation
"The mutex class implements the Lockable concept of Boost.Thread, and is the 
default Mutex template parameter type for signals. If boost has detected 
thread support in your compiler, the mutex class will map to a 
CRITICAL_SECTION on Windows or a pthread_mutex on POSIX. If thread support 
is not detected, mutex will behave similarly to a dummy_mutex. The header 
file boost/config.hpp defines the macro BOOST_HAS_THREADS when boost detects 
threading support. The user may globally disable thread support in boost by 
defining BOOST_DISABLE_THREADS before any boost header files are included.
If you are already using the Boost.Thread library, you may prefer to use its 
boost::mutex class instead as the mutex type for your signals.

You may wish to use a thread-unsafe signal in a multi-threaded program, if 
the signal is only used by a single thread. In that case, you may prefer to 
use the dummy_mutex class as the Mutex template type for your signal. "

There is nothing here that justify a new mutex so the user can ask why 
another mutex. As you has already said on other post this was due to 
performance issues. I was only requesting you to add only this in the 
documentation.

>> - Well the implementation is based on the monitor pattern. The signal 
>> class
>> protects itself from multiple threads access locking each operation with 
>> an
>> internal mutex. This means that we need to lock/unlock a mutex each time 
>> a
>> signal must be invoked, connected, disconnected, ....
>> This coudl be expensive for some applications. I would like to be able to
>> use an externally locked mutex allowing to lock only once for several
>> signals. This can be obtained by adding a LockingStrategy template
>> parameter to the multi_threaded (internally_locked | externally_locked).
>>
>> template <typename Mutex=mutex, typename LockingStrategy
>> =internally_locked> struct multi_threaded {
>>     // ...
>> };
>>
>> When externally_locked is choosen the user needs to get a signal handle
>> providing it has locked the mutex (maybe using a strict lock as 
>> parameter)
>>
>> typedef signal<void (), ..., multi_threaded<mutex,externally_locked> >
>> my_signal1;
>> typedef signal<void (), ..., multi_threaded<mutex,externally_locked> >
>> my_signal2;
>>
>> mutex mtx;
>> my_signal1 sig1(mtx);
>> my_signa12 sig2(mtx);
>>
>> {
>>     scoped_guard lock(mtx); // locked only once
>>     my_signal1::handle hsig1 = sig1.get_handle(lock)
>>     my_signal2::handle hsig2 = sig2.get_handle(lock)
>>     // use hsig1/2 as you will use sig1/2
>>     // ...
>> } // // unlocked only once
>
> I've thought a little about some kind of external locking to handle the
> particular situation of an atomic get/modify/set combiner operation.  I 
> don't
> personally find what you're describing above compelling enough for me to
> implement though, plus there is the added interface complexity.  Couldn't 
> you
> implement the scheme you describe above on top of signals2, using a signal
> with a dummy_mutex?  You should also consider that holding the mutex while
> invoking slots opens the possibility of deadlock, since that establishes a
> locking order between the signal's mutex and whatever the slot may 
> acquire.

Are you saying that the callbacks are called with the mutex unlocked? If 
this is the case, the external_locking strategy needs the collaboration of 
the Signal2 library to wrap these calls with unlock/lock guard. So I don't 
think I can do it above  signals2[dummy_mutex].

>> - For compatibility purposes the signal class must have as default
>> parameter a dummy_mutex.
>> The library can define a basic_signal template class and two
>> specializations, signal (single_threaded) and ts_signal (multi_threaded).
>>
>> - For compatibility purposes, the optional_last_value should replace
>> last_value as the default combiner for signals on multi_threaded
>> environements, but not on single_threaded environements.
>>
>> - For compatibility purposes, preserve  the connection block() and
>> unblock() methods. Note that this follows the same pattern as lock/unlock
>> and scoped_guard. You can state that these are depreceated and remove 
>> them
>> two or three releases after.
>>
>> - For compatibility purposes the tractable and visit_each classes must be
>> preserved with a clear notice that this are not thread-safe. You can 
>> state
>> that these are depreceated and remove them two or three releases after.
>
> I don't believe compatibility should drive the default parameters for
> signals2.  At the extreme, a complete compatibility layer could be 
> provided
> on top of signals2 (but sequestered in seperate headers, maybe under
> boost/signals2/compat), which provides exactly the Boost.Signals 
> interface,
> and uses dummy_mutex underneath.

This seems to what I was purposing you" The library can define a 
basic_signal template class and two specializations, signal 
(single_threaded) and ts_signal (multi_threaded)."

> Doug suggested some time ago a staged introduction (which seems like a 
> good
> idea to me), where the original Boost.Signals would stay around for a few
> releases after Signals2 is introduced.  That would seem to cover the
> two "deprecate/remove later" cases you suggest above.
>
> It seems to me the big issue for backwards compatibility is what do we 
> need to
> keep long-term in Signals2, even after people have had a transition 
> period.
> It seems to me losing boost::trackable completely would cause people the 
> most
> headaches.

I was suggesting a path to provide backwards compatibility. I you have 
another this is OK for me. I would like only to know if you plan to ensure 
backwards compatibility and have more details on how.

>> * What is your evaluation of the documentation?
>> I was surprised that the documentation do not differs too much from the
>> initial Boost.Signal. IMO the new features of the library are not enough
>> documented on the tutorial.
>>
>> The fact that this library plans to replace the Boost.Signal library must
>> be documented on the introduction.
>>
>> The section Q&A: How has the API changed from the original Boost.Signals?
>> must be placed at the begining of the docummentation and should include 
>> the
>> best practice on how to migrate to the new version.
>>
>> Tutorial
>> - I expected to have some examples showing how the new library is used in 
>> a
>> multi-threaded application.
>> - could you add a tutorial showing?
>> "Also, if your slots depend on objects which may be destroyed 
>> concurrently
>>           with signal invocation, you will need to use automatic 
>> connection
>> management.
>>           That is, the objects will need to be owned by
>>           <classname>shared_ptr</classname> and passed to the slot's
>>           <methodname alt="slotN::track">track</methodname>() method 
>> before
>> the slot is connected."
>>
>> - could you add a tutorial showing?
>> "Support for postconstructors (and predestructors) on objects managed by
>> shared_ptr has been added with deconstruct_ptr, postconstructible, and
>> predestructible. This was motivated by the importance of shared_ptr for 
>> the
>> new connection tracking scheme, and the inability to obtain a shared_ptr 
>> to
>> an object in its constructor. "
>>
>> Design Overivew & Rationale
>> - could you explain why do you have removed some sections as Type 
>> erasure,
>> connection class and Choice of Slot Definitions?
>>
>> - could you add an explanation of why "Automatic connection management is
>> achieved through the use of shared_ptr/weak_ptr and slot::track(), as
>> opposed to the old boost::trackable base class. "?
>>
>> - could you add an explanation of why "
>> The signal::combiner() method, which formerly returned a reference to the
>> signal's combiner has been replaced by signal::combiner (which now 
>> returns
>> the combiner by value) and signal::set_combiner. "?
>>
>> - could you add an explanation of why "boost::trackable objects is gone. 
>> "?
>> Maybe on a depreceated section.
>>
>> - could you add an explanation of how the Boost.Signal2 library will
>> replace the Boost.Signal?
>> Which will be the namespace used when the replacement will take place?
>
> The above suggestions on improving the documentation generally seem good. 
> I'm
> willing to make another pass over the docs and try to flesh them out and
> address the above issues.

Could you be more precise on which issues are you ready to take in account 
and which not?

> To answer some of the questions above here:
>
> I removed the "Type erasure", etc sections because they described the
> Boost.Signals implementation, not Signals2.  Signals2 doesn't use 
> boost::any
> at all, for example.  I could have edited them down more carefully, but in 
> my
> defense they were pretty short sections.

Could you explain why you have abandoned the use of boost::any?

> The shared_ptr/weak_ptr connection management is used because 
> boost::trackable
> can't be made thread-safe.  It disconnects in the boost::trackable
> destructor, but since boost::trackable is a base class, its destructor is 
> not
> called until the derived class destructor has already run.  Thus, it 
> leaves
> open the possibility of a signal being invoked and using a partially
> destructed object.
>
> signal::combiner returned a reference, so modification through the 
> returned
> reference could not be protected by a mutex internal to the signal.
> Getting/setting the combiner by value at least insure the combiner will
> always be in a well-defined state when used, although they don't protect
> against multiple threads doing get/modify/set on a signals combiner
> concurrently.

Would you add this to the design rationale?

> I intend to leave everything in Signals2 where it is now (in 
> boost::signals2).
> Although if signals2 needs to use boost::visit_each, I don't think there 
> is
> any reason it needs a separate version of that.  I compiled my benchmark
> program using both Boost.Signal and Signals2 together with no conflicts.
> Well, except for the BOOST_SIGNALS_NUM_ARGS macro which I've since renamed 
> in
> svn.
>
>> Test
>> For compatibility purposes you should preserv all the Boost.Signal tests.
>> Does the regression_test contains bugs from your library or of
>> Boost.Signal? This is a good practice, but why to create a separated 
>> file?
>> If these bugs have a trac it will be interesting to trac them.
>
> I have all the Boost.Signal tests except for the random_signal_system.cpp
> test, which had problems sometimes hanging with both Boost.Signals and my
> code the last time I tried it (circa boost 1.33 I believe).  The
> regression_test is only bugs from my library.  I created it for 
> convenience
> for myself, since it just fits the current workflow better: bug reported,
> write test that exposes bug, fix bug.  The bugs aren't in trac, I actually
> never considered telling people to put bug reports in trac due to the 
> library
> only being in the sandbox.

Does the random_signal_system.cpp  pass the 1.37 regression? If yes, you 
should try again.

>>
>> No. The library should NOT be accepted until all these requirements are
>> satisfied. :-(
>>
>
> Ok, in any case thanks for taking the time to write a review and provide
> feedback.

Good luck with the review,

Vicente