[boost] [coroutine review] late review + comments

Subject: [boost] [coroutine review] late review + comments
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-09-30 21:39:41

Apologies to Hartmut, Oliver, and the community for my late presence
regarding the recently reviewed Coroutine library. I finally had a good
block of time this afternoon/evening to look through the documentation [1],
so I figured, even if Hartmut doesn't count this as a formal review, at
least Oliver can use these comments to improve the documentation.

First, if I were to vote on whether Coroutine should be accepted, it would
be CONDITIONAL ACCEPTANCE just based on present documentation issues. At a
minimum, the documentation needs some fixing up, but I also think Oliver
should address the design and interface suggestions Vicente, Eugene, and
others (I myself haven't yet gone through the entire discussion yet, though
I've been meaning to) have had, which mostly seem to have merit, and then
present an updated. Unfortunately, I can't come up with a specific list of
said suggestions, but I trust Oliver can pick through the discussion as
well as I can :/

Second, I would really like to see some form of this library in Boost :)

One thing that benefits the documentation of a library with a seemingly
nontrivial design space (like Coroutine) is a comparison with past and
present solutions. A quick google search of "c++ coroutine" found 3
libraries that ostensibly implement coroutines or an approximation thereof
[2-4]. I haven't gone through these quite yet, but I'd like to see why one
should use the proposed Coroutine library over these alternatives (even a
1-line justification, like "Library X lacks critical feature Y, by design"
or "Library X does not work on platform Z" would be sufficient.) I think
some documented research to justify the existence of this library is
warranted.

My specific comments on the documentation follow (I just copy/pasted from a
Google document, so I hope this doesn't format badly...). I'm pretty sure
some of what's below repeats what someone else has said, but, for
completeness, I just included everything that initially came to mind as I
was reading the documentation.

   - Rename namespace boost::coro to (e.g.) boost::coroutine or something
   similarly spelled out and not (arguably cryptically) abbreviated.
   - "Executing a coroutine"
      - s/it can migrated/it can be migrated/
      - The first example uses “coro_t c( boost::bind( f, _1 ) );”; I’m
      confused as f is a binary function, but the boost::bind expression only
      works for unary functions.
      - “The coroutine-function, as well as its arguments, if any, are
      copied into the coroutine's state.” Can you elaborate on what you mean by
      “its arguments...are copied into the coroutine’s state”? You
must mean when
      you call the coroutine, i.e., the copying of the
coroutine-function and the
      copying of the coroutine-function’s arguments occur at different times,
      right?
      - The example with output uses ctx when (I believe) you mean c. Also,
      it implies that coroutine<>::is_complete actually executes the
      coroutine-function, which seems non-intuitive to me; I would expect it to
      be non-modifying and only indicate whether the coroutine-function has
      completed or not.
      - s/preemtive/preemptive
      - “Calling coroutine<>::operator()() from inside the same coroutine
      results in undefined behaviour.” Can you give an example of how one could
      possibly mistakenly do this?
      - s/allways/always
      - “The arguments passed to coroutine<>::operator()(), in one
      coroutine, is returned by coroutine<>::self_t::yield() in the other
      coroutine or, if it is the first call to coroutine<>::operator()() (
      coroutine was not started), the coroutine-function will be entered
      and the arguments are passed to coroutine-function on entry.” I think
      I know what you’re trying to say here, but talking about 2
coroutines when
      your examples only have one coroutine object is ripe for
confusion. Either
      clarify your terminology, clarify this explanation of the signature, or
      both.
      - The fact that self_t::yield() must package its return value into a
      boost::tuple (when the coroutine-function has more than one
argument beyond
      self) suggests to me that we should strive for more uniformity
between how
      the coroutine-function is initially entered (now with explicitly separate
      arguments) and subsequently continued (now with a packaged set of
      arguments). A couple of suggestions:
         - Force all coroutine-functions to be nullary or unary (sans the
         self argument); one can manually address more than one argument via
         boost::tuple packaging, and maybe boost::coro::coroutine could
         automatically package >1 argument up into a boost::tuple to
convenience.
         You may also want to provide a macro (named, e.g.,
BOOST_TUPLE_UNPACK) that
         can can unpack a tuple into variables, e.g., something that looks like

BOOST_TUPLE_UNPACK( ( int x ) ( int y ), tuple );

   - Use a coroutine-function signature that looks something like

    void f(coro_t::self_t & self)

    {

        BOOST_TUPLE_UNPACK( ( int x0 ) ( int y0 ), self.arguments() );

        // …

        BOOST_TUPLE_UNPACK( ( int x1 ) ( int y1 ), self.yield() );

    }

Perhaps equivalently, one can always call self.arguments() to get the most
recently passed set of arguments.

   - “coroutine-function with multiple arguments”

   - Isn’t the boost::bind in this example superfluous?
   - “Exit a coroutine-function”
      - yield_break or yield_return? You mention both.
   - There are 2 “Important” notes on forced_unwind exceptions that appear
   to say the same thing.
   - “Stack unwinding”
      - I’m starting to get really confused by your uses of boost::bind;
      “boost::bind( fn, _1 )” when fn is a nullary function??? I guess
you forgot
      the self_t argument? If so, again, why is boost::bind even necessary? Did
      you check that these examples actually compile?
      - The default behavior of the destructor of always unwinding the
      stack seems entirely reasonable, but the preconditions you list seem
      entirely unreasonable and unsafe! Basically, I’m reading that

    { coro::coroutine< void ( ) > c; }

results in undefined behavior, as the preconditions for the destructor are
not met. This almost has to be a documentation error. Further: “After
unwinding, a coroutine is complete.” I was under the impression that
unwinding only occurs in the destructor of the coroutine, at which point
the coroutine object does not exist anymore, so what coroutine are you
referring to, and even if you’re referring to the recently destructed
coroutine, in what way is it complete?

   - “Executing a generator”
      - s/The default StackAllocator concept/The default StackAllocator/
      - Be consistent on when you use StackAllocator, stack-allocator, etc.
      I.e., what do you mean by each?
      - “The first argument of generator-function...”: You mean the *only*
      argument?
      - s/The generator-function executed/The generator-function is
      executed/
      - Same notes as at the bottom of “Executing a coroutine”.
   - “Exit a generator-function”
      - Again, yield_break or yield_return?
      - “boost::bind( fn, _1, _2 )” ??? Huh? How is “a” bound within fn,
      then???
      - s/self_yield/self.yield/
   - It’s unclear to me why a separate generator class is provided in
   addition to the coroutine class. Can you explain what the differences are?
   E.g., I don’t see why coroutine can’t also have an operator
   unspecified-bool-type () rather than is_complete.
   - “Performance”
      - 10 doesn’t seem like a very convincing sample size. If you can’t
      nail down the CPU cycles and time exactly via theoretical analysis of the
      assembly code, then I don’t see what’s preventing you from using a sample
      size several orders of magnitude larger than 10!

Okay, now time to go through the second half of the recent Coroutine
discussion...

- Jeff

[1] http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/index.html
[2] http://code.mozy.com/projects/mordor/
[3] http://www.akira.ruc.dk/~keld/research/COROUTINE/
[4] http://dunkels.com/adam/pt/