[thread_safe_signals][signals2] call for reviewers (review tentatively scheduled Nov 1st - Nov 10th)
Hello all, The review for the Signals2 library (formerly known as thread_safe_signals - description below) submitted by Frank Mori Hess has been tentatively scheduled for Nov 1st - Nov 10th. As per a recent discussion on the review process (http://tinyurl.com/48tdjs), and after consulting with the review wizards, we would like to try something new for this review: have a number of reviewers commit beforehand to submitting a review. The goal of signing up is to ensure that there is both sufficient interest in the library, and that there will be enough reviews submitted for an informed decision on acceptance. If you are interested in this library, I encourage you to sign up now to be a reviewer (by this you commit to submitting a review preferably by the end of the review period). Of course, additional reviews from non-signed-up reviewers will also be welcome, but without enough reviewers signing up beforehand it is not clear that we should proceed with a review at this time. To sign up, please send me a reply (here or privately), and let me know whether the review period is suitable for you (we can make some adjustments if necessary). 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). I you would like to sign up, please let me know by this Saturday (October 18th). About the library: -------- * The library can be downloaded from: http://www.boostpro.com/vault/index.php?&directory=thread_safe_signals (latest version is signals2-2008-10-08.zip) * Documentation is also available online: http://www.comedi.org/projects/signals2/libs/signals2/doc/html/index.html * A synopsis: The Boost.Signals2 library (formerly known as thread_safe_signals) is an implementation of a managed signals and slots system. Signals represent callbacks with multiple targets, and are also called publishers or events in similar systems. Signals are connected to some set of slots, which are callback receivers (also called event targets or subscribers), which are called when the signal is "emitted." Signals and slots are managed, in that signals and slots (or, more properly, objects that occur as part of the slots) can track connections and are capable of automatically disconnecting signal/slot connections when either is destroyed. This enables the user to make signal/slot connections without expending a great effort to manage the lifetimes of those connections with regard to the lifetimes of all objects involved. When signals are connected to multiple slots, there is a question regarding the relationship between the return values of the slots and the return value of the signals. Boost.Signals2 allows the user to specify the manner in which multiple return values are combined. * Relationship to Boost.Signals: This is a thread-safe variant of the original Boost.Signals library. There have been some changes to the interface to support thread-safety, mostly with respect to automatic connection management. The following thread offers some more details on the differences between the two implementations, as well as a plan of a phased replacement of Boost.Signals should Signals2 be accepted: http://tinyurl.com/4sqau3 [nabble] Again, if you are interested in this library, please consider signing up as a reviewer. Kind regards, Stjepan (review manager)
Hi Stjepan, Stjepan Rajko wrote: ...
* Relationship to Boost.Signals: This is a thread-safe variant of the original Boost.Signals library. There have been some changes to the interface to support thread-safety, mostly with respect to automatic connection management.
The following thread offers some more details on the differences between the two implementations, as well as a plan of a phased replacement of Boost.Signals should Signals2 be accepted: http://tinyurl.com/4sqau3 [nabble]
It might be more expeditious to directly paste Frank's points from his email in the above thread. I've pasted them below. Is the intent to only review these changes? It might help if there were a summary section on the diffs with this new version. Glancing over the documentation it wasn't clear where to find the differences other than the section mentioned above for Automatic Connection Management. Are there examples demonstrating new usage patterns? Is there any info on the cost of this thread safe implementation vs. the signals1 in a single threaded application? --- * There is an author change, and the new author (me) has never submitted a lib to boost before. A few of the headers in thread_safe_signals did start as copies of boost.signals headers, but it's mostly a new implementation. * The scheme for managing connections is different, people should probably have an opportunity to disparage it before it gets forced on them. There are also some new classes for supporting postconstructors/predestructors (which may not be needed after all, if I can get my changes to shared_ptr/enable_shared_from_this accepted). * It's not fully backwards compatible. I dropped the old connection management scheme (boost::trackable and visit_each) entirely. It could be made to be (almost) fully backward compatiible for source code, if that's deemed necessary. However, it's also been pointed out that now may be a good time to do some namespace and header location rearrangement to bring signals more into line with current boost policies. Actually, I'm already aware that some of my new classes (postconstructible/deconstruct_ptr/etc.) that I dumped into the boost namespace should be moved. It might be worth considering continuing to provide the old boost.signals library for a transition period, moving thread_safe_signals into a new namespace/header location entirely? * thread_safe_signals is header-only, boost.signals is not. I'm sure some people will see this as a change for the worse. --- Jeff
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 14 October 2008 09:11 am, Jeff Flinn wrote:
It might help if there were a summary section on the diffs with this new version. Glancing over the documentation it wasn't clear where to find the differences other than the section mentioned above for Automatic Connection Management.
There is an entry in the FAQ section which has a summarized list of changes.
Are there examples demonstrating new usage patterns?
The tutorial section of the docs has been updated to the new API. There is no example code for the signal::extended_slot_type and signal::connect_extended() stuff I squeezed in at the last minute, but they are in the reference section.
Is there any info on the cost of this thread safe implementation vs. the signals1 in a single threaded application?
The only benchmarking/optimization I've done is to compare the invocation speed of a signal with zero slots connected. IIRC signals2 was slightly faster. It is probably still possible to optimize the automatic connection tracking overhead during invocation, maybe using a custom allocator for the vector of tracked shared_ptr. I don't know anything about the relative memory usage of the two libs. As far as single-threaded apps go, the signal class does have a Mutex template type parameter, and the library provides signals2::dummy_mutex which can be used to eliminate locking overhead. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFI9KcE5vihyNWuA4URAu34AKCnCOF4isPA25bEhJ3blPHOoeUsQQCeIvkN IkQ+bdpEt9m4RWZPGJoh/VM= =U/k4 -----END PGP SIGNATURE-----
----- Original Message ----- From: "Frank Mori Hess" <frank.hess@nist.gov> To: <boost@lists.boost.org> Sent: Tuesday, October 14, 2008 4:04 PM Subject: Re: [boost] [thread_safe_signals][signals2] call for reviewers(review tentatively scheduled Nov 1st - Nov 10th)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Tuesday 14 October 2008 09:11 am, Jeff Flinn wrote:
It might help if there were a summary section on the diffs with this new version. Glancing over the documentation it wasn't clear where to find the differences other than the section mentioned above for Automatic Connection Management.
There is an entry in the FAQ section which has a summarized list of changes.
This is a good starting point, but in my opinion it should appear in the introduction or on a history section. It would be interesting to sum up in a table which classes/functions are compatible, which are not supported/depreceated and which are new, which new features can be adopted to Boost.Signal? I expect also some examples that show how the non supported/depreceated Boost.Signal are replaced by the equivalent Boost.Signal2. You don't think this must be done before the review?
Are there examples demonstrating new usage patterns?
The tutorial section of the docs has been updated to the new API. There is no example code for the signal::extended_slot_type and signal::connect_extended() stuff I squeezed in at the last minute, but they are in the reference section.
Do you plan to add some tutorial/examples of these new features before the review?
Is there any info on the cost of this thread safe implementation vs. the signals1 in a single threaded application?
The only benchmarking/optimization I've done is to compare the invocation speed of a signal with zero slots connected. IIRC signals2 was slightly faster. It is probably still possible to optimize the automatic connection tracking overhead during invocation, maybe using a custom allocator for the vector of tracked shared_ptr. I don't know anything about the relative memory usage of the two libs.
Where can we find this benchmarking, on the documentation?
As far as single-threaded apps go, the signal class does have a Mutex template type parameter, and the library provides signals2::dummy_mutex which can be used to eliminate locking overhead.
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. BTW, this dummy mutex should be a good candidate to the thread library. There is already one null_mutex class in the interprocess library. Best, Vicente
On Tuesday 14 October 2008 13:53, vicente.botet wrote:
----- Original Message ----- From: "Frank Mori Hess" <frank.hess@nist.gov>
There is an entry in the FAQ section which has a summarized list of changes.
This is a good starting point, but in my opinion it should appear in the introduction or on a history section. It would be interesting to sum up in a table which classes/functions are compatible, which are not supported/depreceated and which are new, which new features can be adopted to Boost.Signal? I expect also some examples that show how the non supported/depreceated Boost.Signal are replaced by the equivalent Boost.Signal2. You don't think this must be done before the review?
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. 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. 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.
There is no example code for the signal::extended_slot_type and signal::connect_extended() stuff I squeezed in at the last minute, but they are in the reference section.
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. There's really not much to it, connect_extended() is just like connect() except it takes a signal::extended_slot_type instead of a signal::slot_type. And a signal::extended_slot_type is just like a signal::slot_type except it takes an extra first argument which is the slots connection to the signal invoking it. I described my motivation for adding it a couple weeks ago in this post: http://lists.boost.org/Archives/boost/2008/10/142995.php That post shows the connection argument added as the last argument though, I ended up putting it as the first argument.
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
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, 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. 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. 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. 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 :)
----- Original Message ----- From: "Frank Mori Hess" <fmhess@speakeasy.net> To: <boost@lists.boost.org> 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@nist.gov>
<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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 15 October 2008 13:58 pm, vicente.botet wrote:
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?
I'm not certain this is what you're asking, but yes it would be possible to implement a signals2::trackable on top of the signals2 automatic connection management scheme (in fact I did have it implemented at one point). It wouldn't be thread-safe, but it could do everything the boost::trackable from Boost.Signals does.
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.
Oh. Yes, some benchmarking would be good to post to this list at the beginning of the review. I'll try to do that if we get enough reviewers to run with. Probably it would be good add a benchmark with a signal invoking slots with connection tracking enabled (a scenario I imagine Boost.Signals would win over Signals2). Of course, anyone could benchmark and post the results.
Are you sure that boost::mutex is not header only?
Yes, unless it has been fixed recently. Both it and the lock classes pull in exception classes that are defined only in the compiled thread library.
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?
Yes, any mutex that models the simple Lockable concept from Boost.Thread can be used. Hmm, I meant to document that fact in the signals2::signal reference but it looks like I forgot. I only mention the Lockable concept and boost::mutex in the reference of signals2::mutex.
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?
I got the impression from Anthony that boost::mutex was intended to be header-only, and he intends to fix the problem. I also posted a patch to detail::lightweight_mutex which makes it support the Lockable concept http://www.nabble.com/-lightweight_mutex--Patch-to-support-new-thread-api-td... I'll gladly drop my implementation of signals2::mutex (which is a patched version of detail::lightweight mutex) when either boost::mutex or detail::lightweight_mutex becomes a suitable replacement. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFI97Hd5vihyNWuA4URAk+JAJ4mvrNSAT/7Ut7l6b+wm2UH8knnzgCeKjYC reXT+mc9BFqtZAe08geVKj0= =lXMy -----END PGP SIGNATURE-----
Hi Jeff, On Tue, Oct 14, 2008 at 6:11 AM, Jeff Flinn <TriumphSprint2000@hotmail.com> wrote:
It might be more expeditious to directly paste Frank's points from his email in the above thread. I've pasted them below. Is the intent to only review these changes?
Thanks for pasting that in. I think any issue related to the library would be appropriate to raise during the review, even if it would apply to the current Boost.Signals library (reviews are good times to figure things out :-)). The original Boost.Signals review discussion can be found here: http://lists.boost.org/Archives/boost/2002/02/index.php I'm not sure how applicable that discussion would be now since I imagine the library has evolved quite a bit over the years, but it might be useful to revisit if we end up discussing things that apply to Boost.Signals as well as the proposed library.
Is there any info on the cost of this thread safe implementation vs. the signals1 in a single threaded application?
I assume that the following comparison was done for a single threaded app (using dummy_mutex), but if not I suspect the results would be an upper bound for the single threaded case: http://www.nabble.com/boost%3A%3Asignal-poor-performance-td19602422.html#a19... Best, Stjepan
On Mon, Oct 13, 2008 at 3:40 PM, Stjepan Rajko <stipe@asu.edu> wrote:
The review for the Signals2 library (formerly known as thread_safe_signals - description below) submitted by Frank Mori Hess has been tentatively scheduled for Nov 1st - Nov 10th. As per a recent discussion on the review process (http://tinyurl.com/48tdjs), and after consulting with the review wizards, we would like to try something new for this review: have a number of reviewers commit beforehand to submitting a review.
I will *definitely* be reviewing this library. - Doug
----- Original Message ----- From: "Doug Gregor" <doug.gregor@gmail.com> To: <boost@lists.boost.org> Sent: Tuesday, October 14, 2008 5:33 PM Subject: Re: [boost] [thread_safe_signals][signals2] call for reviewers(review tentatively scheduled Nov 1st - Nov 10th)
On Mon, Oct 13, 2008 at 3:40 PM, Stjepan Rajko <stipe@asu.edu> wrote:
The review for the Signals2 library (formerly known as thread_safe_signals - description below) submitted by Frank Mori Hess has been tentatively scheduled for Nov 1st - Nov 10th. As per a recent discussion on the review process (http://tinyurl.com/48tdjs), and after consulting with the review wizards, we would like to try something new for this review: have a number of reviewers commit beforehand to submitting a review.
I will *definitely* be reviewing this library.
- Doug
Hi, The name of Boost.Signal2 seems as the V2 of Boost.Signal. Is it the case? Vicente
On Tue, Oct 14, 2008 at 9:45 AM, vicente.botet <vicente.botet@wanadoo.fr> wrote:
Hi, The name of Boost.Signal2 seems as the V2 of Boost.Signal. Is it the case?
Hmm... perhaps in a similar way to how Phoenix is planned to be Lambda V2, yes. Signals2 is a reimplementation of Boost.Signals to support thread safety, and if accepted, the plan mentioned by Doug Gregor and Frank Mori Hess would be to do a phased replacement of Boost.Signals by Boost.Signals2. I believe most of the implementation has been rewritten, but most of the interface kept the same (with additions related to thread-safety). Regards, Stjepan
----- Original Message ----- From: "Stjepan Rajko" <stipe@asu.edu> To: <boost@lists.boost.org> Sent: Tuesday, October 14, 2008 7:00 PM Subject: Re: [boost] [thread_safe_signals][signals2] call forreviewers(review tentatively scheduled Nov 1st - Nov 10th)
On Tue, Oct 14, 2008 at 9:45 AM, vicente.botet <vicente.botet@wanadoo.fr> wrote:
Hi, The name of Boost.Signal2 seems as the V2 of Boost.Signal. Is it the case?
Hmm... perhaps in a similar way to how Phoenix is planned to be Lambda V2, yes. Signals2 is a reimplementation of Boost.Signals to support thread safety, and if accepted, the plan mentioned by Doug Gregor and Frank Mori Hess would be to do a phased replacement of Boost.Signals by Boost.Signals2. I believe most of the implementation has been rewritten, but most of the interface kept the same (with additions related to thread-safety).
Regards,
Stjepan
Thanks, now it is clear. I think that it is very important if the autors plan a replacement. Vicente
On Tue, Oct 14, 2008 at 8:33 AM, Doug Gregor <doug.gregor@gmail.com> wrote:
On Mon, Oct 13, 2008 at 3:40 PM, Stjepan Rajko <stipe@asu.edu> wrote:
The review for the Signals2 library (formerly known as thread_safe_signals - description below) submitted by Frank Mori Hess has been tentatively scheduled for Nov 1st - Nov 10th. As per a recent discussion on the review process (http://tinyurl.com/48tdjs), and after consulting with the review wizards, we would like to try something new for this review: have a number of reviewers commit beforehand to submitting a review.
I will *definitely* be reviewing this library.
Thank you - you saved me the trouble of pestering you otherwise :-) Stjepan
participants (6)
-
Doug Gregor -
Frank Mori Hess -
Frank Mori Hess -
Jeff Flinn -
Stjepan Rajko -
vicente.botet