[Review.Coroutine] Some comments
Hi, thanks Oliver to move the Coroutine library to a Boost review ready proposal. Before doing a formal review, I have some comments, suggestions and questions after a quick reading. Design ==== * I don't know if showing the signature in the documentation will be more informative. This will avoid, in particular, the be forced to explain what T and R and in the the current declaration. Could you replace template< typename Signature, typename Allocator = ctx::stack_allocator
class coroutine { public: class self_t { public: T yield( R); void yield_break(); }; by template< typename Signature, typename Allocator = ctx::stack_allocator
class coroutine; template< typename R, typename ...ArgTypes, typename Allocator
class coroutine<R(ArgTypes...)> { public: class self_t { public: tuple<ArgTypes...>yield( R); void yield_break(); }; * Could you add the following noexcept specifier? class coroutine { // ... coroutine() BOOST_NOEXCEPT; coroutine( coroutine && other) BOOST_NOEXCEPT; coroutine & operator=( coroutine && other) BOOST_NOEXCEPT; explicitoperator bool() const BOOST_NOEXCEPT; bool is_complete() const BOOST_NOEXCEPT; //... }; void swap( coroutine & l, coroutine & r) BOOST_NOEXCEPT; The same applies to generator. * As the template class Allocator is not used to allocate internal memory of the coroutine class. Could the Allocator parameter be renamed StackAllocator? * Could you add an standard Allocator parameter as e.g. the one of packaged_task? 0. template <class F, class Allocator> explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f); C++ International Standard * I understand that the Boost.Context Allocator parameter is part od the class, as context is a low-level class, but coroutine is more high level. Is there a possibility to type erase the StackAllocator (and preceding Allocator) template parameter, as packaged_task do? This will allow to store coroutines (and generators) with the same signature in a container even if they don't share the Allocator. template<typename>class coroutine; template< typename R, typename ...ArgTypes
class coroutine<R(ArgTypes...)> { public: //... template< typename Fn,typename Allocator> generator( Fn fn, std::size_t size = ctx::default_stacksize(), flag_unwind_t do_unwind = stack_unwind, bool preserve_fpu = true, Allocator alloc = Allocator() ); //... }; * Parameters and move semantics. Could the signature parameters be rvalue references? R operator()(A0 a0, ..., A9 a9); I'm asking this because I think that Boost.Tuple has not implemented yet move semantics. * What about moving some of the coroutine/generator optional parameters to a coro::attributes class std::size_t size = ctx::default_stacksize(), flag_unwind_t do_unwind = stack_unwind, bool preserve_fpu = true, * Usually (at least in the C++ standard) a function doesn't throws because of its preconditions are not satisfied. Preconditions: |size| > ctx::minimum_stacksize(), |size| < ctx::maximum_stacksize() when ! ctx::is_stack_unbound(). Effects: Creates a generator which will execute |fn|. If |do_unwind| is |stack_unwind| the destructor of |*this| unwinds the stack before destructing it. If |preserve_fpu| is |true| the floating-point registers are preserved between context switches. Throws: /invalid_argument/ if a precondition is not satisfied. Should the function throw in this case? This should be coherent with |Result operator()()| <http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/coroutine/generator/generator.html#coroutine.generator.generator._code__phrase_role__identifier__result__phrase___phrase_role__keyword__operator__phrase__phrase_role__special________phrase___code_> Preconditions: |*this| is not a /not-a-generator/, |! is_complete()|. which doesn't throws any specific exception if the preconditions are not satisfied. . Documentation ========= coroutine.html s/it can migrated between threads/it can migrate between threads/ s/yield_return/yield_break/ * documentation for the following constructor is missing in the reference template< typename Fn > coroutine( Fn && fn, std::size_t size = ctx::default_stacksize(), flag_unwind_t do_unwind = stack_unwind, bool preserve_fpu = true, Allocator alloc = Allocator() ); Best, Vicente
Hi, most of Vicente's suggestions are done ... (git://gitorious.org/boost-dev/boost-dev.git, branch coroutine) Am 04.09.2012 23:06, schrieb Vicente J. Botet Escriba:
* Could you add an standard Allocator parameter as e.g. the one of packaged_task?
template <class F, class Allocator> explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f); boost::coroutine uses internally intrusive_ptr instead of shared_ptr - the trick of storing the rebounded allocator as deleter in shared_ptr is not possible (because intrusive_ptr has no such 'deleter' concept). I would not pay for shared_ptr because it is heavy weight compared to intrusiv_ptr (no need for atomic increment etc.). intrusive_ptr has only the size of a pointer.
On solution could be to make coroutine non-copyable, add support for intrusive_ptr (use-counter and **intrusive_ptr_add_ref()/ intrusive_ptr_release()). The user can decide how to allocate the coroutine (or use boost::optional). What do you think?
* Parameters and move semantics. Could the signature parameters be rvalue references?
R operator()(A0 a0, ..., A9 a9);
I'm asking this because I think that Boost.Tuple has not implemented yet move semantics.
boost::tuple does not handle moveable-only objects - so it will not work. we've to wiat until the supprot is added to boost::tuple. regards, Oliver
Hi Oliver,
boost::tuple does not handle moveable-only objects - so it will not work. we've to wiat until the supprot is added to boost::tuple.
Would it be possible to have a version (activated by a preprocessor macro) that uses std::tuple and supports movable-only objects. I often find myself dealing with moveable-only types. Regards, Eugene
Le 07/09/12 21:07, Oliver Kowalke a écrit :
Hi,
most of Vicente's suggestions are done ... (git://gitorious.org/boost-dev/boost-dev.git, branch coroutine)
Am 04.09.2012 23:06, schrieb Vicente J. Botet Escriba:
* Could you add an standard Allocator parameter as e.g. the one of packaged_task?
template <class F, class Allocator> explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f); boost::coroutine uses internally intrusive_ptr instead of shared_ptr - the trick of storing the rebounded allocator as deleter in shared_ptr is not possible (because intrusive_ptr has no such 'deleter' concept). I would not pay for shared_ptr because it is heavy weight compared to intrusiv_ptr (no need for atomic increment etc.). intrusive_ptr has only the size of a pointer. I'm not suggesting that you use shared_ptr. I guess there are other means to get this work, e.g doing it explicitly in the destructor and move operations, isn't it?
On solution could be to make coroutine non-copyable,
Is coroutine copyable?
add support for intrusive_ptr (use-counter and **intrusive_ptr_add_ref()/ intrusive_ptr_release()). The user can decide how to allocate the coroutine (or use boost::optional).
What do you think? I don't understand what you are proposing. Anyway I think exploring the Allocator concept interface is worth the effort.
* Parameters and move semantics. Could the signature parameters be rvalue references?
R operator()(A0 a0, ..., A9 a9);
I'm asking this because I think that Boost.Tuple has not implemented yet move semantics.
boost::tuple does not handle moveable-only objects - so it will not work. we've to wiat until the supprot is added to boost::tuple.
Have you take a look at the boost::fusion::tuple move C++11 evolution? Best, Vicente
Am 08.09.2012 10:48, schrieb Vicente J. Botet Escriba: > Le 07/09/12 21:07, Oliver Kowalke a écrit : >> Hi, >> >> most of Vicente's suggestions are done ... >> (git://gitorious.org/boost-dev/boost-dev.git, branch coroutine) >> >> Am 04.09.2012 23:06, schrieb Vicente J. Botet Escriba: >>> * Could you add an standard Allocator parameter as e.g. the one of >>> packaged_task? >>> >>> template <class F, class Allocator> >>> explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f); >> boost::coroutine uses internally intrusive_ptr instead of shared_ptr >> - the trick of storing the rebounded allocator as deleter in >> shared_ptr is not possible (because intrusive_ptr has no such >> 'deleter' concept). >> I would not pay for shared_ptr because it is heavy weight compared to >> intrusiv_ptr (no need for atomic increment etc.). intrusive_ptr has >> only the size of a pointer. > I'm not suggesting that you use shared_ptr. I guess there are other > means to get this work, e.g doing it explicitly in the destructor and > move operations, isn't it? I can't imagine that it will work. Anyway, I could store an delete-functor (calls allocator::destroy() + deallocate()) in context_base as shared_ptr does and call this delete functor from intrusive_ptr_release. Let's see if it will work. >> >> On solution could be to make coroutine non-copyable, > > Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable > >> >>> * Parameters and move semantics. Could the signature parameters be >>> rvalue references? >>> >>> R operator()(A0 a0, ..., A9 a9); >>> >>> I'm asking this because I think that Boost.Tuple has not implemented >>> yet move semantics. >> boost::tuple does not handle moveable-only objects - so it will not >> work. we've to wiat until the supprot is added to boost::tuple. >> > Have you take a look at the boost::fusion::tuple move C++11 evolution? I'll take a look into the docu
Le 08/09/12 14:30, Oliver Kowalke a écrit : > Am 08.09.2012 10:48, schrieb Vicente J. Botet Escriba: >> Le 07/09/12 21:07, Oliver Kowalke a écrit : >>> Hi, >>> >>> most of Vicente's suggestions are done ... >>> (git://gitorious.org/boost-dev/boost-dev.git, branch coroutine) >>> >>> Am 04.09.2012 23:06, schrieb Vicente J. Botet Escriba: >>>> * Could you add an standard Allocator parameter as e.g. the one of >>>> packaged_task? >>>> >>>> template <class F, class Allocator> >>>> explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f); >>> boost::coroutine uses internally intrusive_ptr instead of shared_ptr >>> - the trick of storing the rebounded allocator as deleter in >>> shared_ptr is not possible (because intrusive_ptr has no such >>> 'deleter' concept). >>> I would not pay for shared_ptr because it is heavy weight compared >>> to intrusiv_ptr (no need for atomic increment etc.). intrusive_ptr >>> has only the size of a pointer. >> I'm not suggesting that you use shared_ptr. I guess there are other >> means to get this work, e.g doing it explicitly in the destructor and >> move operations, isn't it? > I can't imagine that it will work. > Anyway, I could store an delete-functor (calls allocator::destroy() + > deallocate()) in context_base as shared_ptr does and call this delete > functor from intrusive_ptr_release. Let's see if it will work. I'm sure you will find a good solution. > >>> >>> On solution could be to make coroutine non-copyable, >> >> Is coroutine copyable? > moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here? > >> >>> >>>> * Parameters and move semantics. Could the signature parameters be >>>> rvalue references? >>>> >>>> R operator()(A0 a0, ..., A9 a9); >>>> >>>> I'm asking this because I think that Boost.Tuple has not >>>> implemented yet move semantics. >>> boost::tuple does not handle moveable-only objects - so it will not >>> work. we've to wiat until the supprot is added to boost::tuple. >>> >> Have you take a look at the boost::fusion::tuple move C++11 evolution? > I'll take a look into the docu I don't know when these should be released, but I know they are working on it. In any case I expect that boost::fusion::tuple will manage with move semantics one day, but to my knowledge no body is working on boost::tuple. Maybe someone has fresh infos. Best, Vicente
Am 08.09.2012 16:25, schrieb Vicente J. Botet Escriba:
On solution could be to make coroutine non-copyable,
Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here?
I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used. But I think I've a solution to accept and hold an allocator in coroutine
* Parameters and move semantics. Could the signature parameters be rvalue references?
R operator()(A0 a0, ..., A9 a9);
I'm asking this because I think that Boost.Tuple has not implemented yet move semantics. boost::tuple does not handle moveable-only objects - so it will not work. we've to wiat until the supprot is added to boost::tuple.
Have you take a look at the boost::fusion::tuple move C++11 evolution?
I'll take a look into the docu
I don't know when these should be released, but I know they are working on it. In any case I expect that boost::fusion::tuple will manage with move semantics one day, but to my knowledge no body is working on boost::tuple. Maybe someone has fresh infos.
OK regards, Oliver
Am 08.09.2012 16:25, schrieb Vicente J. Botet Escriba:
On solution could be to make coroutine non-copyable,
Is coroutine copyable?
moveable-only - I mean to derive from boost::noncopyable
I'm lost. How this can help here?
I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used.
Well, if it were my design, I would make coroutine non-copyable and simply forget about all the related troubles of copy semantics. Furthermore, I would entirely avoid the use of dynamically allocated resources and a coroutine allocator. But, of course, I am predominantly interested in using coroutine in embedded systems that lack a heap and benefit from the predictability of statically allocated resources. As you already mentioned in various communications, though, we can work on a lightweight embedded version in the future. Whatever you do, keep up the good work! Best regards, Chris.
regards, Oliver
Le 08/09/12 20:05, Christopher Kormanyos a écrit :
Am 08.09.2012 16:25, schrieb Vicente J. Botet Escriba:
On solution could be to make coroutine non-copyable, Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here? I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used. Well, if it were my design, I would make coroutine non-copyable and simply forget about all the related troubles of copy semantics. Furthermore, I would entirely avoid the use of dynamically allocated resources and a coroutine allocator.
But, of course, I am predominantly interested in using coroutine in embedded systems that lack a heap and benefit from the predictability of statically allocated resources. As you already mentioned in various communications, though, we can work on a lightweight embedded version in the future.
The suggested Allocator is just there to responds to your need (predictability) :) Best, Vicente
Am 08.09.2012 21:57, schrieb Vicente J. Botet Escriba:
Le 08/09/12 20:05, Christopher Kormanyos a écrit :
Am 08.09.2012 16:25, schrieb Vicente J. Botet Escriba:
> On solution could be to make coroutine non-copyable, Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here? I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used. Well, if it were my design, I would make coroutine non-copyable and simply forget about all the related troubles of copy semantics. Furthermore, I would entirely avoid the use of dynamically allocated resources and a coroutine allocator.
But, of course, I am predominantly interested in using coroutine in embedded systems that lack a heap and benefit from the predictability of statically allocated resources. As you already mentioned in various communications, though, we can work on a lightweight embedded version in the future.
The suggested Allocator is just there to responds to your need (predictability) :) Vicente - I looked at packaged_task as you suggested. I found following code:
packaged_task() : { typedef typename Allocator::template rebind<detail::task_object<R,FR>
::other A2; A2 a2(a); // allocated on the stack, after return from ctor destroyed typedef thread_detail::allocator_destructor<A2> D; task = task_ptr(::new(a2.allocate(1)) detail::task_object<R,FR>(f), D(a2, 1) ); // a2 passed to allocator_destructor }
template <class _Alloc> class allocator_destructor { ... _Alloc& alloc_; // reference to A2 size_type s_; public: allocator_destructor(_Alloc& a, size_type s)BOOST_NOEXCEPT : alloc_(a), s_(s) {} ... }; allocator_destructor hold only a reference to the allocator A2 but the instance was allocated on the stack. Doesn't this mean that the alloc_ member of allocator_destructor will point to a non-existing object? Oliver
Le 08/09/12 22:15, Oliver Kowalke a écrit :
Am 08.09.2012 21:57, schrieb Vicente J. Botet Escriba:
The suggested Allocator is just there to responds to your need (predictability) :)
Vicente - I looked at packaged_task as you suggested. I found following code:
packaged_task() : { typedef typename Allocator::template rebind<detail::task_object<R,FR>
::other A2; A2 a2(a); // allocated on the stack, after return from ctor destroyed typedef thread_detail::allocator_destructor<A2> D; task = task_ptr(::new(a2.allocate(1)) detail::task_object<R,FR>(f), D(a2, 1) ); // a2 passed to allocator_destructor }
template <class _Alloc> class allocator_destructor { ... _Alloc& alloc_; // reference to A2 size_type s_; public: allocator_destructor(_Alloc& a, size_type s)BOOST_NOEXCEPT : alloc_(a), s_(s) {} ... };
allocator_destructor hold only a reference to the allocator A2 but the instance was allocated on the stack. Doesn't this mean that the alloc_ member of allocator_destructor will point to a non-existing object?
Yes, you are right. The code is using something not very clean that works only for stateless allocators. I guess that storing the allocator on allocator_destructor should solve the issue. Please could you create a ticket to track the issue? Best, Vicente
Am 08.09.2012 21:57, schrieb Vicente J. Botet Escriba:
Le 08/09/12 20:05, Christopher Kormanyos a écrit :
Am 08.09.2012 16:25, schrieb Vicente J. Botet Escriba:
> On solution could be to make coroutine non-copyable, Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here? I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used. Well, if it were my design, I would make coroutine non-copyable and simply forget about all the related troubles of copy semantics. Furthermore, I would entirely avoid the use of dynamically allocated resources and a coroutine allocator.
But, of course, I am predominantly interested in using coroutine in embedded systems that lack a heap and benefit from the predictability of statically allocated resources. As you already mentioned in various communications, though, we can work on a lightweight embedded version in the future.
The suggested Allocator is just there to responds to your need (predictability) :)
solved - allocator can be passed to coroutine/generators ctor : git://gitorious.org/boost-dev/boost-dev.git (branch coroutine)
> On solution could be to make coroutine non-copyable,
Is coroutine copyable? moveable-only - I mean to derive from boost::noncopyable I'm lost. How this can help here? I was thinking about to make coroutine deriving from boost::noncopyable and not moveable-only. Then I could refactor coroutine that it does not need to allocate memory for inner classes. The user can decide if the coroutine instance is allocated on the stack or in the freestore and which technique is used.
Well, if it were my design, I would make coroutine non-copyable and simply forget about all the related troubles of copy semantics. Furthermore, I would entirely avoid the use of dynamically allocated resources and a coroutine allocator.
But, of course, I am predominantly interested in using coroutine in embedded systems that lack a heap and benefit from the predictability of statically allocated resources. As you already mentioned in various communications, though, we can work on a lightweight embedded version in the future.
The suggested Allocator is just there to responds to your need (predictability) :)
Best, Vicente
I understand. Thank you. Best regards, Chris.
participants (4)
-
Christopher Kormanyos -
Eugene Yakubovich -
Oliver Kowalke -
Vicente J. Botet Escriba