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

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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
Permalink | Reply |
headers
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
Permalink | Reply |
headers
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
Permalink | Reply |
headers
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
Permalink | Reply |
headers
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.
Permalink | Reply |
headers
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
Permalink | Reply |
headers
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
Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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
Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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
Permalink | Reply |
headers
Nat Goodspeed | 16 Jan 22:35
Favicon

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

Johan Råde wrote:

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

Second that! While I love the generality of the explicit tracking, I've 
been jumping through some hoops trying to adapt existing Boost.Signals 
code that uses boost::trackable.

If signals2 could provide a thread-safe version of boost::trackable, it 
would make conversion a much easier "sell" within my organization.
Permalink | Reply |
headers
Frank Mori Hess | 18 Jan 17:34
Favicon

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

On Friday 16 January 2009 16:35, Nat Goodspeed wrote:
> Johan Råde wrote:
> > 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.
>
> Second that! While I love the generality of the explicit tracking, I've
> been jumping through some hoops trying to adapt existing Boost.Signals
> code that uses boost::trackable.
>
> If signals2 could provide a thread-safe version of boost::trackable, it
> would make conversion a much easier "sell" within my organization.

I have added a thread-UNsafe boost::signals2::trackable to svn to ease 
porting of single-threaded Boost.Signals code that doesn't need to be made 
thread-safe.  

Would you give a little more detail on what parts of boost::trackable 
porting you've found the most painful?  Because to get thread-safe 
connection management in any form you'd still have to make your objects 
owned by shared_ptr and deal with the "no shared_from_this() in 
constructors" issue, since I don't think the release version of 
enable_shared_from_this supports use in constructors yet.

With respect to the "no shared_from_this() in constructors" issue, I've 
added a "deconstruct" factory function based on make_shared and Michael 
Marcin's make_shared_access patch.  It constructs the shared_ptr with a 
single allocation like make_shared and calls 
postconstructible::postconstruct like deconstruct_ptr.  Its use can be
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 11 Feb 21:08
Favicon

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

Sorry for the long delay... I got behind on the Boost list and am only 
gradually catching up.  :-(

>> Johan Råde wrote:

>>> Frank also mentioned the possibility of adding
>>> a thread safe version of boost::trackable
>>> based on boost::enable_shared_from_this.

Frank Mori Hess wrote:

> I have added a thread-UNsafe boost::signals2::trackable to svn to ease 
> porting of single-threaded Boost.Signals code that doesn't need to be made 
> thread-safe.  

That will be useful transitionally if we can count on a thread-safe 
boost::signals2::trackable arriving later. If signals2::trackable would 
only ever be thread-unsafe, it's not useful to us.

I recently introduced into our code some machinery based on 
boost::signal. Others are concurrently working on multithreading. I 
don't want my new functionality to become a source of difficult race 
bugs -- or even to be /perceived/ that way. I want to avoid a label of: 
"this mechanism is thread-unsafe, avoid it in all new code."

Accordingly, I've just replaced boost::signal with 
boost::signals2::signal, and so forth, throughout our code.

There were several existing uses of boost::trackable. I've coded around 
them in several ways, as described below. Since I didn't introduce them,
(Continue reading)

Permalink | Reply |
headers
Frank Mori Hess | 11 Feb 22:17
Favicon

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


On Wednesday 11 February 2009 15:08 pm, Nat Goodspeed wrote:
> Frank Mori Hess wrote:
> > I have added a thread-UNsafe boost::signals2::trackable to svn to ease
> > porting of single-threaded Boost.Signals code that doesn't need to be
> > made thread-safe.
>
> That will be useful transitionally if we can count on a thread-safe
> boost::signals2::trackable arriving later. If signals2::trackable would
> only ever be thread-unsafe, it's not useful to us.

It's intended for people who don't want to bother rewriting a lot of 
Boost.Signals code that doesn't need thread-safety.

> With an instance 'smartptr' of ListenerClass derived from
> boost::trackable, I'd like to write something like:
>
>      holler.listen(boost::bind(&ListenerClass::method, smartptr, _1));
>
> If my listen() method were able to tease apart the object returned from
> bind(), it could detect the case of a shared_ptr<boost::trackable
> subclass>.

It could, by using boost::visit_each.  boost::bind supports visit_each to let 
you apply a visitor to all the objects bound inside the returned functor.  
The signals libraries use visit_each to discover trackable objects that have 
been bound into slots.

> Since I don't know how to do that, I've introduced a number of
> alternative ways to get disconnect-on-destruction. I don't yet know
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 17 Feb 23:36
Favicon

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

Frank Mori Hess wrote:

>> If my listen() method were able to tease apart the object returned from
>> bind(), it could detect the case of a shared_ptr<boost::trackable
>> subclass>.
> 
> It could, by using boost::visit_each.  boost::bind supports visit_each to let 
> you apply a visitor to all the objects bound inside the returned functor.  
> The signals libraries use visit_each to discover trackable objects that have 
> been bound into slots.
> 
> I would spend some time learning how to use visit_each with bind.  You could 
> implement a wrapper function that inspects slots for your own Trackable base 
> class objects, and then takes whatever step is needed to extract a shared_ptr 
> and pass it to slot::track before connecting.

Thanks for the suggestion! I'm excited about this approach, but I've run 
into a snag.

As you note above, in addition to the case of my own Trackable base 
class, I want my visitor to detect a bound boost::shared_ptr<anything> 
and pass the shared_ptr to my new slot_type object's track() method. In 
fact, for a boost::shared_ptr<SomeTrackableSubclass>, I want the 
shared_ptr tracking to take precedence. As you said, this is the safer 
mechanism.

I can in fact detect a bound shared_ptr and pass it to track() as I 
want. The problem is that binding a shared_ptr captures a copy, so the 
referenced object will live until the connection is explicitly 
disconnected! That makes the slot_type::track() mechanism moot.
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 18 Feb 01:41
Favicon

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

Nat Goodspeed wrote:

> I want my visitor to detect a bound boost::shared_ptr<anything> 
> and pass the shared_ptr to my new slot_type object's track() method.

Frank, that functionality isn't specific to either boost::trackable or 
my own Trackable base class. Supposing we can resolve the problem of a 
bound copy of a shared_ptr (previous message in thread), couldn't the 
Signals2 library incorporate that logic itself?
Permalink | Reply |
headers
Frank Mori Hess | 18 Feb 15:33
Favicon

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


On Tuesday 17 February 2009, Nat Goodspeed wrote:
> Nat Goodspeed wrote:
> > I want my visitor to detect a bound boost::shared_ptr<anything>
> > and pass the shared_ptr to my new slot_type object's track() method.
>
> Frank, that functionality isn't specific to either boost::trackable or
> my own Trackable base class. Supposing we can resolve the problem of a
> bound copy of a shared_ptr (previous message in thread), couldn't the
> Signals2 library incorporate that logic itself?

I don't view the "dynamically replace bound shared_ptr with weak_ptr in 
incoming bind functor" idea as viable.  But if you mean the general feature 
of searching for some base class during connect and automatically tracking 
those objects, that might get added in the future.  I would do it strictly as 
an optional extension though, in seperate headers and only using the 
library's existing public interfaces.  It would add a free function findable 
by ADL that would look for the trackable base class when connecting, 
something like:

class shared_trackable: public enable_shared_from_this<shared_trackable>
{};

namespace bs2 = boost::signals2;

template<typename Signal, typename Func>
bs2::connection connect(Signal &sig, const Func &f)
{
  typename Signal::slot_type myslot(f);
  // apply visitor to f here which finds shared_trackable objects
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 18 Feb 17:48
Favicon

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

Frank Mori Hess wrote:

>> Nat Goodspeed wrote:

>>> I want my visitor to detect a bound boost::shared_ptr<anything>
>>> and pass the shared_ptr to my new slot_type object's track() method.

>> Frank, couldn't the
>> Signals2 library incorporate that logic itself?

> I don't view the "dynamically replace bound shared_ptr with weak_ptr in 
> incoming bind functor" idea as viable.  

Sigh, yeah -- that would seem to call for a whole new boost::bind() 
feature supporting copy-with-transformation.

> But if you mean the general feature 
> of searching for some base class during connect and automatically tracking 
> those objects, that might get added in the future.  I would do it strictly as 
> an optional extension though, in separate headers and only using the 
> library's existing public interfaces.  It would add a free function findable 
> by ADL that would look for the trackable base class when connecting, 
> something like:
> 
> class shared_trackable: public enable_shared_from_this<shared_trackable>
> {};

If I understand correctly, we expect the coder to pass to boost::bind() 
a plain pointer, a reference or a weak_ptr to a shared_trackable 
subclass instance. None of those will artificially prolong the life of
(Continue reading)

Permalink | Reply |
headers
Frank Mori Hess | 19 Feb 15:48
Favicon

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


On Wednesday 18 February 2009, Nat Goodspeed wrote:
> Frank Mori Hess wrote:
> > But if you mean the general feature
> > of searching for some base class during connect and automatically
> > tracking those objects, that might get added in the future.  I would do
> > it strictly as an optional extension though, in separate headers and only
> > using the library's existing public interfaces.  It would add a free
> > function findable by ADL that would look for the trackable base class
> > when connecting, something like:
> >
> > class shared_trackable: public enable_shared_from_this<shared_trackable>
> > {};
>
> If I understand correctly, we expect the coder to pass to boost::bind()
> a plain pointer, a reference or a weak_ptr to a shared_trackable
> subclass instance. None of those will artificially prolong the life of
> that instance. However, using enable_shared_from_this lets your visitor
> obtain a shared_ptr to pass to slot_type::track(). Yes?

Yes

>
> It's still the case that if the coder passes to boost::bind() an actual
> shared_ptr to the shared_trackable subclass instance, the
> slot_type::track() mechanism becomes irrelevant because the object won't
> die. That might warrant a note in the documentation for this feature.

Yes, that's why I suggested you could have the visitor give a compile error on 
finding a shared_ptr<shared_trackable> in the bind functor.
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 19 Feb 17:55
Favicon

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

Frank Mori Hess wrote:

> On Wednesday 18 February 2009, Nat Goodspeed wrote:

>> It's still the case that if the coder passes to boost::bind() an actual
>> shared_ptr to the shared_trackable subclass instance, the
>> slot_type::track() mechanism becomes irrelevant because the object won't
>> die. That might warrant a note in the documentation for this feature.

> Yes, that's why I suggested you could have the visitor give a compile error on 
> finding a shared_ptr<shared_trackable> in the bind functor.

I see. In fact, that compile error could be what prompts our coders to 
convert to weak_ptr when passing a shared_ptr to boost::bind().

Then there's the interesting question as to whether my visitor should 
complain about *any* shared_ptr in the boost::bind() object, since any 
such object becomes effectively immortal.

If my visitor handles weak_ptr<anything> by passing the corresponding 
shared_ptr<anything> to slot_type::track(), then it seems reasonable to 
me to force our coders to convert any shared_ptr to weak_ptr before 
boost::bind()ing it. Does that make sense?

In that case I don't really understand why my visitor would need to 
distinguish between shared_ptr<shared_trackable> and 
shared_ptr<anything_else> -- or weak_ptr<shared_trackable> and 
weak_ptr<anything_else>.

Put differently, shared_trackable only seems to me to make a difference
(Continue reading)

Permalink | Reply |
headers
Frank Mori Hess | 22 Feb 18:53
Favicon

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

On Thursday 19 February 2009, Nat Goodspeed wrote:
> Then there's the interesting question as to whether my visitor should
> complain about *any* shared_ptr in the boost::bind() object, since any
> such object becomes effectively immortal.
>
> If my visitor handles weak_ptr<anything> by passing the corresponding
> shared_ptr<anything> to slot_type::track(), then it seems reasonable to
> me to force our coders to convert any shared_ptr to weak_ptr before
> boost::bind()ing it. Does that make sense?
>
> In that case I don't really understand why my visitor would need to
> distinguish between shared_ptr<shared_trackable> and
> shared_ptr<anything_else> -- or weak_ptr<shared_trackable> and
> weak_ptr<anything_else>.
>
> Put differently, shared_trackable only seems to me to make a difference
> when the visitor encounters a plain pointer or reference to it. In that
> case, the visitor can use shared_trackable::shared_from_this() and still
> pass a shared_ptr to slot_type::track(). Am I overlooking something?

There is nothing inherently wrong with binding a shared_ptr to a slot.  
I've done it intentionally myself before.  It doesn't necessarily make 
anything immortal.  It's only a problem when the shared_ptr bound to the 
slot owns an object "A", and then slot is connected to a signal which also 
lives somewhere inside "A".  Then you have the usual shared_ptr-cycle 
problem, which can be broken by using a weak_ptr and slot::track.  But you 
may be binding a shared_ptr which owns an object "B" which is totally 
unrelated to object containing the signal you are connecting to.  Or, even 
if objects "A" and "B" are the same type, the objects might for example be 
arranged in some kind of tree where a shared_ptr owning child object "A"
(Continue reading)

Permalink | Reply |
headers
Frank Mori Hess | 18 Feb 14:57
Favicon

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


On Tuesday 17 February 2009, Nat Goodspeed wrote:
> As you note above, in addition to the case of my own Trackable base
> class, I want my visitor to detect a bound boost::shared_ptr<anything>
> and pass the shared_ptr to my new slot_type object's track() method. In
> fact, for a boost::shared_ptr<SomeTrackableSubclass>, I want the
> shared_ptr tracking to take precedence. As you said, this is the safer
> mechanism.
>
> I can in fact detect a bound shared_ptr and pass it to track() as I
> want. The problem is that binding a shared_ptr captures a copy, so the
> referenced object will live until the connection is explicitly
> disconnected! That makes the slot_type::track() mechanism moot.
>
> It looks as though I could only achieve what I want if my visit_each()
> visitor could *modify* the boost::bind object to replace the bound
> shared_ptr with its wrapped plain pointer. I don't believe this is
> possible?

Wouldn't it be better just to make your visitor just detect a bound shared_ptr 
to Trackable and report it, forcing the calling code to get it right?  You 
could use BOOST_STATIC_ASSERT to give a compile-time error, for example.
Permalink | Reply |
headers
Nat Goodspeed | 18 Feb 17:32
Favicon

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

Frank Mori Hess wrote:

> On Tuesday 17 February 2009, Nat Goodspeed wrote:

>> I can in fact detect a bound shared_ptr and pass it to track() as I
>> want. The problem is that binding a shared_ptr captures a copy, so the
>> referenced object will live until the connection is explicitly
>> disconnected! That makes the slot_type::track() mechanism moot.
>>
>> It looks as though I could only achieve what I want if my visit_each()
>> visitor could *modify* the boost::bind object to replace the bound
>> shared_ptr with its wrapped plain pointer. I don't believe this is
>> possible?

> Wouldn't it be better just to make your visitor just detect a bound shared_ptr 
> to Trackable and report it, forcing the calling code to get it right?  You 
> could use BOOST_STATIC_ASSERT to give a compile-time error, for example.

I'm sorry, I didn't express myself clearly. There are actually several 
different cases:

1. visitor discovers plain pointer/reference to a subclass of my 
Trackable base class:

Use Trackable to track the new connection.

This is working now -- but as you point out, it's less safe than your 
slot_type::track() mechanism since a Trackable might be destroyed during 
a slot call.
(Continue reading)

Permalink | Reply |
headers
Frank Mori Hess | 19 Feb 15:59
Favicon

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


On Wednesday 18 February 2009, Nat Goodspeed wrote:
> 1. visitor discovers plain pointer/reference to a subclass of my
> Trackable base class:
>
> Use Trackable to track the new connection.
>
> This is working now -- but as you point out, it's less safe than your
> slot_type::track() mechanism since a Trackable might be destroyed during
> a slot call.

There's also the fact that disconnect doesn't get called until the base class 
destructor (after the derived class destructors have already run).  Really, 
you might as well be using boost::signals2::trackable for this case, since it 
is equivalent to what you are doing (although I'd avoid this case entirely if 
you care about thread-safety).

> 2. visitor discovers a shared_ptr to something other than a Trackable
> subclass:
>
> This is the case I was asking about. My visitor can in fact pass the
> shared_ptr to slot_type::track(), but it's pointless because the
> shared_ptr copy stored in the boost::bind() result makes the referenced
> object effectively immortal.
>
> To my surprise, I find that it's not destroyed even when I explicitly
> disconnect the resulting connection.

It will get destroyed eventually.  The signal cleans up its slot list little 
by little during connect/invoke.  It doesn't immediately remove disconnected
(Continue reading)

Permalink | Reply |
headers
Nat Goodspeed | 19 Feb 19:08
Favicon

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

Frank Mori Hess wrote:

> On Wednesday 18 February 2009, Nat Goodspeed wrote:

>> 1. visitor discovers plain pointer/reference to a subclass of my
>> Trackable base class:
>>
>> Use Trackable to track the new connection.
>>
>> This is working now -- but as you point out, it's less safe than your
>> slot_type::track() mechanism since a Trackable might be destroyed during
>> a slot call.

> There's also the fact that disconnect doesn't get called until the base class 
> destructor (after the derived class destructors have already run).  Really, 
> you might as well be using boost::signals2::trackable for this case, since it 
> is equivalent to what you are doing

Now that it's been introduced [1], I will, thanks!

 > (although I'd avoid this case entirely if you care about thread-safety).

I understand and agree. Thanks for explaining the details of the 
vulnerabilities. This now becomes an awareness problem on our side.

We have existing uses of the original Boost.Signals library, and some 
existing classes derived from the original boost::trackable. I don't yet 
understand the authors' intent, or the typical lifespans of these 
classes. Nonetheless I'm trying to switch all such usage over to 
Signals2 with a minimum of disruption so that, as more threads are
(Continue reading)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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
Permalink | Reply |

Gmane