JavaTurboModule/LongLivedObject cleanup (#36367)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36367

* Don't iterate over JSI Value to get args, convert each of the args individually instead
* Make LongLivedObjectCollection constructor private
* allowRelease does not need to be virtual
* Move away from const-ness as a thread-safety indicator

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D43354535

fbshipit-source-id: eaf8eb931ab6ec307a3dc2690eabeb34bb6afa77

ReactCommon/react/bridging/CallbackWrapper.h

@@ -35,8 +35,8 @@ class CallbackWrapper : public LongLivedObject {
       jsi::Function &&callback,
       jsi::Runtime &runtime,
       std::shared_ptr<CallInvoker> jsInvoker) {
-    auto wrapper = std::shared_ptr<CallbackWrapper>(
-        new CallbackWrapper(std::move(callback), runtime, jsInvoker));
+    auto wrapper = std::shared_ptr<CallbackWrapper>(new CallbackWrapper(
+        std::move(callback), runtime, std::move(jsInvoker)));
     LongLivedObjectCollection::get().add(wrapper);
     return wrapper;
   }

ReactCommon/react/bridging/LongLivedObject.cpp

@@ -16,27 +16,22 @@ LongLivedObjectCollection &LongLivedObjectCollection::get() {
   return instance;
 }
 
-LongLivedObjectCollection::LongLivedObjectCollection() {}
-
-void LongLivedObjectCollection::add(std::shared_ptr<LongLivedObject> so) const {
+void LongLivedObjectCollection::add(std::shared_ptr<LongLivedObject> so) {
   std::lock_guard<std::mutex> lock(collectionMutex_);
-  collection_.insert(so);
+  collection_.insert(std::move(so));
 }
 
-void LongLivedObjectCollection::remove(const LongLivedObject *o) const {
+void LongLivedObjectCollection::remove(const LongLivedObject *o) {
   std::lock_guard<std::mutex> lock(collectionMutex_);
-  auto p = collection_.begin();
-  for (; p != collection_.end(); p++) {
+  for (auto p = collection_.begin(); p != collection_.end(); p++) {
     if (p->get() == o) {
+      collection_.erase(p);
       break;
     }
   }
-  if (p != collection_.end()) {
-    collection_.erase(p);
-  }
 }
 
-void LongLivedObjectCollection::clear() const {
+void LongLivedObjectCollection::clear() {
   std::lock_guard<std::mutex> lock(collectionMutex_);
   collection_.clear();
 }
@@ -47,8 +42,6 @@ size_t LongLivedObjectCollection::size() const {
 }
 
 // LongLivedObject
-LongLivedObject::LongLivedObject() {}
-LongLivedObject::~LongLivedObject() {}
 
 void LongLivedObject::allowRelease() {
   LongLivedObjectCollection::get().remove(this);

ReactCommon/react/bridging/LongLivedObject.h

@@ -26,11 +26,11 @@ namespace react {
  */
 class LongLivedObject {
  public:
-  virtual void allowRelease();
+  void allowRelease();
 
  protected:
-  LongLivedObject();
-  virtual ~LongLivedObject();
+  LongLivedObject() = default;
+  virtual ~LongLivedObject() = default;
 };
 
 /**
@@ -40,17 +40,18 @@ class LongLivedObjectCollection {
  public:
   static LongLivedObjectCollection &get();
 
-  LongLivedObjectCollection();
   LongLivedObjectCollection(LongLivedObjectCollection const &) = delete;
   void operator=(LongLivedObjectCollection const &) = delete;
 
-  void add(std::shared_ptr<LongLivedObject> o) const;
-  void remove(const LongLivedObject *o) const;
-  void clear() const;
+  void add(std::shared_ptr<LongLivedObject> o);
+  void remove(const LongLivedObject *o);
+  void clear();
   size_t size() const;
 
  private:
-  mutable std::unordered_set<std::shared_ptr<LongLivedObject>> collection_;
+  LongLivedObjectCollection() = default;
+
+  std::unordered_set<std::shared_ptr<LongLivedObject>> collection_;
   mutable std::mutex collectionMutex_;
 };
 

ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.cpp

@@ -79,13 +79,13 @@ void Promise::reject(const std::string &message) {
 
 jsi::Value createPromiseAsJSIValue(
     jsi::Runtime &rt,
-    const PromiseSetupFunctionType func) {
+    PromiseSetupFunctionType &&func) {
   jsi::Function JSPromise = rt.global().getPropertyAsFunction(rt, "Promise");
   jsi::Function fn = jsi::Function::createFromHostFunction(
       rt,
       jsi::PropNameID::forAscii(rt, "fn"),
       2,
-      [func](
+      [func = std::move(func)](
           jsi::Runtime &rt2,
           const jsi::Value &thisVal,
           const jsi::Value *args,

ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h

@@ -36,7 +36,7 @@ using PromiseSetupFunctionType =
     std::function<void(jsi::Runtime &rt, std::shared_ptr<Promise>)>;
 jsi::Value createPromiseAsJSIValue(
     jsi::Runtime &rt,
-    const PromiseSetupFunctionType func);
+    PromiseSetupFunctionType &&func);
 
 class RAIICallbackWrapperDestroyer {
  public:

ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp

@@ -15,6 +15,7 @@
 #include <ReactCommon/TurboModule.h>
 #include <ReactCommon/TurboModulePerfLogger.h>
 #include <ReactCommon/TurboModuleUtils.h>
+#include <butter/function.h>
 #include <jsi/JSIDynamic.h>
 #include <react/debug/react_native_assert.h>
 #include <react/jni/NativeMap.h>
@@ -77,12 +78,13 @@ jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
   auto callbackWrapperOwner =
       std::make_shared<RAIICallbackWrapperDestroyer>(weakWrapper);
 
-  std::function<void(folly::dynamic)> fn =
-      [weakWrapper, callbackWrapperOwner, wrapperWasCalled = false](
-          folly::dynamic responses) mutable {
+  return JCxxCallbackImpl::newObjectCxxArgs(
+      [weakWrapper = std::move(weakWrapper),
+       callbackWrapperOwner = std::move(callbackWrapperOwner),
+       wrapperWasCalled = false](folly::dynamic responses) mutable {
         if (wrapperWasCalled) {
           throw std::runtime_error(
-              "callback 2 arg cannot be called more than once");
+              "Callback arg cannot be called more than once");
         }
 
         auto strongWrapper = weakWrapper.lock();
@@ -91,37 +93,29 @@ jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
         }
 
         strongWrapper->jsInvoker().invokeAsync(
-            [weakWrapper, callbackWrapperOwner, responses]() mutable {
+            [weakWrapper = std::move(weakWrapper),
+             callbackWrapperOwner = std::move(callbackWrapperOwner),
+             responses = std::move(responses)]() {
               auto strongWrapper2 = weakWrapper.lock();
               if (!strongWrapper2) {
                 return;
               }
 
-              // TODO (T43155926) valueFromDynamic already returns a Value
-              // array. Don't iterate again
-              jsi::Value args =
-                  jsi::valueFromDynamic(strongWrapper2->runtime(), responses);
-              auto argsArray = args.getObject(strongWrapper2->runtime())
-                                   .asArray(strongWrapper2->runtime());
-              std::vector<jsi::Value> result;
-              for (size_t i = 0; i < argsArray.size(strongWrapper2->runtime());
-                   i++) {
-                result.emplace_back(
-                    strongWrapper2->runtime(),
-                    argsArray.getValueAtIndex(strongWrapper2->runtime(), i));
+              std::vector<jsi::Value> args;
+              args.reserve(responses.size());
+              for (const auto &val : responses) {
+                args.emplace_back(
+                    jsi::valueFromDynamic(strongWrapper2->runtime(), val));
               }
+
               strongWrapper2->callback().call(
                   strongWrapper2->runtime(),
-                  (const jsi::Value *)result.data(),
-                  result.size());
-
-              callbackWrapperOwner.reset();
+                  (const jsi::Value *)args.data(),
+                  args.size());
             });
 
         wrapperWasCalled = true;
-      };
-
-  return JCxxCallbackImpl::newObjectCxxArgs(fn);
+      });
 }
 
 // This is used for generating short exception strings.
@@ -369,8 +363,7 @@ jsi::Value convertFromJMapToValue(JNIEnv *env, jsi::Runtime &rt, jobject arg) {
       jArguments,
       "makeNativeMap",
       "(Ljava/util/Map;)Lcom/facebook/react/bridge/WritableNativeMap;");
-  auto constants =
-      (jobject)env->CallStaticObjectMethod(jArguments, jMakeNativeMap, arg);
+  auto constants = env->CallStaticObjectMethod(jArguments, jMakeNativeMap, arg);
   auto jResult = jni::adopt_local(constants);
   auto result = jni::static_ref_cast<NativeMap::jhybridobject>(jResult);
   return jsi::valueFromDynamic(rt, result->cthis()->consume());