Bug 1322095 - Part 1a: Move WrapRunnable's args when running. r=jya draft
authorEric Rahm <erahm@mozilla.com>
Thu, 16 Jan 2020 18:27:40 +0000
changeset 2598860 24127df167d1d1b907e86ea2c3c1d6491db03ebc
parent 2598325 7e0886a94d70b8696d6fc0481d9f9ae12b85c41a
child 2598861 1308de61f622b6c9a46baebd9ace9eb2bc7a2f06
push id476908
push userreviewbot
push dateThu, 16 Jan 2020 18:28:04 +0000
treeherdertry@ad83d4ce63b7 [default view] [failures only]
reviewersjya
bugs1322095
milestone74.0a1
Bug 1322095 - Part 1a: Move WrapRunnable's args when running. r=jya In order to support `UniquePtr` as an arg type for `WrapRunnable` as well as avoid unnecessary copies in the future we need to switch to moving args when invoking a runnable. This updates `WrapRunnables` so that they move their args when running and adds/updates some tests. To accomplish this `std::apply` is swapped in for our bespoke implementation and `std::tuple` is used to hold the args. We then `std::move` the args when `Run` is called. We also needed to support an r-value `Class` param for the runnable method on a bound object versions could work with `UniquePtr` as the holder class. Differential Revision: https://phabricator.services.mozilla.com/D59960 Differential Diff: PHID-DIFF-5uh77gp2xhs662uka5iz
media/mtransport/runnable_utils.h
media/mtransport/test/runnable_utils_unittest.cpp
--- a/media/mtransport/runnable_utils.h
+++ b/media/mtransport/runnable_utils.h
@@ -7,18 +7,19 @@
 // Original author: ekr@rtfm.com
 
 #ifndef runnable_utils_h__
 #define runnable_utils_h__
 
 #include "nsThreadUtils.h"
 #include "mozilla/Move.h"
 #include "mozilla/RefPtr.h"
-#include "mozilla/Tuple.h"
 
+#include <functional>
+#include <tuple>
 #include <utility>
 
 // Abstract base class for all of our templates
 namespace mozilla {
 
 namespace detail {
 
 enum RunnableResult { NoResult, ReturnsResult };
@@ -32,77 +33,34 @@ static inline nsresult RunOnThreadIntern
 template <RunnableResult result>
 class runnable_args_base : public Runnable {
  public:
   runnable_args_base() : Runnable("media-runnable_args_base") {}
 
   NS_IMETHOD Run() override = 0;
 };
 
-template <typename R>
-struct RunnableFunctionCallHelper {
-  template <typename FunType, typename... Args, size_t... Indices>
-  static R apply(FunType func, Tuple<Args...>& args,
-                 std::index_sequence<Indices...>) {
-    return func(Get<Indices>(args)...);
-  }
-};
-
-// A void specialization is needed in the case where the template instantiator
-// knows we don't want to return a value, but we don't know whether the called
-// function returns void or something else.
-template <>
-struct RunnableFunctionCallHelper<void> {
-  template <typename FunType, typename... Args, size_t... Indices>
-  static void apply(FunType func, Tuple<Args...>& args,
-                    std::index_sequence<Indices...>) {
-    func(Get<Indices>(args)...);
-  }
-};
-
-template <typename R>
-struct RunnableMethodCallHelper {
-  template <typename Class, typename M, typename... Args, size_t... Indices>
-  static R apply(Class obj, M method, Tuple<Args...>& args,
-                 std::index_sequence<Indices...>) {
-    return ((*obj).*method)(Get<Indices>(args)...);
-  }
-};
-
-// A void specialization is needed in the case where the template instantiator
-// knows we don't want to return a value, but we don't know whether the called
-// method returns void or something else.
-template <>
-struct RunnableMethodCallHelper<void> {
-  template <typename Class, typename M, typename... Args, size_t... Indices>
-  static void apply(Class obj, M method, Tuple<Args...>& args,
-                    std::index_sequence<Indices...>) {
-    ((*obj).*method)(Get<Indices>(args)...);
-  }
-};
-
 }  // namespace detail
 
 template <typename FunType, typename... Args>
 class runnable_args_func : public detail::runnable_args_base<detail::NoResult> {
  public:
   // |explicit| to pacify static analysis when there are no |args|.
   template <typename... Arguments>
   explicit runnable_args_func(FunType f, Arguments&&... args)
       : mFunc(f), mArgs(std::forward<Arguments>(args)...) {}
 
   NS_IMETHOD Run() override {
-    detail::RunnableFunctionCallHelper<void>::apply(
-        mFunc, mArgs, std::index_sequence_for<Args...>{});
+    std::apply(mFunc, std::move(mArgs));
     return NS_OK;
   }
 
  private:
   FunType mFunc;
-  Tuple<Args...> mArgs;
+  std::tuple<Args...> mArgs;
 };
 
 template <typename FunType, typename... Args>
 runnable_args_func<FunType, typename mozilla::Decay<Args>::Type...>*
 WrapRunnableNM(FunType f, Args&&... args) {
   return new runnable_args_func<FunType,
                                 typename mozilla::Decay<Args>::Type...>(
       f, std::forward<Args>(args)...);
@@ -112,93 +70,104 @@ template <typename Ret, typename FunType
 class runnable_args_func_ret
     : public detail::runnable_args_base<detail::ReturnsResult> {
  public:
   template <typename... Arguments>
   runnable_args_func_ret(Ret* ret, FunType f, Arguments&&... args)
       : mReturn(ret), mFunc(f), mArgs(std::forward<Arguments>(args)...) {}
 
   NS_IMETHOD Run() override {
-    *mReturn = detail::RunnableFunctionCallHelper<Ret>::apply(
-        mFunc, mArgs, std::index_sequence_for<Args...>{});
+    *mReturn = std::apply(mFunc, std::move(mArgs));
     return NS_OK;
   }
 
  private:
   Ret* mReturn;
   FunType mFunc;
-  Tuple<Args...> mArgs;
+  std::tuple<Args...> mArgs;
 };
 
 template <typename R, typename FunType, typename... Args>
 runnable_args_func_ret<R, FunType, typename mozilla::Decay<Args>::Type...>*
 WrapRunnableNMRet(R* ret, FunType f, Args&&... args) {
   return new runnable_args_func_ret<R, FunType,
                                     typename mozilla::Decay<Args>::Type...>(
       ret, f, std::forward<Args>(args)...);
 }
 
 template <typename Class, typename M, typename... Args>
 class runnable_args_memfn
     : public detail::runnable_args_base<detail::NoResult> {
  public:
   template <typename... Arguments>
-  runnable_args_memfn(Class obj, M method, Arguments&&... args)
-      : mObj(obj), mMethod(method), mArgs(std::forward<Arguments>(args)...) {}
+  runnable_args_memfn(Class&& obj, M method, Arguments&&... args)
+      : mObj(std::forward<Class>(obj)),
+        mMethod(method),
+        mArgs(std::forward<Arguments>(args)...) {}
 
   NS_IMETHOD Run() override {
-    detail::RunnableMethodCallHelper<void>::apply(
-        mObj, mMethod, mArgs, std::index_sequence_for<Args...>{});
+    std::apply(
+        [&](Args&&... args) {
+          ((*mObj).*mMethod)(std::forward<Args>(args)...);
+        },
+        std::move(mArgs));
     return NS_OK;
   }
 
  private:
-  Class mObj;
+  // For holders such as RefPtr and UniquePtr make sure concrete copy is held
+  // rather than a potential dangling reference.
+  typename mozilla::Decay<Class>::Type mObj;
   M mMethod;
-  Tuple<Args...> mArgs;
+  std::tuple<Args...> mArgs;
 };
 
 template <typename Class, typename M, typename... Args>
 runnable_args_memfn<Class, M, typename mozilla::Decay<Args>::Type...>*
-WrapRunnable(Class obj, M method, Args&&... args) {
+WrapRunnable(Class&& obj, M method, Args&&... args) {
   return new runnable_args_memfn<Class, M,
                                  typename mozilla::Decay<Args>::Type...>(
-      obj, method, std::forward<Args>(args)...);
+      std::forward<Class>(obj), method, std::forward<Args>(args)...);
 }
 
 template <typename Ret, typename Class, typename M, typename... Args>
 class runnable_args_memfn_ret
     : public detail::runnable_args_base<detail::ReturnsResult> {
  public:
   template <typename... Arguments>
-  runnable_args_memfn_ret(Ret* ret, Class obj, M method, Arguments... args)
+  runnable_args_memfn_ret(Ret* ret, Class&& obj, M method, Arguments... args)
       : mReturn(ret),
-        mObj(obj),
+        mObj(std::forward<Class>(obj)),
         mMethod(method),
         mArgs(std::forward<Arguments>(args)...) {}
 
   NS_IMETHOD Run() override {
-    *mReturn = detail::RunnableMethodCallHelper<Ret>::apply(
-        mObj, mMethod, mArgs, std::index_sequence_for<Args...>{});
+    std::apply(
+        [&](Args&&... args) {
+          *mReturn = ((*mObj).*mMethod)(std::forward<Args>(args)...);
+        },
+        std::move(mArgs));
     return NS_OK;
   }
 
  private:
   Ret* mReturn;
-  Class mObj;
+  // For holders such as RefPtr and UniquePtr make sure concrete copy is held
+  // rather than a potential dangling reference.
+  typename mozilla::Decay<Class>::Type mObj;
   M mMethod;
-  Tuple<Args...> mArgs;
+  std::tuple<Args...> mArgs;
 };
 
 template <typename R, typename Class, typename M, typename... Args>
 runnable_args_memfn_ret<R, Class, M, typename mozilla::Decay<Args>::Type...>*
-WrapRunnableRet(R* ret, Class obj, M method, Args&&... args) {
+WrapRunnableRet(R* ret, Class&& obj, M method, Args&&... args) {
   return new runnable_args_memfn_ret<R, Class, M,
                                      typename mozilla::Decay<Args>::Type...>(
-      ret, obj, method, std::forward<Args>(args)...);
+      ret, std::forward<Class>(obj), method, std::forward<Args>(args)...);
 }
 
 static inline nsresult RUN_ON_THREAD(
     nsIEventTarget* thread,
     detail::runnable_args_base<detail::NoResult>* runnable, uint32_t flags) {
   return detail::RunOnThreadInternal(
       thread, static_cast<nsIRunnable*>(runnable), flags);
 }
--- a/media/mtransport/test/runnable_utils_unittest.cpp
+++ b/media/mtransport/test/runnable_utils_unittest.cpp
@@ -9,31 +9,88 @@
 
 #include "prio.h"
 
 #include "nsCOMPtr.h"
 #include "nsNetCID.h"
 #include "nsXPCOM.h"
 
 #include "mozilla/RefPtr.h"
+#include "mozilla/UniquePtr.h"
 
 #include "nsASocketHandler.h"
 #include "nsServiceManagerUtils.h"
 #include "nsThreadUtils.h"
 
 #include "runnable_utils.h"
 
 #define GTEST_HAS_RTTI 0
 #include "gtest/gtest.h"
 #include "gtest_utils.h"
 
 using namespace mozilla;
 
 namespace {
 
+// Helper used ot make sure args are properly copied and/or moved.
+struct CtorDtorState {
+  enum class State { Empty, Copy, Explicit, Move, Moved };
+
+  static const char* ToStr(const State& state) {
+    switch (state) {
+      case State::Empty:
+        return "empty";
+      case State::Copy:
+        return "copy";
+      case State::Explicit:
+        return "explicit";
+      case State::Move:
+        return "move";
+      case State::Moved:
+        return "moved";
+      default:
+        return "unknown";
+    }
+  }
+
+  void DumpState() const { std::cerr << ToStr(state_) << std::endl; }
+
+  CtorDtorState() { DumpState(); }
+
+  explicit CtorDtorState(int* destroyed)
+      : dtor_count_(destroyed), state_(State::Explicit) {
+    DumpState();
+  }
+
+  CtorDtorState(const CtorDtorState& other)
+      : dtor_count_(other.dtor_count_), state_(State::Copy) {
+    DumpState();
+  }
+
+  // Clear the other's dtor counter so it's not counted if moved.
+  CtorDtorState(CtorDtorState&& other)
+      : dtor_count_(std::exchange(other.dtor_count_, nullptr)),
+        state_(State::Move) {
+    other.state_ = State::Moved;
+    DumpState();
+  }
+
+  ~CtorDtorState() {
+    const char* const state = ToStr(state_);
+    std::cerr << "Destructor called with end state: " << state << std::endl;
+
+    if (dtor_count_) {
+      *dtor_count_ += 1;
+    }
+  }
+
+  int* dtor_count_ = nullptr;
+  State state_ = State::Empty;
+};
+
 class Destructor {
  private:
   ~Destructor() {
     std::cerr << "Destructor called" << std::endl;
     *destroyed_ = true;
   }
 
  public:
@@ -62,17 +119,16 @@ class TargetClass {
   void m1set(bool* z) {
     std::cerr << __FUNCTION__ << std::endl;
     *z = true;
   }
   int return_int(int x) {
     std::cerr << __FUNCTION__ << std::endl;
     return x;
   }
-  void destructor_target(Destructor*) {}
 
   void destructor_target_ref(RefPtr<Destructor> destructor) {}
 
   int* ran_;
 };
 
 class RunnableArgsTest : public MtransportTest {
  public:
@@ -173,31 +229,119 @@ TEST_F(DispatchTest, TestNonMethodRet) {
 
   test_utils_->sts_target()->Dispatch(
       WrapRunnableNMRet(&z, SetNonMethodRet, &cl_, 10), NS_DISPATCH_SYNC);
 
   ASSERT_EQ(1, ran_);
   ASSERT_EQ(10, z);
 }
 
-TEST_F(DispatchTest, TestDestructor) {
+TEST_F(DispatchTest, TestDestructorRef) {
   bool destroyed = false;
-  RefPtr<Destructor> destructor = new Destructor(&destroyed);
-  target_->Dispatch(
-      WrapRunnable(&cl_, &TargetClass::destructor_target, destructor),
-      NS_DISPATCH_SYNC);
-  ASSERT_FALSE(destroyed);
-  destructor = nullptr;
+  {
+    RefPtr<Destructor> destructor = new Destructor(&destroyed);
+    target_->Dispatch(
+        WrapRunnable(&cl_, &TargetClass::destructor_target_ref, destructor),
+        NS_DISPATCH_SYNC);
+    ASSERT_FALSE(destroyed);
+  }
   ASSERT_TRUE(destroyed);
+
+  // Now try with a move.
+  destroyed = false;
+  {
+    RefPtr<Destructor> destructor = new Destructor(&destroyed);
+    target_->Dispatch(WrapRunnable(&cl_, &TargetClass::destructor_target_ref,
+                                   std::move(destructor)),
+                      NS_DISPATCH_SYNC);
+    ASSERT_TRUE(destroyed);
+  }
 }
 
-TEST_F(DispatchTest, TestDestructorRef) {
-  bool destroyed = false;
-  RefPtr<Destructor> destructor = new Destructor(&destroyed);
-  target_->Dispatch(
-      WrapRunnable(&cl_, &TargetClass::destructor_target_ref, destructor),
-      NS_DISPATCH_SYNC);
-  ASSERT_FALSE(destroyed);
-  destructor = nullptr;
-  ASSERT_TRUE(destroyed);
+TEST_F(DispatchTest, TestMove) {
+  int destroyed = 0;
+  {
+    CtorDtorState state(&destroyed);
+
+    // Dispatch with:
+    //   - moved arg
+    //   - by-val capture in function should consume a move
+    //   - expect destruction in the function scope
+    target_->Dispatch(WrapRunnableNM([](CtorDtorState s) {}, std::move(state)),
+                      NS_DISPATCH_SYNC);
+    ASSERT_EQ(1, destroyed);
+  }
+  // Still shouldn't count when we go out of scope as it was moved.
+  ASSERT_EQ(1, destroyed);
+
+  {
+    CtorDtorState state(&destroyed);
+
+    // Dispatch with:
+    //   - copied arg
+    //   - by-val capture in function should consume a move
+    //   - expect destruction in the function scope and call scope
+    target_->Dispatch(WrapRunnableNM([](CtorDtorState s) {}, state),
+                      NS_DISPATCH_SYNC);
+    ASSERT_EQ(2, destroyed);
+  }
+  // Original state should be destroyed
+  ASSERT_EQ(3, destroyed);
+
+  {
+    CtorDtorState state(&destroyed);
+
+    // Dispatch with:
+    //   - moved arg
+    //   - by-ref in function should accept a moved arg
+    //   - expect destruction in the wrapper invocation scope
+    target_->Dispatch(
+        WrapRunnableNM([](const CtorDtorState& s) {}, std::move(state)),
+        NS_DISPATCH_SYNC);
+    ASSERT_EQ(4, destroyed);
+  }
+  // Still shouldn't count when we go out of scope as it was moved.
+  ASSERT_EQ(4, destroyed);
+
+  {
+    CtorDtorState state(&destroyed);
+
+    // Dispatch with:
+    //   - moved arg
+    //   - r-value function should accept a moved arg
+    //   - expect destruction in the wrapper invocation scope
+    target_->Dispatch(
+        WrapRunnableNM([](CtorDtorState&& s) {}, std::move(state)),
+        NS_DISPATCH_SYNC);
+    ASSERT_EQ(5, destroyed);
+  }
+  // Still shouldn't count when we go out of scope as it was moved.
+  ASSERT_EQ(5, destroyed);
+}
+
+TEST_F(DispatchTest, TestUniquePtr) {
+  // Test that holding the class in UniquePtr works
+  int ran = 0;
+  auto cl = MakeUnique<TargetClass>(&ran);
+
+  target_->Dispatch(WrapRunnable(std::move(cl), &TargetClass::m1, 1),
+                    NS_DISPATCH_SYNC);
+  ASSERT_EQ(1, ran);
+
+  // Test that UniquePtr works as a param to the runnable
+  int destroyed = 0;
+  {
+    auto state = MakeUnique<CtorDtorState>(&destroyed);
+
+    // Dispatch with:
+    //   - moved arg
+    //   - Function should move construct from arg
+    //   - expect destruction in the wrapper invocation scope
+    target_->Dispatch(
+        WrapRunnableNM([](UniquePtr<CtorDtorState> s) {}, std::move(state)),
+        NS_DISPATCH_SYNC);
+    ASSERT_EQ(1, destroyed);
+  }
+  // Still shouldn't count when we go out of scope as it was moved.
+  ASSERT_EQ(1, destroyed);
 }
 
 }  // end of namespace