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

----- Original Message ----- 
From: "Stjepan Rajko" <stjepan.rajko <at> gmail.com>
To: <boost <at> lists.boost.org>; "boost users" <boost-users <at> lists.boost.org>; 
<boost-announce <at> 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á 

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost