Re: [boost] [signals2][review] The review of the signals2 library is scheduled to end on Monday, November 10th

Subject: Re: [boost] [signals2][review] The review of the signals2 library is scheduled to end on Monday, November 10th
From: Franz Alt (f.alt_at_[hidden])
Date: 2008-11-10 05:09:16

>Hello,
>
>The review of the signals2 library is scheduled to end on Monday,
>November 10th.  We have had some good discussion so far and a couple
>of reviews.  I hope the remaining few days will provide enough time to
>others that were planning on submitting a review.
>
>If anyone needs any more discussion time or time to write the review,
>please let me know (so we can see about extending the review period,
>or so that I know to wait for your review longer).
>
>Kind regards,
>
>Stjepan
>_______________________________________________
>Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>
>
>

Hi all,

my result of the review:

* What is your evaluation of the design?

Note: I've mainly focused on a user point of view for this review. At the end
of the review I've taken some look into the libraries source code.

Normally the first contact with a new library is the libraries documentation.
At boost.signals2 library documentation the design is not really clear. Ok,
the signal-slot mechanism should be quite clear for most of software
developers but the documentation could be more detailed concerning to basic
mechanisms. Boost.signals2 wants to be a thread safe library. In the
documentation there is nothing about the design in relation to a thread
context.

* What is your evaluation of the implementation?

Like I've said above, my review focused mainly the user point of view. At the
end I've taken a quick look into the source code.

The code is quite hard to read for my taste. It's not a well formatted source
code. Sometimes signatures are not well structured and ordered. The signal
class would be a good example here:

template<typename Signature,
  typename Combiner = optional_last_value<typename boost::function_traits<Signature>::result_type>,
  typename Group = int,
  typename GroupCompare = std::less<Group>,
  typename SlotFunction = function<Signature>,
  typename ExtendedSlotFunction = typename detail::extended_signature<function_traits<Signature>::arity, Signature>::function_type,
  typename Mutex = mutex >
class signal: public detail::signalN<function_traits<Signature>::arity,
  Signature, Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::type

If the user wants to use a different mutex (e.g. 'dummy_mutex') it must take
the default values for all the other template arguments. This could lead to
something like this:

typedef signals2::signal
  <
        void (bool),
        signals2::optional_last_value<typename boost::function_traits<void (bool)>::result_type>,
        int,
        std::less<int>,
        function<void (bool)>,
        typename signals2::detail::extended_signature<function_traits<void (bool)>::arity, void (bool)>::function_type,
        signals2::dummy_mutex
  > signal_t;

This isn't really user friendly. But it's sometimes a matter of taste.

* What is your evaluation of the documentation?

Ok, but not satisfying at all.

I liked the splitting of the tutorials into the sections beginner, intermediate
and advanced. A libraries newbie could easily navigate through the beginners
sections and than towards to the more difficulty areas. This is a good practise
to take the user through the documentation by hand.

I don't liked the level of detail of the examples. Before I began with the
review of boost.signals2 I read about the boost.signals library in 'Beyond the
C++ Standard Library' from Bjorn Karlsson. Without the knowledge of this
article in this book, the examples don't give the user the imagination what
could be done with boost.signals2. More detailed and straight forwared examples
would help the users very much.

I also don't liked the fact that only three times the word 'thread' was named.
Boost.signals2 wants to be a 'thread safe signals' library. Therefore some more
details and especially more examples concerning on that area should be given to
the user.

During the review I've read about the 'signal::extended_slot_type'. But there
was no example about that slot type. No deeper hint in the documentation only
one sentence about that. The explanation of the author was that extended slots
is a feature that comes to the library in the last minutes.

* What is your evaluation of the potential usefulness of the library?

In general, a thread-safe implementation of a signal slot mechanism could be an
enrichment of an application for todays multiprocessor processors.

* Did you try to use the library? With what compiler? Did you have any problems?

I wrote some single-threaded and some multi-threaded test programs. The library
was easy to use. No exceptions, no errors.

The used compilers are the Microsoft Visual Studio 2008 (SP1) under Windows XP
and GCC 4.0.2 under Mac OS X 10.5.5.

The only problems I've had was under Windows. The signals2-2008-10-08 produces
some compiler errors and warnings. For this reason I used signals2-2008-10-22
at Windows.

* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

I've spend some time to read about boost.signals1 at 'Beyond the C++ Standard
Library' from Bjorn Karlsson to get a feeling about the original version of the
boost.signals library. I've also spend some days to write test programs and to
study the source code of the library. However, I've tried to focus on a users
point of view.

* Are you knowledgeable about the problem domain?

I know some signal slots implementations like the one from Trolltechs Qt. I also
know threads and the usage of them.

* Do you think the library should be accepted as a Boost library?

NO, not now! But with some modifications in some weeks.

I think that a thread-safe version of a signal-slot library is a must-have for
the boost library. Regarding to the current version of boost.signals2 library
(signals2-2008-10-08.zip) I have the feeling that the library is at alpha state.
Like I've wrote before the documentation is not very detailed. To less examples
to less design and detail informations about the thread safety. Undocumented
functionalities like 'extended_slot' and some more.

At the boost.devel newsgroup some performance outputs are posted. One benchmark
was very interesing:

        "On Monday 03 November 2008 13:31, Michael Marcin wrote:
	> 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.

        Almost 2 seconds of the overhead is due to atomic reference counting for 
        shared_ptr.  If I compile the benchmark with DISABLE_BOOST_THREADS I get:

        boost::signals2::signal, 10 connections, tracking enabled, invoking 1000000 
        times: 3.20 s [...]"
        
The former version of boost.signals needs 0.78s for the test, the new version
needs 3.20s. This supports my feeling that there could be done more optimication
at boost.signals2.

To cut a long story short boost.signals2 should be part of boost. Definitive!
But I think some weeks of work could help the library very much.

Franz