Re: [boost] [coroutine][review] Very Late Review

Subject: Re: [boost] [coroutine][review] Very Late Review
From: Giovanni Piero Deretta (gpderetta_at_[hidden])
Date: 2012-09-19 12:18:26

On Tue, Sep 18, 2012 at 9:37 PM, Oliver Kowalke <oliver.kowalke_at_[hidden]>wrote:

> Am 17.09.2012 18:24, schrieb Giovanni Piero Deretta:
>
>> I like the idea that has been proposed during the review that the result
>> of
>> a coroutine should be retrivable at any time via a get method.I dislike
>>
>> the proposed optional<T> result for operator(). If the previous delayed
>> result functionality is implemented, operator() should just return *this,
>> so that the subsequent get() call can be chained or the result tested for
>> non empty. This change has the potential of deeply affecting the
>> interface,
>> so I would really like to see it subject to a mini review before
>> acceptance.
>>
> I would not add a coroutine<>::get() method because a coroutine is a
> 'special function' (in the sense of multiple entry/leave). AFAIK
> boost::function also does not have a get() method.

well, if I read your discussion with Vicente correctly,you are planning to
add either a bind or a get method to self to bind parameters. For simmetry
the same solution should be used for the coroutine itself, especially if I
get to convince you to use the same class template for both the coroutine
and the self object.

>
>  It has been suggested that coroutine::self be removed an have a thread
>> specific object. I strongly disagree with this option. First of all it
>> will
>> make it too easy to break type safety:
>>
>> typedef this_coroutine<int(int)> this_coro;
>>
>> typedef this_coroutine<void()> that_coro;
>>
>> coroutine<int(int)>([]{
>>         that_coro::yield(); // oops compiles fine, UB at runtime.
>> });
>>
> that_coro::yield() could assert if the internal 'control block'
> (coroutine_base) is not
> that of the current coroutine - of course at runtime.

Not sure how would you implement the above. The only way I can think is to
keep the signature at runtime (with typeinfo or equivalent) and compare it
with the yield signature, but such a comparison might be expensive.

>
>
>  In practice, one need to pass the signature to any code that wants to
>> yield
>> anyway, so might as well pass the self object around, at least one can
>> rely
>> on template type deduction.
>>
> some of the community members are not happy with tracking the self object
> trough the code

You could make the same argument for every function that takes a callback
(for example asio callbacks,  or even for_each): instead of making the
callback arguments explicit, you may thread them thru a sort-of dynamically
scoped variable. Yes, is convenient (this is what Perl sometimes does) but
I would argue it is not a good design (again, Perl).

The user has always the possiblity of adding this functionality himself.

>
>
>  Also, one does not really want arbitrary code to yield out of the
>> coroutine
>> as this can be dangerous (think of code between yield statements as a sort
>> of critical section) as invariants can be broken or mutices held.
>>
> you can not prevent user doing such stuff, IMHO

Of course you can't but that doesn't mean you have to hand the user aloaded
gun :)

>
>
>  Instead I suggest doing away with a separate self type and instead making
>> the first parameter to the coroutine functor a full fledged coroutine,
>> with
>> simmetric signature:
>>
>> coroutine<int(void)> my_coroutine([](coroutine<**void(int)> caller) {
>>                caller(10);
>> });
>>
>> int i = my_coroutine();
>>
> I'm sure that users asking for a typedef in coroutine<int(void)> for the
> first parameter of the coroutine function
>
> typedef coroutine< int(void) > coro_t;
> typedef coro_t::caller_t  caller_t;
>
> coro_t my_coroutine( []( caller_t caller) {
>     caller( 10);
>
> });
>
> int i = my_coroutine();
>
> in the case of the reviewed library the coro_t::caller_t from the example
> above is equivalent to
> coro_t::self_t provided by the lib. The coro_t::self_t type is not a
> instance of the original coroutine, it is a new instance
> of coroutine_self<> (not derived from a coroutine's base classes).
> If I would rename coroutine_self<>::yield() to
> coroutine_self<>::operator() I get pretty much the same as you described in
> your example. Of course self_t is not a coroutine but it provides
> 'coroutine' - semantics
> (== context jumps).

I'm strongly arguing that they should be exactly the same type (well,
except for the symmetric signature) and have exactly the same behaviour.
Removing the distinction between the calle and the caller would actually
simplify the implementation and will also be truer to the 'coroutine' name.

 I do not like the separation between coroutines and generators. Instead the
> generator class should be dropped.
>
I disagree because a coroutine<> is a 'specialized' routine and a
> generator<> provides a sequence of elements. the requirement of coroutine<>
> is that its coroutine-fn must have return value (a last return statement in
> its body) and
> a generator<> requires that the generator-fn returns always void (return
> values passed via self_t::yield()).

I won't argue strongly about this. Iff you add the bind/get parameter to
caller_t and unifiy caller_t and coroutine, then you no longer have an
useful distinction between the two.

Also note that the current design (coroutine + generator) still leaves the
output iterator space unfilled.

>
>
>  The reason that I had generator as a
>> separate class in the original coroutine SoC is because I wanted to give
>> it
>> iterator semantics (this also required internally using a reference
>> counted
>> pointer to make the iterator copiable, which I disliked).
>>
> a requirement of iterators is that its source/container is still valid
> during its live-time.
> why not require the same for generators? the iterators of generator<> take
> only a reference to the associated generator object.

Sure, of course.

>
>
>  Instead I propose
>> to make every coroutine<T()> fulfill the the input range concept and every
>> coroutine<void(T)> fulfill the output range concept (this is a
>> functionality not covered by generators). In practice it means that for
>> these classes of coroutines, begin and end are defined returning
>> appropriate iterator (i.e. some very thin layers around the coroutine
>> itself). As a range is required to survive its iterator anyway, the
>> iterators can simply use plain pointers to the coroutine.
>>
>> The last suggestion works very well if coupled with the suggestion of
>> making 'self' a full coroutine:
>>
>> std::vector<int> a = { ... };
>> std::vector<int> b = { ... };
>>
>> std::cout << "The result of 'merge(a,b) is:\n";
>> for(auto x : callcc([&](coroutine<void(int)**> c) {
>>  std::merge(a.begin(),
>> a.end(), b.begin(), b.end(), c.begin()); })
>> {
>>          std::cout << x << "\n";
>> }
>>
>> I thought modelling generalized coroutines<T(T)> as ranges, but I have yet
>> to find a good model.
>>
> I've read the docu of boost.range - the requirements of a single pass
> range is:
>
> - For any Range |a|, |[boost::begin(a),boost::end(**a))| is a valid
> range, that is, |boost::end(a)| is reachable from |boost::begin(a)| in a
> finite number of increments.
>    -> might be violated by an infinite loop inside the coroutine-fn
>

technically violated, but in practice, because of the halting problem, the
two are are indistinguishable.

>
> - . A Range provides iterators for accessing a half-open range
> |[first,one_past_last)| of elements and provides information about the
> number of elements in the Range.
>    -> it is for a coroutine or a generator impossible to determine the
> number of elements
>

so is for an input range and an output range. What about a range of
boost::function_input_iterators? I'll argue that Boost.Range concept is
overconstrained.

Also Boost.Range requires the range to be copyable. This means that in
c++11, std::vector<my_movable_only_type> would not be a range. The
requirement should be relaxed to movable.

>
> is it correct to apply the range concept to coroutine/generator?

Yes!

>
>
>
>  The coroutine object itself holds a reference counted pointer to the
>> implementation. As the coroutine is movable but not copyable, I do not
>> think that reference counting is needed.
>>
> The implementation uses a intrusive pointer - it has the footprint
> (memory) of a raw pointer and the functions
> used to increment/decrement the ref-counter are inlined - you pay only of
> incrementing/decrementing an integer.
> The reference count is required because I creating coroutine_self<> which
> gets the ref-counted ptr.

by tweaking the implementation the coroutine_self type and the coroutine
wouldn't need to share anything.

HTH,

-- 
gpd