HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Small coroutine class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
classsmallcoroutine

Problem

What do you think about this?

#include 

#include 

#include 

#include 

template 
class coroutine
{
  jmp_buf env_in_;
  jmp_buf env_out_;

  bool running_{};

  char stack_[N];

public:
  coroutine() = default;

  auto running() const noexcept
  {
    return running_;
  }

  template 
  void run(F&& f, A&& ...a)
  {
    if (setjmp(env_in_))
    {
      return;
    }
    // else do nothing

    auto top(reinterpret_cast(&top));
    alloca(top - (stack_ + N));

    running_ = true;

    [this, f = ::std::forward(f)](A&& ...a) __attribute__ ((noinline))
    {
      f(::std::ref(*this), ::std::forward(a)...);

      running_ = false;

      yield();
    }(::std::forward(a)...);
  }

  void yield() noexcept
  {
    if (setjmp(env_out_))
    {
      return;
    }
    else
    {
      longjmp(env_in_, 1);
    }
  }

  void resume()
  {
    assert(running_);
    if (setjmp(env_in_))
    {
      return;
    }
    else
    {
      longjmp(env_out_, 1);
    }
  }
};


Usage:

#include 

#include "coroutine.hpp"

int main()
{
  coroutine<> c;

  c.run([](decltype(c)& c)
    {
      for (int i{}; i != 3; ++i)
      {
        ::std::cout << i << ::std::endl;

        c.yield();
      }
    }
  );

  while (c.running())
  {
    c.resume();
  }

  return 0;
}


EDIT

Change

auto top(reinterpret_cast(&top));


to

char* top;
top = reinterpret_cast(&top);


Also check out an updated version of the code.

Solution

I think you need to document your assumptions and/or intended use cases a little better. Right now, I don't see how anything based on setjmp/longjmp could actually implement a coroutine, since longjmp can only ever jump upward in the call-stack, never downward. (Really, it can't jump "downward" because there is no "downward": the only things below us on the call-stack are things that have already finished executing, and things which will execute in the future.)

Notice that "things which will execute in the future" includes "interrupt handlers". You might want to check whether your implementation plays well with Unix signal handlers — I suspect it does not.

alloca(top - (stack_ + N));


This is a red flag: you're calling alloca without assigning the result anywhere. A sufficiently smart compiler will just optimize away this useless call. (Yeah, I kinda get what you're trying to do... but the compiler is under no obligation to get it.) What you really need to do here is heap-allocate a buffer for your stack, and then somehow get the saved stack pointer in the jmp_buf to point to it.

And I only kinda see what you're doing here, anyway. Are you assuming that the coroutine object itself must be allocated on the stack? What happens if I make a global or static coroutine object?

[this, f = ::std::forward(f)](A&& ...a) __attribute__ ((noinline))


The __attribute__((noinline)) here is another red flag. This time I don't get what you're trying to do. What would break if this lambda's operator() got inlined?

Also, style nit: I've never seen anyone put a space after the word __attribute__ before.

Other style nits:

auto running() const noexcept


auto is a strange way to spell bool. It's not saving you any bytes.

// else do nothing


The above comment is highly misleading, in that it's followed by 13 lines of really dense code — hardly "do nothing"! I'd delete that comment.

auto top(reinterpret_cast(&top));


I'm actually amazed that this works, given that the decltype of &top is not known at the time the RHS is parsed. ...Oh wait, yeah, it doesn't work. Both GCC and Clang reject this code.

for (int i{}; i != 3; ++i)


Kudos for using prefix increment. Everything else about this for-loop is pretty unusual, though. I strongly recommend looping over the range [0,3) like this instead:

for (int i = 0; i < 3; ++i)


API design suggestions:

-
Empty angle brackets suck. I'd recommend something more like

template  class coroutine_with_size { ... }

using coroutine = coroutine_with_size;


-
It's unfortunate that your coroutine has to do all its own I/O; a better design would be like the one found in Javascript (and Python), where coroutines can yield values. Unfortunately, the ability to yield values causes a cascade of API changes. Look:

c.run([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (c.running())
{
    int ii = c.resume();
    std::cout << ii << std::endl;
}


Well, unless c is somehow "looking ahead" to determine the value it's going to return next time, c.running() doesn't really make sense anymore. We've got to have c.resume() do all the hard work, at which point we need a way to say "okay, no more data, please stop iterating". That is, we need a way for c.resume() to terminate execution but not return a value. This smells like an exception. So:

c.run([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}


But now what is c.run() actually doing? It can't be actually starting to run the provided lambda, because there's nowhere for us to stash the yielded result yet (we haven't gotten to ii's declaration). So all it's really doing now is constructing the coroutine object and then returning immediately. That smells like a constructor. So:

auto c = coroutine([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}


And unfortunately now we've got move-constructible coroutine objects, and your entire implementation strategy falls apart even more than it had been. Crap!

...Speaking of which, your current implementation provides defaulted copy and move constructors! You should definitely delete those:

coroutine(const coroutine&) = delete;
coroutine& operator=(const coroutine&) = delete;
// move operations get deleted implicitly due to the above;
// but I wouldn't object to deleting them explicitly as well


Getting back to the API-design example: the next step in a Javascript-like design would be allowing c.resume() to take input values, as well as yielding output values. Mind you, I'm

Code Snippets

alloca(top - (stack_ + N));
[this, f = ::std::forward<F>(f)](A&& ...a) __attribute__ ((noinline))
auto running() const noexcept
// else do nothing
auto top(reinterpret_cast<char*>(&top));

Context

StackExchange Code Review Q#144112, answer score: 3

Revisions (0)

No revisions yet.