Stjepan Rajko | 1 Nov 20:20

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

Hello all,

The review for the Signals2 library (formerly known as
thread_safe_signals) submitted by Frank Mori Hess begins today (Nov
1st) and is scheduled to end on Nov 10th.  I would like to thank Franz
Alt, Terry Golubiewski, Doug Gregor, Ravikiran Rajagopal and Andrew
Webber for making this review possible by committing to reviewing the
library.

How to submit a review:
--------

As usual, EVERYONE is welcome to participate in the review discussions
and to submit a review.   I strongly encourage participation from
reviewers that would examine the library from a purely user standpoint
(commenting on the interface and / or the documentation), as well as
reviewers that would be willing to look into the details of the
implementation (i.e., you don't have to focus on both).

Here are some questions you might want to answer in your review (feel
free to skip those that don't apply to your analysis):

    * What is your evaluation of the design?
    * What is your evaluation of the implementation?
    * What is your evaluation of the documentation?
    * What is your evaluation of the potential usefulness of the library?
    * Did you try to use the library? With what compiler? Did you have
any problems?
    * How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?
(Continue reading)

Frank Mori Hess | 1 Nov 21:27
Favicon

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

On Saturday 01 November 2008 15:22, Stjepan Rajko wrote:
> Hello all,
>
> The review for the Signals2 library (formerly known as
> thread_safe_signals) submitted by Frank Mori Hess begins today (Nov

As requested earlier by Vincent, I threw together a little benchmark 
program (attached) comparing the speed of boost.signals with signals2.  It 
produces the following output on a P4 running Linux:

boost::signals2::signal, 1 connections, invoking 1000000 times: 0.20 s

boost::signal, 1 connections, invoking 1000000 times: 0.26 s

boost::signals2::signal, 10 connections, invoking 1000000 times: 0.77 s

boost::signal, 10 connections, invoking 1000000 times: 0.91 s

boost::signal, 1 connections, tracking enabled, invoking 1000000 times: 
0.28 s

boost::signals2::signal, 1 connections, tracking enabled, invoking 1000000 
times: 0.72 s

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

(Continue reading)

Frank Mori Hess | 2 Nov 01:46
Favicon

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

On Saturday 01 November 2008 16:27, Frank Mori Hess wrote:
>
> As requested earlier by Vincent, I threw together a little benchmark
> program (attached) comparing the speed of boost.signals with signals2. 
> It produces the following output on a P4 running Linux:

>
> boost::signals2::signal, 10 connections, invoking 1000000 times: 0.77 s
>
> boost::signal, 10 connections, invoking 1000000 times: 0.91 s

Oops, there was an error in the benchmark program, the above two times for 
10 untracked connections were reversed.  The 0.77s time belongs to 
boost::signal, and the 0.91s time is boost::signals2::signal.
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Michael Marcin | 3 Nov 19:31

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

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

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

Frank Mori Hess | 4 Nov 01:31
Favicon

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

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

Probably an similar sized chunk of overhead is due to a heap allocation 
inside the std::vector used to hold the tracked shared_ptr during 
invocation.  That part is actually something that could be optimized, at 
least for slots tracking less than some arbitrary fixed number of 
shared_ptr (say 10).
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Johan Råde | 3 Nov 19:54
Favicon

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

What is the rationale for ditching boost::trackable?

If I were to switch from signals to signals2 in the projejct I'm currently working on,
some trackable declaration would have to be replaced by several track calls.
That would add a lot of clutter and I feel the new mechanism is more error prone.

--Johan Råde

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

Frank Mori Hess | 3 Nov 21:01
Favicon

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


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.

> If I were to switch from signals to signals2 in the projejct I'm currently
> working on, some trackable declaration would have to be replaced by several
> track calls. That would add a lot of clutter and I feel the new mechanism
> is more error prone.

Wrt clutter and error-proneness, 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.
Johan Råde | 3 Nov 21:31
Favicon

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

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

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

Johan Råde | 4 Nov 20:41
Favicon

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

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

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

Johan Råde | 4 Nov 06:50
Favicon

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

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.

I think it should be available, to ease porting from the old signals library.
It should probably not be available 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.

I like this idea.

--Johan Råde

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

(Continue reading)

vicente.botet | 4 Nov 16:14

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.
(Continue reading)

Frank Mori Hess | 4 Nov 23:47
Favicon

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


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

Maybe I'm missing your point though, as I see now below your multi_threaded<> 
(Continue reading)

vicente.botet | 5 Nov 00:57

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

----- Original Message ----- 
From: "Frank Mori Hess" <frank.hess <at> nist.gov>
To: <boost <at> lists.boost.org>
Sent: Tuesday, November 04, 2008 11:47 PM
Subject: Re: [boost] [signals2][review] The review of the 
signals2library(formerly thread_safe_signals) begins today, Nov 1st

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

Maybe. Accumulators use it already and also the forthcomming Flyweigth 
library.
(Continue reading)

Frank Mori Hess | 6 Nov 16:13
Favicon

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


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 
(Continue reading)

Fabio Fracassi | 10 Nov 11:30

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

Hi,

I consider this submission a evolutionary development of the signals
library, so I'd like to focus the review questions on the parts that
changed.

Stjepan Rajko schrieb:
[...]
> 
> Here are some questions you might want to answer in your review (feel
> free to skip those that don't apply to your analysis):
> 
>     * What is your evaluation of the design?

* Automatic connection management:
This is great, even if it wasn't required for thread safety reasons, I
prefer using shared_ptr based connection tracking over boost::trackable,
since it enables me to track (and thus safely use) 3d party classes.

So on that note I would suggest to strike the "Unfortunately" from the
Auto Connection Management Design Rationale page, and emphasize the
benefit of the new design.

* Namespace signals2:
Under normal circumstances I would argue for keeping the name "signals"
for the namespace, because frankly its the best name for the concept.
But considering the real-world constraint of staying compatible with
code using Qt, any other name is much preferable. (I consider this a
deal-breaker, since using "signals" forces you to either patch boost, or
to educate your users of the Issue and have them deal with the problem)
(Continue reading)

Frank Mori Hess | 10 Nov 16:25
Favicon

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


On Monday 10 November 2008 05:30 am, Fabio Fracassi wrote:
> * Automatic connection management:
> This is great, even if it wasn't required for thread safety reasons, I
> prefer using shared_ptr based connection tracking over boost::trackable,
> since it enables me to track (and thus safely use) 3d party classes.

One issue that has occurred to me with the shared_ptr use, is it requires the 
use of boost::shared_ptr.  So, if someone's program is instead always using 
std::shared_ptr for example, that could be a bit of an annoyance.  One 
solution would be to make it support anything that provides the 
shared_ptr/weak_ptr interface by making slotN::track() a template and doing 
some type erasure inside the library.  Unfortunately, neither the shared_ptr 
or weak_ptr interface provides a typedef for it's associated 
weak_ptr/shared_ptr type, and I would need to get both the weak_ptr and 
shared_ptr type from the single argument passed to slotN::track().  So, I'd 
have to add some template traits classes that could determine if a class is a 
shared_ptr or weak_ptr, and the type of its associated weak_ptr or 
shared_ptr.  I could provide specializations of the traits classes for the 
shared_ptr/weak_ptr implementations I know about (boost, std, tr1, etc), 
otherwise the user would have to provide a specialization.

The other option would be to add yet another template parameter to the signal 
class for the shared_ptr type, but it has too many parameters already.

> I haven't done a comparison, but the Docu seems to be mostly the same as
> the original signals. Thus for the usecases that are documentated there
> its high quality, but I miss the same for the new features:
>
> * I miss tutorial quality examples for some of the new features:
(Continue reading)

vicente.botet | 12 Nov 11:41

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

----- Original Message ----- 
From: "Fabio Fracassi" <f.fracassi <at> gmx.net>
To: <boost-users <at> lists.boost.org>
Cc: <boost <at> lists.boost.org>
Sent: Monday, November 10, 2008 11:30 AM
Subject: Re: [Boost-users] [signals2][review] The review of the signals2 
library (formerly thread_safe_signals) begins today, Nov 1st

> I have a tendency to yes, because even now it is better than what we have 
> now (which is not saying that signals1 was bad!), on the other hand I 
> think that the missing documentation is crucial.

I think that a mini review would ensure that the documentation is updated as 
we expect.

Vicente 
Johan Råde | 19 Nov 19:45
Favicon

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

Here is a late review.

>     * What is your evaluation of the design?

Excellent. The design of Boost.signals is excellent,
and Boost.signals2 follows the same design.

Boost.signals2 should provide an optional (non-thread safe)
compatibility mode with Boost.signals.
This mode should not be available by default,
but only if the user defines some macro.
This will make porting from Boost.signals
to Boost.signals2 much smoother.

Frank also mentioned the possibility of adding
a thread safe version of boost::trackable
based on boost::enable_shared_from_this.
That would be a very valuable addition to the library.

>     * What is your evaluation of the implementation?

Have not looked at the implementation.

>     * What is your evaluation of the documentation?

I agree with the other reviewers that it could be improved.

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

Very useful. The lack of thread safety is the main weakness of Boost.signals,
(Continue reading)

Hansi | 19 Nov 22:59

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

Hi,

I don't have a review...
I have made a few tests with the library. I think the library is good, 
but I don't know if I make something wrong, but it seems for me that the 
library is slow. I have made a few tests with VC++ 8.0 and _SCL_SECURE=0 
and it seems that it is about 50 times slower than a normal 
boost::function with a bind.... It's a lot...

Best regards
Hansjörg

The test code was:

class PerformanceCounter
{
private:
	LARGE_INTEGER CurrentTickCount_;
	LARGE_INTEGER OldTickCount_;
	LARGE_INTEGER TickFrquency_;
	double EllapsedTime_;
	double TotalTime_;
public:
	PerformanceCounter():
	EllapsedTime_(0.0),
	TotalTime_(0.0)
	{
		::QueryPerformanceFrequency(&TickFrquency_);
		::QueryPerformanceCounter(&CurrentTickCount_);
		OldTickCount_ = CurrentTickCount_;
(Continue reading)

Frank Mori Hess | 20 Nov 01:11
Favicon

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

On Wednesday 19 November 2008 16:59, Hansi wrote:
> I have made a few tests with the library. I think the library is good,
> but I don't know if I make something wrong, but it seems for me that the
> library is slow. I have made a few tests with VC++ 8.0 and _SCL_SECURE=0
> and it seems that it is about 50 times slower than a normal
> boost::function with a bind.... It's a lot...

I distilled your benchmark down to something minimal I could compile under 
Linux (attached) and I get:

$ ./a.out
0.25 s

10000000
2.24 s

when compiling with gcc with -O3.  So, I'm getting a factor of 9.  The 
original Boost.Signals doesn't do any better in this particular benchmark.

One odd thing I noticed is the signals2 benchmark actually runs slower (2.8 
sec) if I define NDEBUG!
Attachment (hansi_bench.cpp): text/x-c++src, 879 bytes
_______________________________________________
Boost-users mailing list
Boost-users <at> lists.boost.org
http://lists.boost.org/mailman/listinfo.cgi/boost-users
Ravi | 19 Nov 14:20
Favicon

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

In general, I view signals2 as an update to the signals library. As a long 
time (happy) user of signals, the only issues I am concerned with are the 
modifications/changes compared to signals. With the exception of 'trackable', 
the library interface has changed very little in signals2; all of my signals 
code still in use has been ported to signals2 with very little difficulty. 

On Saturday 01 November 2008 03:22:49 pm Stjepan Rajko wrote:
>     * What is your evaluation of the design?

The design is very similar to that of the existing signals library. As a long 
time stable library, I am happy with the design. My major concern is that 
shared_ptr is required for tracking: making shared_ptr work with other custom 
smart pointers (required at many shops) is an extra step that can be tedious. 
However, I do not see any other way to avoid the pre-destruction problem 
mentioned in the documentation without making the interface considerably more 
complex.

>     * What is your evaluation of the implementation?

I did not look at the implementation. I am unhappy about the switch to a 
header-only implementation. Some of my platforms have very little memory into 
which I have to shoehorn multiple applications; boost.signals is one of the 3 
or 4 libraries that is used in multiple applications and the prospect of 
increasing the overall memory footprint worries me a lot.

This problem is not confined to signals2, of course. As a side rant, this 
tendency to make everything header-only makes life harder for those of us who 
work with embedded systems; it is hard enough to overcome historical 
prejudices against C++ in embedded systems without adding perceived "code 
bloat" to the mix. My only hope is that the CMake-based build sytem becoming 
(Continue reading)

Stjepan Rajko | 19 Nov 17:47

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

On Wed, Nov 19, 2008 at 6:20 AM, Ravi <lists_ravi <at> 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
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost


Gmane