patterncppMinor
Small coroutine class
Viewed 0 times
classsmallcoroutine
Problem
What do you think about this?
Usage:
EDIT
Change
to
Also check out an updated version of the code.
#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
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.
This is a red flag: you're calling
And I only kinda see what you're doing here, anyway. Are you assuming that the
The
Also, style nit: I've never seen anyone put a space after the word
Other style nits:
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.
I'm actually amazed that this works, given that the decltype of
Kudos for using prefix increment. Everything else about this for-loop is pretty unusual, though. I strongly recommend looping over the range
API design suggestions:
-
Empty angle brackets suck. I'd recommend something more like
-
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:
Well, unless
But now what is
And unfortunately now we've got move-constructible
...Speaking of which, your current implementation provides defaulted copy and move constructors! You should definitely delete those:
Getting back to the API-design example: the next step in a Javascript-like design would be allowing
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 noexceptauto is a strange way to spell bool. It's not saving you any bytes.// else do nothingThe 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 wellGetting 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 nothingauto top(reinterpret_cast<char*>(&top));Context
StackExchange Code Review Q#144112, answer score: 3
Revisions (0)
No revisions yet.