On Wed, Mar 18, 2015 at 4:08 AM, Jeremy Maitin-Shepard
Very nice work!
Thanks!
This is pretty much _the_ way to implement real composable futures, right?
well, at least I think it is.
A few comments:
1. Since shared_state_union<T> is basically expected<T>, it could be nice to unify them explicitly. The atomic option would also be useful for expected<T>, I would think.
thats a great insight. I need to read the expect papers again. Regarding the 'atomic option', I was actually thinking of removing it, instead switching to specifying the memory order in get()/set_{value,exception}.
2. I noticed a number of bugs in the implementation of future::then and future::next:
a. future::then throws if the state is ready and f throws, but instead should just return a ready future with the exception. More generally, calling a function with some arguments, and setting a promise/shared_state_union<T>/expected<T> either with the result or the exception is a useful operation to expose directly to users. This could then be used by both future::then as well has async, for instance.
b. future::then and future::next should probably consistently release ownership of their state even if the state is already ready. future::then also has to be fixed to call f with an rvalue future representing this, rather than an lvalue; currently, calling future::then with a standard "then" function declared as taking a future by value wouldn't compile. To ensure that the state is consistently released, you could use f(future(std::move(*this))), or perhaps better yet use a local variable to be robust to move elision potentially allowed by a future c++ standard. Additionally the return type needs to be the appropriate future type based on the return type of the function, rather than assuming it is the same as the current future type.
I complely removed the current then/next implementation in favor of a generic then that works with any waitable. This makes it explicit that you can compose future types (and in fact any waitable) as long as they provide a get_event. I removed 'next' completely to simplify the implementation. The optimization opportunity to save an allocation on the except case is probably not worth the complexity.
3. Since invoking future::then and future::next release ownership, it is worth considering making them only work on rvalues. Whether the extra compile-time error checking this buys is worth the added verbosity of having to add std::move in some cases is not clear, though.
the new free function then takes its waitable by value (so you have to move it). I'll leave the current signature of then as is for now for compatibility with the standard proposal and boost::future.
4. In some contexts a thread-unsafe future/event that saves the cost of atomic operations would be useful (unless it turns out that this cost is negligible on modern CPUs, which I don't think is the case). However, it would be important for the thread-safe and thread-unsafe futures/events to interoperate in a reasonable way. (I'm not sure exactly what reasonable semantics would be.) Obviously there would have to be some caveats, but we would want to avoid another future-island situation.
The required RMW are definitely non neglegible. I tried to keep them at a minimum but they still have a cost. My original future implementation actually had thread safety switch but it would be terribly error prone. Instead I'm studying a way for future to start as simply a deferred synchronous computation, but that can become asynchronous on request of another thread (via work stealing or work requesting for example).
I strongly encourage you to continue this work, and see about getting this incorporated into boost::future and possibly std::future, as it seems to be a cleaner and leaner abstraction than what is currently done.
Thanks!
Benchmarks (and unit tests) would also be particularly helpful.
I have yet to find a good, fair, small and realistic benchmark for futures. Ideas are welcome. -- gpd