Make FunctionCache robust now

mrousavy · Jun 20, 2024 · 198b894 · 198b894
1 parent 166feb4
commit 198b894
Show file tree

Hide file tree

Showing 7 changed files with 22 additions and 30 deletions.
diff --git a/example/ios/Podfile.lock b/example/ios/Podfile.lock
@@ -1371,7 +1371,7 @@ SPEC CHECKSUMS:
   fmt: 4c2741a687cc09f0634a2e2c72a838b99f1ff120
   glog: fdfdfe5479092de0c4bdbebedd9056951f092c4f
   hermes-engine: 01d3e052018c2a13937aca1860fbedbccd4a41b7
-  NitroModules: 16e8fac285dd9355d9ad84a4d4ad6c59c4aa55de
+  NitroModules: cdc2f86b357f8015baa3f8e3201c381c5c16c622
   RCT-Folly: 02617c592a293bd6d418e0a88ff4ee1f88329b47
   RCTDeprecation: b03c35057846b685b3ccadc9bfe43e349989cdb2
   RCTRequired: 194626909cfa8d39ca6663138c417bc6c431648c

diff --git a/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp b/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp
@@ -73,26 +73,32 @@ jsi::Value HybridObject::get(facebook::jsi::Runtime& runtime, const facebook::js
   std::string name = propName.utf8(runtime);
   auto& functionCache = _functionCache[&runtime];
 
-
-  if (functionCache.count(name) > 0) {
-    [[likely]];
-    // cache hit
-    return jsi::Value(runtime, *functionCache[name]);
+  if (functionCache.contains(name)) [[likely]] {
+    // cache hit - let's see if the function is still alive..
+    std::shared_ptr<jsi::Function> function = functionCache[name].lock();
+    if (function != nullptr) [[likely]] {
+      // function is still alive, we can use it.
+      return jsi::Value(runtime, *function);
+    }
   }
 
-  if (_getters.count(name) > 0) {
-    // it's a property getter
+  if (_getters.contains(name)) {
+    // it's a property getter. call it directly
     return _getters[name](runtime, jsi::Value::undefined(), nullptr, 0);
   }
 
-  if (_methods.count(name) > 0) {
-    // cache miss - create jsi::Function and cache it.
+  if (_methods.contains(name)) {
+    // it's a function. we now need to wrap it in a jsi::Function, store it in cache, then return it.
     HybridFunction& hybridFunction = _methods.at(name);
+    // get (or create) a runtime-specific function cache
+    auto runtimeCache = FunctionCache::getOrCreateCache(runtime).lock();
+    // create the jsi::Function
     jsi::Function function = jsi::Function::createFromHostFunction(runtime, jsi::PropNameID::forUtf8(runtime, name),
                                                                    hybridFunction.parameterCount, hybridFunction.function);
-    // TODO: Weakify function using RuntimeWatch so the Runtime safely manages it's lifetime, not us.
-    functionCache[name] = std::make_shared<jsi::Function>(std::move(function));
-    return jsi::Value(runtime, *functionCache[name]);
+    // throw it into the cache
+    auto globalFunction = runtimeCache->makeGlobal(std::move(function));
+    functionCache[name] = globalFunction;
+    return jsi::Value(runtime, *globalFunction.lock());
   }
 
   if (name == "toString") {
@@ -123,8 +129,7 @@ void HybridObject::set(facebook::jsi::Runtime& runtime, const facebook::jsi::Pro
 }
 
 void HybridObject::ensureInitialized(facebook::jsi::Runtime& runtime) {
-  if (!_didLoadMethods) {
-    [[unlikely]];
+  if (!_didLoadMethods) [[unlikely]] {
     // lazy-load all exposed methods
     loadHybridMethods();
     _didLoadMethods = true;

diff --git a/packages/react-native-nitro-modules/cpp/jsi/FunctionCache.cpp b/packages/react-native-nitro-modules/cpp/jsi/FunctionCache.cpp
@@ -37,7 +37,7 @@ std::weak_ptr<FunctionCache> FunctionCache::getOrCreateCache(jsi::Runtime &runti
 }
 
 std::weak_ptr<jsi::Function> FunctionCache::makeGlobal(jsi::Function&& function) {
-  auto shared = std::make_shared<jsi::Function>(function);
+  auto shared = std::make_shared<jsi::Function>(std::move(function));
   _cache.push_back(shared);
   return std::weak_ptr(shared);
 }

diff --git a/packages/react-native-nitro-modules/cpp/react-native-nitro.hpp b/packages/react-native-nitro-modules/cpp/react-native-nitro.hpp
@@ -1,7 +1,7 @@
 #ifndef NITRO_H
 #define NITRO_H
 
-#include "test-object/TestHybridObject.hpp"
+#include "TestHybridObject.hpp"
 
 namespace nitro {
 

diff --git a/packages/react-native-nitro-modules/cpp/test-object/TestHybridObject.cpp b/packages/react-native-nitro-modules/cpp/test-object/TestHybridObject.cpp
@@ -16,9 +16,6 @@ void TestHybridObject::loadHybridMethods() {
   // this.nullableString get & set
   registerHybridGetter("nullableString", &TestHybridObject::getNullableString, this);
   registerHybridSetter("nullableString", &TestHybridObject::setNullableString, this);
-  // this.enum
-  registerHybridGetter("enum", &TestHybridObject::getEnum, this);
-  registerHybridSetter("enum", &TestHybridObject::setEnum, this);
   // methods
   registerHybridMethod("multipleArguments", &TestHybridObject::multipleArguments, this);
   // callbacks

diff --git a/packages/react-native-nitro-modules/cpp/test-object/TestHybridObject.hpp b/packages/react-native-nitro-modules/cpp/test-object/TestHybridObject.hpp
@@ -28,12 +28,6 @@ class TestHybridObject : public HybridObject {
   void setString(const std::string& newValue) {
     _string = newValue;
   }
-  void setEnum(TestEnum testEnum) {
-    _enum = testEnum;
-  }
-  TestEnum getEnum() {
-    return _enum;
-  }
   std::optional<std::string> getNullableString() {
     return _nullableString;
   }
@@ -83,7 +77,6 @@ class TestHybridObject : public HybridObject {
 private:
   int _int;
   std::string _string;
-  TestEnum _enum;
   std::optional<std::string> _nullableString;
 
   void loadHybridMethods() override;

diff --git a/packages/react-native-nitro-modules/ios/core/Promise.swift b/packages/react-native-nitro-modules/ios/core/Promise.swift
@@ -9,9 +9,6 @@ public class Promise {
   private let promise: margelo.Promise? = nil
 
   init() {
-    let xh = nitro.multiply(5, 5)
-    let x: margelo.PromiseFactory.RunPromise? = nil
-    let y = nitro.multiply(0, 0)
   }
 
 }