Re: [boost] [thread_safe_signals][signals2] call for reviewers(review tentatively scheduled Nov 1st - Nov 10th)

Subject: Re: [boost] [thread_safe_signals][signals2] call for reviewers(review tentatively scheduled Nov 1st - Nov 10th)
From: Frank Mori Hess (fmhess_at_[hidden])
Date: 2008-10-14 21:52:58

On Tuesday 14 October 2008 13:53, vicente.botet wrote:
> ----- Original Message -----
> From: "Frank Mori Hess" <frank.hess_at_[hidden]>

> > There is an entry in the FAQ section which has a summarized list of
> > changes.
>
> This is a good starting point, but in my opinion it should appear in the
> introduction or on a history section. It would be interesting to sum up
> in a table which classes/functions are compatible, which are not
> supported/depreceated and which are new, which new features can be
> adopted to Boost.Signal? I expect also some examples that show how the
> non supported/depreceated Boost.Signal are replaced by the equivalent
> Boost.Signal2. You don't think this must be done before the review?

Yes, moving the API changes list up to its own section so it is visible in 
the main TOC seems like a good idea.  A table with removed/deprecated 
features and their replacements does seem like it would be more readable. 

As far as examples showing how to port code goes, there's really not much 
to add at the moment.  Most of the changes are either trivial, so I'd 
giving examples of things like replacing declarations of 
boost::signals::connection with boost::signals2::connection.  Or, they are 
sufficiently different (like the automatic connection management) that 
bringing up the old way while trying to explain the new way doesn't help.  
That is, the appropriate place for both new users of Signals2 and users 
familiar with Boost.Signals to learn about the Signals2 automatic 
connection management is in the tutorial + reference.  I think adding some 
links in the API changes list to the appropriate tutorial section would be 
helpful though.

Now, if it is decided in review that something like a 
boost::signals2::trackable needs to be added to ease the pain of porting, 
that is something that would deserve more exposition in the API changes 
section of the docs.  And discussion/example usage of a 
signals2::trackable class wouldn't belong in the tutorial, since I'd want 
to discourage new users from using it.

> > There is no
> > example code for the signal::extended_slot_type and
> > signal::connect_extended() stuff I squeezed in at the last minute, but
> > they
> > are in the reference section.
>
> Do you plan to add some tutorial/examples of these new features before
> the review?

It definitely should be added to the tutorial.  I'm a little leery of 
putting a bunch of slightly updated zip files up after the review has been 
announced, and people being confused about which version is under review, 
whether they need to re-review the updated version, etc.  But assuming the 
library and signal::connect_extended() are accepted, I'll add a section to 
the tutorial about it before it gets into a boost release.  

There's really not much to it, connect_extended() is just like connect() 
except it takes a signal::extended_slot_type instead of a 
signal::slot_type.  And a signal::extended_slot_type is just like a 
signal::slot_type except it takes an extra first argument which is the 
slots connection to the signal invoking it.  I described my motivation for 
adding it a couple weeks ago in this post:

http://lists.boost.org/Archives/boost/2008/10/142995.php

That post shows the connection argument added as the last argument though, 
I ended up putting it as the first argument.

>
> Where can we find this benchmarking, on the documentation?

The program should be in the zip file at

libs/signals2/test/invocation_benchmark.cpp

You have to make 2 trivial changes to compile it against Boost.Signals: 
change the #include directive, and fix the declaration of the signal_type 
typedef so it doesn't look in the signals2 namespace.  I didn't put any 
benchmark results into the documentation

> I think that a benchmark/comparation of the Boost.Signal2 with a
> signals2::dummy_mutex parameter and the Boost.Signal is mandatory.
> Otherwise we let each user check which one is better on a single_thread
> environement.

I don't think mutex benchmarks belong in the Signal2 docs, maybe in 
Boost.Thread?  signals2::dummy_mutex should add no overhead, and although 
I am providing signals2::mutex, it's only because boost::mutex isn't quite 
header-only yet, and detail::lightweight_mutex doesn't conform to the 
Boost.Thread Mutex concept.  Also, the mutex implementations and thus 
performance vary depending on what platform you are running on.  

It doesn't seem customary for boost libraries to provide extensive 
benchmark information in their documentation.  Also, the boost library 
guidelines indicate optimization should generally be a secondary concern.

What could be useful information (that isn't currently provided by the 
docs) would be how many mutex locks are aquired during the course of 
various functions.  For example, signal invocation results in a lock of 
the signal's mutex, plus a lock for each connection as it checks if the 
connection is disconnected or blocked.  Maybe it could go in an appendix 
describing some of the implementation details.

Actually, I personally wouldn't mind if people wanted to drop the Mutex, 
SlotFunction, and ExtendedSlotFunction template parameters from the signal 
classes entirely, and make them all hard-coded implementation details of 
the library.  No choices, no user confusion :)