From 09430395fd7dea427c75af131b08bba8c5136535 Mon Sep 17 00:00:00 2001 From: Hannes Roth Date: Fri, 13 Feb 2015 13:14:22 -0800 Subject: [PATCH] (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 --- folly/futures/Future-inl.h | 3 --- folly/futures/detail/Core.h | 38 ++++++++++++++++--------------- folly/futures/detail/FSM.h | 19 ++++++++-------- folly/futures/test/FSM.cpp | 15 ++++++------ folly/futures/test/FutureTest.cpp | 15 +++++++++++- 5 files changed, 52 insertions(+), 38 deletions(-) diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h index 77ff9fa9..a8454155 100644 --- a/folly/futures/Future-inl.h +++ b/folly/futures/Future-inl.h @@ -773,9 +773,6 @@ Future&& Future::waitVia(DrivableExecutor* e) && { namespace futures { namespace { - template - Future chainHelper(F, Callbacks...); - template Future chainHelper(Future f) { return f; diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h index c2bc1deb..d9826f8b 100644 --- a/folly/futures/detail/Core.h +++ b/folly/futures/detail/Core.h @@ -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* to Armed, if it is active (the usual case). */ -enum class State { +enum class State : uint8_t { Start, OnlyResult, OnlyCallback, @@ -73,12 +73,12 @@ enum class State { /// doesn't access a Future or Promise object from more than one thread at a /// time there won't be any problems. template -class Core : protected FSM { +class Core { 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() : FSM(State::Start) {} + Core() {} ~Core() { assert(detached_ == 2); } @@ -93,7 +93,7 @@ class Core : protected FSM { /// May call from any thread bool hasResult() const { - switch (getState()) { + switch (fsm_.getState()) { case State::OnlyResult: case State::Armed: case State::Done: @@ -128,13 +128,13 @@ class Core : protected FSM { callback_ = std::move(func); }; - FSM_START + FSM_START(fsm_) case State::Start: - FSM_UPDATE(State::OnlyCallback, setCallback_); + FSM_UPDATE(fsm_, State::OnlyCallback, setCallback_); break; case State::OnlyResult: - FSM_UPDATE(State::Armed, setCallback_); + FSM_UPDATE(fsm_, State::Armed, setCallback_); transitionToArmed = true; break; @@ -155,13 +155,13 @@ class Core : protected FSM { void setResult(Try&& t) { bool transitionToArmed = false; auto setResult_ = [&]{ result_ = std::move(t); }; - FSM_START + FSM_START(fsm_) case State::Start: - FSM_UPDATE(State::OnlyResult, setResult_); + FSM_UPDATE(fsm_, State::OnlyResult, setResult_); break; case State::OnlyCallback: - FSM_UPDATE(State::Armed, setResult_); + FSM_UPDATE(fsm_, State::Armed, setResult_); transitionToArmed = true; break; @@ -236,10 +236,11 @@ class Core : protected FSM { private: void maybeCallback() { - FSM_START + FSM_START(fsm_) case State::Armed: if (active_) { - FSM_UPDATE2(State::Done, []{}, std::bind(&Core::doCallback, this)); + FSM_UPDATE2(fsm_, State::Done, []{}, + std::bind(&Core::doCallback, this)); } FSM_BREAK @@ -273,15 +274,16 @@ class Core : protected FSM { } } - folly::Optional> result_; - std::function&&)> callback_; - std::shared_ptr context_{nullptr}; + FSM fsm_ {State::Start}; std::atomic detached_ {0}; std::atomic active_ {true}; - std::atomic executor_ {nullptr}; - exception_wrapper interrupt_; - std::function interruptHandler_; folly::MicroSpinLock interruptLock_ {0}; + folly::Optional> result_ {}; + std::function&&)> callback_ {nullptr}; + std::shared_ptr context_ {nullptr}; + std::atomic executor_ {nullptr}; + exception_wrapper interrupt_ {}; + std::function interruptHandler_ {nullptr}; }; template diff --git a/folly/futures/detail/FSM.h b/folly/futures/detail/FSM.h index 997b6c21..661fefa2 100644 --- a/folly/futures/detail/FSM.h +++ b/folly/futures/detail/FSM.h @@ -97,22 +97,23 @@ public: } }; -#define FSM_START \ - {bool done = false; while (!done) { auto state = getState(); switch (state) { +#define FSM_START(fsm) {\ + bool done = false; \ + while (!done) { auto state = fsm.getState(); switch (state) { -#define FSM_UPDATE2(b, protectedAction, unprotectedAction) \ - done = updateState(state, (b), (protectedAction), (unprotectedAction)); +#define FSM_UPDATE2(fsm, 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): \ - FSM_UPDATE((b), (action)); \ + FSM_UPDATE(fsm, (b), (action)); \ break; -#define FSM_CASE2(a, b, protectedAction, unprotectedAction) \ +#define FSM_CASE2(fsm, a, b, protectedAction, unprotectedAction) \ case (a): \ - FSM_UPDATE2((b), (protectedAction), (unprotectedAction)); \ + FSM_UPDATE2(fsm, (b), (protectedAction), (unprotectedAction)); \ break; #define FSM_BREAK done = true; break; diff --git a/folly/futures/test/FSM.cpp b/folly/futures/test/FSM.cpp index 4f4710f5..cf477591 100644 --- a/folly/futures/test/FSM.cpp +++ b/folly/futures/test/FSM.cpp @@ -51,14 +51,15 @@ TEST(FSM, example) { } TEST(FSM, magicMacrosExample) { - struct MyFSM : public FSM { + struct MyFSM { + FSM fsm_; int count = 0; int unprotectedCount = 0; - MyFSM() : FSM(State::A) {} + MyFSM() : fsm_(State::A) {} void twiddle() { - FSM_START - FSM_CASE(State::A, State::B, [&]{ count++; }); - FSM_CASE2(State::B, State::A, + FSM_START(fsm_) + FSM_CASE(fsm_, State::A, State::B, [&]{ count++; }); + FSM_CASE2(fsm_, State::B, State::A, [&]{ count--; }, [&]{ unprotectedCount--; }); FSM_END } @@ -67,12 +68,12 @@ TEST(FSM, magicMacrosExample) { MyFSM fsm; fsm.twiddle(); - EXPECT_EQ(State::B, fsm.getState()); + EXPECT_EQ(State::B, fsm.fsm_.getState()); EXPECT_EQ(1, fsm.count); EXPECT_EQ(0, fsm.unprotectedCount); fsm.twiddle(); - EXPECT_EQ(State::A, fsm.getState()); + EXPECT_EQ(State::A, fsm.fsm_.getState()); EXPECT_EQ(0, fsm.count); EXPECT_EQ(-1, fsm.unprotectedCount); } diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp index 8d421285..a46aea9f 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -83,6 +83,19 @@ class ThreadExecutor : public Executor { typedef FutureException eggs_t; 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)); +} + // Future TEST(Future, onError) { @@ -1075,7 +1088,7 @@ TEST(Future, waitWithDuration) { folly::Baton<> b; auto t = std::thread([&]{ b.post(); - std::this_thread::sleep_for(milliseconds(100)); + /* sleep override */ std::this_thread::sleep_for(milliseconds(100)); p.setValue(); }); b.wait(); -- 2.34.1