Commit 09430395 authored by Hannes Roth's avatar Hannes Roth Committed by Alecs King

(Wangle) Have Core own an FSM instead of inheriting

Summary:
This seems cleaner. I also moved a Core member around and changed state to be a smaller size to save us 16 bytes. Might be neat to try to get this down to 128?

Fixed compilation with GCC 4.9, too.

Test Plan: Ran all the tests. Will also benchmark.

Reviewed By: hans@fb.com

Subscribers: trunkagent, folly-diffs@, jsedgwick, yfeldblum

FB internal diff: D1843129

Signature: t1:1843129:1423789502:60e56d1af553994991195fcc350d37d644a556ca
parent 96242018
...@@ -773,9 +773,6 @@ Future<T>&& Future<T>::waitVia(DrivableExecutor* e) && { ...@@ -773,9 +773,6 @@ Future<T>&& Future<T>::waitVia(DrivableExecutor* e) && {
namespace futures { namespace futures {
namespace { namespace {
template <class Z, class F, class... Callbacks>
Future<Z> chainHelper(F, Callbacks...);
template <class Z> template <class Z>
Future<Z> chainHelper(Future<Z> f) { Future<Z> chainHelper(Future<Z> f) {
return f; return f;
......
...@@ -46,7 +46,7 @@ that the callback is only executed on the transition from Armed to Done, ...@@ -46,7 +46,7 @@ that the callback is only executed on the transition from Armed to Done,
and that transition can happen immediately after transitioning from Only* and that transition can happen immediately after transitioning from Only*
to Armed, if it is active (the usual case). to Armed, if it is active (the usual case).
*/ */
enum class State { enum class State : uint8_t {
Start, Start,
OnlyResult, OnlyResult,
OnlyCallback, OnlyCallback,
...@@ -73,12 +73,12 @@ enum class State { ...@@ -73,12 +73,12 @@ enum class State {
/// doesn't access a Future or Promise object from more than one thread at a /// doesn't access a Future or Promise object from more than one thread at a
/// time there won't be any problems. /// time there won't be any problems.
template<typename T> template<typename T>
class Core : protected FSM<State> { class Core {
public: public:
/// This must be heap-constructed. There's probably a way to enforce that in /// 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 /// code but since this is just internal detail code and I don't know how
/// off-hand, I'm punting. /// off-hand, I'm punting.
Core() : FSM<State>(State::Start) {} Core() {}
~Core() { ~Core() {
assert(detached_ == 2); assert(detached_ == 2);
} }
...@@ -93,7 +93,7 @@ class Core : protected FSM<State> { ...@@ -93,7 +93,7 @@ class Core : protected FSM<State> {
/// May call from any thread /// May call from any thread
bool hasResult() const { bool hasResult() const {
switch (getState()) { switch (fsm_.getState()) {
case State::OnlyResult: case State::OnlyResult:
case State::Armed: case State::Armed:
case State::Done: case State::Done:
...@@ -128,13 +128,13 @@ class Core : protected FSM<State> { ...@@ -128,13 +128,13 @@ class Core : protected FSM<State> {
callback_ = std::move(func); callback_ = std::move(func);
}; };
FSM_START FSM_START(fsm_)
case State::Start: case State::Start:
FSM_UPDATE(State::OnlyCallback, setCallback_); FSM_UPDATE(fsm_, State::OnlyCallback, setCallback_);
break; break;
case State::OnlyResult: case State::OnlyResult:
FSM_UPDATE(State::Armed, setCallback_); FSM_UPDATE(fsm_, State::Armed, setCallback_);
transitionToArmed = true; transitionToArmed = true;
break; break;
...@@ -155,13 +155,13 @@ class Core : protected FSM<State> { ...@@ -155,13 +155,13 @@ class Core : protected FSM<State> {
void setResult(Try<T>&& t) { void setResult(Try<T>&& t) {
bool transitionToArmed = false; bool transitionToArmed = false;
auto setResult_ = [&]{ result_ = std::move(t); }; auto setResult_ = [&]{ result_ = std::move(t); };
FSM_START FSM_START(fsm_)
case State::Start: case State::Start:
FSM_UPDATE(State::OnlyResult, setResult_); FSM_UPDATE(fsm_, State::OnlyResult, setResult_);
break; break;
case State::OnlyCallback: case State::OnlyCallback:
FSM_UPDATE(State::Armed, setResult_); FSM_UPDATE(fsm_, State::Armed, setResult_);
transitionToArmed = true; transitionToArmed = true;
break; break;
...@@ -236,10 +236,11 @@ class Core : protected FSM<State> { ...@@ -236,10 +236,11 @@ class Core : protected FSM<State> {
private: private:
void maybeCallback() { void maybeCallback() {
FSM_START FSM_START(fsm_)
case State::Armed: case State::Armed:
if (active_) { if (active_) {
FSM_UPDATE2(State::Done, []{}, std::bind(&Core::doCallback, this)); FSM_UPDATE2(fsm_, State::Done, []{},
std::bind(&Core::doCallback, this));
} }
FSM_BREAK FSM_BREAK
...@@ -273,15 +274,16 @@ class Core : protected FSM<State> { ...@@ -273,15 +274,16 @@ class Core : protected FSM<State> {
} }
} }
folly::Optional<Try<T>> result_; FSM<State> fsm_ {State::Start};
std::function<void(Try<T>&&)> callback_;
std::shared_ptr<RequestContext> context_{nullptr};
std::atomic<unsigned char> detached_ {0}; std::atomic<unsigned char> detached_ {0};
std::atomic<bool> active_ {true}; std::atomic<bool> active_ {true};
std::atomic<Executor*> executor_ {nullptr};
exception_wrapper interrupt_;
std::function<void(exception_wrapper const&)> interruptHandler_;
folly::MicroSpinLock interruptLock_ {0}; folly::MicroSpinLock interruptLock_ {0};
folly::Optional<Try<T>> result_ {};
std::function<void(Try<T>&&)> callback_ {nullptr};
std::shared_ptr<RequestContext> context_ {nullptr};
std::atomic<Executor*> executor_ {nullptr};
exception_wrapper interrupt_ {};
std::function<void(exception_wrapper const&)> interruptHandler_ {nullptr};
}; };
template <typename... Ts> template <typename... Ts>
......
...@@ -97,22 +97,23 @@ public: ...@@ -97,22 +97,23 @@ public:
} }
}; };
#define FSM_START \ #define FSM_START(fsm) {\
{bool done = false; while (!done) { auto state = getState(); switch (state) { bool done = false; \
while (!done) { auto state = fsm.getState(); switch (state) {
#define FSM_UPDATE2(b, protectedAction, unprotectedAction) \ #define FSM_UPDATE2(fsm, b, protectedAction, unprotectedAction) \
done = updateState(state, (b), (protectedAction), (unprotectedAction)); done = fsm.updateState(state, (b), (protectedAction), (unprotectedAction));
#define FSM_UPDATE(b, action) FSM_UPDATE2((b), (action), []{}) #define FSM_UPDATE(fsm, b, action) FSM_UPDATE2(fsm, (b), (action), []{})
#define FSM_CASE(a, b, action) \ #define FSM_CASE(fsm, a, b, action) \
case (a): \ case (a): \
FSM_UPDATE((b), (action)); \ FSM_UPDATE(fsm, (b), (action)); \
break; break;
#define FSM_CASE2(a, b, protectedAction, unprotectedAction) \ #define FSM_CASE2(fsm, a, b, protectedAction, unprotectedAction) \
case (a): \ case (a): \
FSM_UPDATE2((b), (protectedAction), (unprotectedAction)); \ FSM_UPDATE2(fsm, (b), (protectedAction), (unprotectedAction)); \
break; break;
#define FSM_BREAK done = true; break; #define FSM_BREAK done = true; break;
......
...@@ -51,14 +51,15 @@ TEST(FSM, example) { ...@@ -51,14 +51,15 @@ TEST(FSM, example) {
} }
TEST(FSM, magicMacrosExample) { TEST(FSM, magicMacrosExample) {
struct MyFSM : public FSM<State> { struct MyFSM {
FSM<State> fsm_;
int count = 0; int count = 0;
int unprotectedCount = 0; int unprotectedCount = 0;
MyFSM() : FSM<State>(State::A) {} MyFSM() : fsm_(State::A) {}
void twiddle() { void twiddle() {
FSM_START FSM_START(fsm_)
FSM_CASE(State::A, State::B, [&]{ count++; }); FSM_CASE(fsm_, State::A, State::B, [&]{ count++; });
FSM_CASE2(State::B, State::A, FSM_CASE2(fsm_, State::B, State::A,
[&]{ count--; }, [&]{ unprotectedCount--; }); [&]{ count--; }, [&]{ unprotectedCount--; });
FSM_END FSM_END
} }
...@@ -67,12 +68,12 @@ TEST(FSM, magicMacrosExample) { ...@@ -67,12 +68,12 @@ TEST(FSM, magicMacrosExample) {
MyFSM fsm; MyFSM fsm;
fsm.twiddle(); fsm.twiddle();
EXPECT_EQ(State::B, fsm.getState()); EXPECT_EQ(State::B, fsm.fsm_.getState());
EXPECT_EQ(1, fsm.count); EXPECT_EQ(1, fsm.count);
EXPECT_EQ(0, fsm.unprotectedCount); EXPECT_EQ(0, fsm.unprotectedCount);
fsm.twiddle(); fsm.twiddle();
EXPECT_EQ(State::A, fsm.getState()); EXPECT_EQ(State::A, fsm.fsm_.getState());
EXPECT_EQ(0, fsm.count); EXPECT_EQ(0, fsm.count);
EXPECT_EQ(-1, fsm.unprotectedCount); EXPECT_EQ(-1, fsm.unprotectedCount);
} }
......
...@@ -83,6 +83,19 @@ class ThreadExecutor : public Executor { ...@@ -83,6 +83,19 @@ class ThreadExecutor : public Executor {
typedef FutureException eggs_t; typedef FutureException eggs_t;
static eggs_t eggs("eggs"); static eggs_t eggs("eggs");
// Core
TEST(Future, coreSize) {
// If this number goes down, it's fine!
// If it goes up, please seek professional advice ;-)
size_t size = 168;
if (sizeof(folly::exception_wrapper) == 80) {
// it contains strings, which can be bigger for -fb builds (e.g. fbstring)
size = 200;
}
EXPECT_EQ(size, sizeof(detail::Core<void>));
}
// Future // Future
TEST(Future, onError) { TEST(Future, onError) {
...@@ -1075,7 +1088,7 @@ TEST(Future, waitWithDuration) { ...@@ -1075,7 +1088,7 @@ TEST(Future, waitWithDuration) {
folly::Baton<> b; folly::Baton<> b;
auto t = std::thread([&]{ auto t = std::thread([&]{
b.post(); b.post();
std::this_thread::sleep_for(milliseconds(100)); /* sleep override */ std::this_thread::sleep_for(milliseconds(100));
p.setValue(); p.setValue();
}); });
b.wait(); b.wait();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment