Frank Mori Hess wrote:

boost::signal, 10 connections, tracking enabled, invoking 1000000 times: 0.78 s

boost::signals2::signal, 10 connections, tracking enabled, invoking 1000000 times: 4.92 s

Over 6 times slower in this case. That seems to warrant some concern. -- Michael Marcin

Frank Mori Hess wrote:

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

On Monday 03 November 2008 13:54 pm, Johan Råde wrote:

...

What is the rationale for ditching boost::trackable?

It's not thread-safe. I'm not opposed to including an implementation of it for backwards compatibility though. It does seem like a less painful porting path for single-threaded code should be provided.

Yes, the code I'm working on contains 411 calls to connect (I just counted them), all of them managed using trackable, so porting to signals2 would be a head ache. Most, but not all, of the objects are managed using shared pointers. There are also scoped pointers, plain pointers and QPointers. --Johan

Frank Mori Hess wrote:

...

What is the rationale for ditching boost::trackable?

It's not thread-safe. I'm not opposed to including an implementation of it for backwards compatibility though. It does seem like a less painful porting path for single-threaded code should be provided.

I think that would be a good idea. It will make porting a lot smoother. However, since it is not thread safe, it should probably not be enabled by default, but only when the user defines some macro.

one idea (I haven't decided if it's a good one or not yet) would be to add a signals2::trackable which is based on enable_shared_from_this. It could use visit_each like Boost.Signals does, and then use enable_shared_from_this on any signals2::trackable objects it finds to get a shared_ptr for tracking. Or it could throw an exception if it finds a signals2::trackable object not owned by a shared_ptr.

That is a very interesting idea. --Johan Råde

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

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

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

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

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

Yes, I actually fixed this recently. It seems the change even slipped into the second signals2 zip file I put in the vault, in spite of my claim there that the only change was fixing compile errors on windows.

- Use specific macros instead of #define BOOST_SIGNALS_NUM_ARGS BOOST_PP_ITERATION() use #define BOOST_SIGNALS2_NUM_ARGS BOOST_PP_ITERATION()

Unfortunately I didn't get around to making the names consistent on the macros provided for the boost preprocessor stuff. But yes, it should be cleaned up.

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

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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFJENDl5vihyNWuA4URAsQgAJ4wVX9F+jfqsNIxF6vE0eoAeShecACglevR 4lMuDp2aYVKIuIeKjHjTa7E= =Wb/m -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 04 November 2008 18:57 pm, vicente.botet wrote:

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.

There is a sentence mentioning performance in the description of the dummy_mutex class, which is linked to from the signals2::mutex page. I suppose signals2::mutex should also have a little rationale paragraph stating why it even exists in signals2 (to avoid requiring linking to libboost_thread), and that it will probably be deprecated and turned into a typedef to boost::mutex at some time in the future.

Are you saying that the callbacks are called with the mutex unlocked?

Yes.

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

You could have the connect methods of your externally locked mutexes wrap the slots in a function that unlocks the external mutex before calling the wrapped slot. You'd have to take some care to avoid your wrapper slot from hiding the tracked objects in the original slot, but it could be done. That raises a related issue: currently the implementation will automatically disconnect a slot that throws an expired_slot exception. A signals2::slot will throw an expired_slot exception if it has expired slots when called. This has the nice effect that a slot wrapped inside something else before being connected to a signal will still automatically disconnect via throwing an expired_slot exception. On the down side, supporting this feature requires combiners to catch and ignore expired_slot exceptions that might be thrown by dereferencing the slot iterators. I removed documentation of this feature because I thought it wasn't needed due to the addition of extended_slot_type/connect_extended, which provide another convenient way for a slot to disconnect itself without throwing expired_slot. But it does handle the case of connection management for slots called indirectly by a signal invocation, so maybe I should put it back in.

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

Yes, it's very similar. I just wanted to emphasize a separation between the signals 2 interface "proper", and another optional interface that would exist just to support Boost.Signals compatibility.

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.

My plan would be to keep Boost.Signals around as long as people deem desirable (the specific schedule doesn't matter much to me). The only change to it would be in the documentation, stating it's deprecated in favor of Signals2, and it may be removed in the future. Beyond that, I'd add a compatibility trackable class in the signals2 namespace which could be used by people with preexisting single-threaded code using boost::trackable, who don't want to convert all their trackable classes to be owned by shared_ptr and mess with postconstructors. The use of the trackable compatibility class would be discouraged in the documentation for new code, but it would be kept in the Signals2 library permanently.

...

...

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.

...

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?

All of them, except possibly the one about putting the porting section at the beginning of the documentation (depending on what you mean by that). I'll put the porting section as a top-level section, so it is visible in the table of contents on the front page, but I don't regard it as more important than the tutorial or reference sections.

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

I didn't abandon it, it simply never existed in the Signals2 implementation. Signals2 didn't start from the Boost.Signals implementation. It started as a minimal, incomplete, thread-safe implementation of the Boost.Signals interface for my own use until the real Boost.Signals library was made thread-safe. See this post and its replies for more info: http://lists.boost.org/boost-users/2007/01/24995.php Actually, there is a lot of discussion in february of 2007 on the boost-users list where we are discussing the development of this library, and Timmo Stange's version (which actually did start from the Boost.Signals implementation).

...

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?

Yes

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

It still seems to have some problem when I try using a fairly recent boost svn trunk revision (49360). When I use the command line options "2 0.5 10", the test hangs maybe 10% of the time. That's entirely using Boost.Signals, not my code. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFJEwmf5vihyNWuA4URAhUdAJ9+2wouVW035m3EBDowIWbzX5Z4MgCfXvPF L6yC+EEM97BmFMtKJb4kbvo= =IntN -----END PGP SIGNATURE-----

On Wed, Nov 19, 2008 at 6:20 AM, Ravi <lists_ravi@lavabit.com> wrote:

[snip signals2 review]

Ravi, thanks for your review. For anyone else that is interested in this library - I am planning on writing up and posting the review results no earlier than 8pm MST (UTC-7) tonight (and hopefully not much later either :-)). If anyone else submits a review before that time, I will take it into consideration. Kind regards, Stjepan