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

Subject: Re: [boost] [thread_safe_signals][signals2] call forreviewers(review tentatively scheduled Nov 1st - Nov 10th)
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-10-15 13:58:52

----- Original Message ----- 
From: "Frank Mori Hess" <fmhess_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, October 15, 2008 3:52 AM
Subject: Re: [boost] [thread_safe_signals][signals2] call 
forreviewers(review tentatively scheduled Nov 1st - Nov 10th)
On Tuesday 14 October 2008 13:53, vicente.botet wrote:
> ----- Original Message -----
> From: "Frank Mori Hess" <frank.hess_at_[hidden]>

<snip>

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

Great

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

Yes, this will be better.

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

Do you mean that  signals2::trackable could be included without disturbance?

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

You can also post them to this ML.

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

It will be better if you use conditional compilation?

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

You have misunderstood my concern. I'm talking about a benchmasrk on a 
single threaded application without any mutex. Nothing to be with the Thread 
library.

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

Are you sure that  boost::mutex  is not header only?
Maybe you are right, but when an application needs thread safe signals 
usually it will work with mutex, directly or indirectly.
Can a user give a boost::mutex as parameter of the signal class?
I don't think that we need to reinvent the weel because the header files are 
not lightweight. Why not contribute to the Thread library making a proposal 
for a lightweight_mutex?

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

I know but Signals2 in not a new library. If Signals2 would replace Signals 
we need to know if it behaves as weel as Signals on the context Signals was 
designed for. i.e. single threaded applications. Yes optimization should be 
a secondary concern when you don't have nothing to compare. At least this is 
my opinion.

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

Yes, this information can be useful for the review.

> 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 :)

I'm not confuse ;-). No. I don't think it's a good idea to remove the Mutex 
template parameter. It would be even better to have a more declarative 
parameter as thread_model (single_threaded|multi_threaded) and then the 
implementation will do whatever is needed.

Best,
Vicente