----- Original Message ----- From: "Stjepan Rajko" <stjepan.rajko@gmail.com> To: <boost@lists.boost.org>; "boost users" <boost-users@lists.boost.org>; <boost-announce@lists.boost.org> Sent: Saturday, November 01, 2008 8:22 PM Subject: [boost] [signals2][review] The review of the signals2 library(formerly thread_safe_signals) begins today, Nov 1st Hi, here follows my expectations from this new version: * the library must be forward compatible with the Boost.Signal (changing only the namespace). * the new version must perform as well as the Boost.Signal library on single_threaded environements. * on multi_treaded applications the library must provide a mechanism to limit the locking on each operation. * What is your evaluation of the design? I don't think the current design satisfy my expectations. Next follows some details: - The number of parameters of the signal class starts to be too high. The use of the Boost.Parameter library could simplify this. - I don't like too much the Mutex template parameter, I would prefer a more declarative template parameter as single_threaded|multi_threaded<>. The fact that the library adds a new mutex class for performance issues must documented. - 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 - 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. * What is your evaluation of the implementation? I have no taken the time to evaluate this part. Only some comments: - Be coherent with the library name and the header protecting macro #ifndef BOOST_TS_ #ifndef BOOST_TSS_ #ifndef BOOST_SIGNALS2_ #ifndef BOOST_SIGNALS_COMMON_MACROS_HEADER #ifndef BOOST_DECONSTRUCT_PTR_HEADER #ifndef BOOST_LAST_VALUE_HPP #ifndef BOOST_SHARED_CONNECTION_BLOCK_HEADER #ifndef _THREAD_SAFE_SIGNAL_HPP BOOST_SIGNALS2_ seems the best prefix. - Use specific macros instead of #define BOOST_SIGNALS_NUM_ARGS BOOST_PP_ITERATION() use #define BOOST_SIGNALS2_NUM_ARGS BOOST_PP_ITERATION() * 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? 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. Performances section must be added. A depreceated section could be added. * What is your evaluation of the potential usefulness of the library? As this library is a thread safe extension of a already accepted and useful Boost library there is no doubt of its usefulness. * Did you try to use the library? With what compiler? Did you have any problems? No. I'have used Boost.Signal on some of my applications. I have no tried the new version. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? In-depth study of the documentation. A glance on the implementation. * Are you knowledgeable about the problem domain? Yes. * Do you think the library should be accepted as a Boost library? Well if the library will be a replacement of the Boost.Signal library, the performances on a single thread environement should be as wood as the initial Boost.Signal. And the library must be forward compatible with the Boost.Signal at least on single thread environements. So all the incompatibilities must be removed on this context. Otherwise the replacement can not be done. For the multi_threaded environements the extenally_locked locking strategy seems to me a must have. No. The library should NOT be accepted until all these requirements are satisfied. :-( ______________________ Vicente Juan Botet Escribá