From 1be516d1df75d5c33f9b064c288f1e8533f5c8c3 Mon Sep 17 00:00:00 2001 From: Hans Fugal Date: Tue, 21 Oct 2014 10:24:06 -0700 Subject: [PATCH] (wangle) express current Core functionality with a state machine MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Summary: This is a refactor of the current functionality to use a state machine (inheriting from `FSM`). This will make it easier to extend to do cancellation and Future collapsing. Performance is the same, maybe slightly faster (abt 1%). (I might be a little conservative with the atomics, it might be worth going through and reasoning about whether they all need to be atomic). The state machine is two states, Waiting (no value), and Done (has a value). Transitioning to Done will execute the callback if it exists and we are active. Otherwise the callback will happen when it is set and active is true. There is a subjective balancing act in place here, between making a state for every single mutable bit of information (which results in an explosion of states: hasValue X hasCallback X isActive X isCancelled …), and finding a sweet spot of expressivity. This isn't too far from the way Twitter did it, though we don't (yet) have all the states they have (and they don't have the concept of hot/cold futures). But I got there by way of replacing the `mutex_` with the state, after changing all those variables to atomics so they didn't need mutex protection (resulting in only `callback_` and `result_` needing it). I expect the state machine will morph as the rest of the functionality is added, but hopefully it will be easier to understand and keep correct (that's the idea anyway). Test Plan: Tests still pass (and not by accident, I made several awesome mistakes along the way). Reviewed By: davejwatson@fb.com Subscribers: net-systems@, fugalh, exa, njormrod FB internal diff: D1618240 Tasks: 4618297 --- folly/wangle/detail/Core.h | 103 +++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/folly/wangle/detail/Core.h b/folly/wangle/detail/Core.h index 1cfdcc13..ee60e65c 100644 --- a/folly/wangle/detail/Core.h +++ b/folly/wangle/detail/Core.h @@ -28,6 +28,7 @@ #include #include #include +#include namespace folly { namespace wangle { namespace detail { @@ -36,14 +37,19 @@ namespace folly { namespace wangle { namespace detail { template void empty_callback(Try&&) { } +enum class State { + Waiting, + Done, +}; + /** The shared state object for Future and Promise. */ template -class Core { +class Core : protected FSM { public: // This must be heap-constructed. There's probably a way to enforce that in // code but since this is just internal detail code and I don't know how // off-hand, I'm punting. - Core() = default; + Core() : FSM(State::Waiting) {} ~Core() { assert(calledBack_); assert(detached_ == 2); @@ -67,36 +73,48 @@ class Core { template void setCallback(F func) { - { - std::lock_guard lock(mutex_); - + auto setCallback_ = [&]{ if (callback_) { throw std::logic_error("setCallback called twice"); } callback_ = std::move(func); + }; + + bool done = false; + while (!done) { + switch (getState()) { + case State::Waiting: + done = updateState(State::Waiting, State::Waiting, setCallback_); + break; + + case State::Done: + done = updateState(State::Done, State::Done, + setCallback_, + [&]{ maybeCallback(); }); + break; + } } - - maybeCallback(); } void setResult(Try&& t) { - { - std::lock_guard lock(mutex_); - - if (ready()) { + bool done = false; + while (!done) { + switch (getState()) { + case State::Waiting: + done = updateState(State::Waiting, State::Done, + [&]{ result_ = std::move(t); }, + [&]{ maybeCallback(); }); + break; + + case State::Done: throw std::logic_error("setResult called twice"); } - - result_ = std::move(t); - assert(ready()); } - - maybeCallback(); } bool ready() const { - return result_.hasValue(); + return getState() == State::Done; } // Called by a destructing Future @@ -117,71 +135,54 @@ class Core { } void deactivate() { - std::lock_guard lock(mutex_); active_ = false; } void activate() { - { - std::lock_guard lock(mutex_); - active_ = true; + active_ = true; + if (ready()) { + maybeCallback(); } - maybeCallback(); } bool isActive() { return active_; } void setExecutor(Executor* x) { - std::lock_guard lock(mutex_); executor_ = x; } private: void maybeCallback() { - std::unique_lock lock(mutex_); - if (!calledBack_ && - result_ && callback_ && isActive()) { - // TODO(5306911) we should probably try/catch here - if (executor_) { - MoveWrapper>> val(std::move(result_)); + assert(ready()); + if (!calledBack_ && isActive() && callback_) { + // TODO(5306911) we should probably try/catch + calledBack_ = true; + Executor* x = executor_; + if (x) { MoveWrapper&&)>> cb(std::move(callback_)); - executor_->add([cb, val]() mutable { (*cb)(std::move(**val)); }); - calledBack_ = true; + MoveWrapper>> val(std::move(result_)); + x->add([cb, val]() mutable { (*cb)(std::move(**val)); }); } else { - calledBack_ = true; - lock.unlock(); callback_(std::move(*result_)); } } } void detachOne() { - bool shouldDelete; - { - std::lock_guard lock(mutex_); - detached_++; - assert(detached_ == 1 || detached_ == 2); - shouldDelete = (detached_ == 2); - } - - if (shouldDelete) { + if (++detached_ == 2) { // we should have already executed the callback with the value assert(calledBack_); delete this; } + assert(detached_ == 1 || detached_ == 2); } folly::Optional> result_; std::function&&)> callback_; - bool calledBack_ = false; - unsigned char detached_ = 0; - bool active_ = true; - Executor* executor_ = nullptr; - - // this lock isn't meant to protect all accesses to members, only the ones - // that need to be threadsafe: the act of setting result_ and callback_, and - // seeing if they are set and whether we should then continue. - folly::MicroSpinLock mutex_ {0}; + std::atomic calledBack_ {false}; + std::atomic detached_ {0}; + std::atomic active_ {true}; + std::atomic executor_ {nullptr}; }; template -- 2.34.1