(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

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 77ff9fa9f9ab06ab0363a1851a011fdf02b3ec4d..a8454155f518c25a82173ed9a49bf595ef9b452e 100644 (file)
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -773,9 +773,6 @@ Future<T>&& Future<T>::waitVia(DrivableExecutor* e) && {
 namespace futures {
 
   namespace {
-    template <class Z, class F, class... Callbacks>
-    Future<Z> chainHelper(F, Callbacks...);
-
     template <class Z>
     Future<Z> chainHelper(Future<Z> f) {
       return f;

diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h
index c2bc1deba7d17bfe82086bb20a60ccfdb712ee4c..d9826f8b9c1f999b1278a7c28380b950aa12b717 100644 (file)
--- 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<typename T>
-class Core : protected FSM<State> {
+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>(State::Start) {}
+  Core() {}
   ~Core() {
     assert(detached_ == 2);
   }
@@ -93,7 +93,7 @@ class Core : protected FSM<State> {
 
   /// 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<State> {
       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<State> {
   void setResult(Try<T>&& 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<State> {
 
  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<State> {
     }
   }
 
-  folly::Optional<Try<T>> result_;
-  std::function<void(Try<T>&&)> callback_;
-  std::shared_ptr<RequestContext> context_{nullptr};
+  FSM<State> fsm_ {State::Start};
   std::atomic<unsigned char> detached_ {0};
   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::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>

diff --git a/folly/futures/detail/FSM.h b/folly/futures/detail/FSM.h
index 997b6c21f8555e8fd619cfd449a4f69e4f3d3ef3..661fefa2a265e998384671266aca715bf0f3c421 100644 (file)
--- 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 4f4710f58ad8d5bc5b62733ac95b85df29ac0b43..cf47759113777e3d08f0094ebe9c1af0b395ad45 100644 (file)
--- 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<State> {
+  struct MyFSM {
+    FSM<State> fsm_;
     int count = 0;
     int unprotectedCount = 0;
-    MyFSM() : FSM<State>(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 8d42128596f4f8cc87b36c62b47819b97501cf22..a46aea9f5022f8bd0ece794c481ea9b1988ccf56 100644 (file)
--- 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<void>));
+}
+
 // 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();