From 198b894fe890f947dd9ae800fac59f29a5169a5e Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Thu, 20 Jun 2024 16:40:06 +0200 Subject: [PATCH] Make FunctionCache robust now --- example/ios/Podfile.lock | 2 +- .../cpp/core/HybridObject.cpp | 33 +++++++++++-------- .../cpp/jsi/FunctionCache.cpp | 2 +- .../cpp/react-native-nitro.hpp | 2 +- .../cpp/test-object/TestHybridObject.cpp | 3 -- .../cpp/test-object/TestHybridObject.hpp | 7 ---- .../ios/core/Promise.swift | 3 -- 7 files changed, 22 insertions(+), 30 deletions(-) diff --git a/example/ios/Podfile.lock b/example/ios/Podfile.lock index baa79d0ac..54bc2a58b 100644 --- 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 index 526a4da86..736010408 100644 --- 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 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(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 index c3031d4bd..a5f2b9f8a 100644 --- 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::getOrCreateCache(jsi::Runtime &runti } std::weak_ptr FunctionCache::makeGlobal(jsi::Function&& function) { - auto shared = std::make_shared(function); + auto shared = std::make_shared(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 index a337b0429..88c195540 100644 --- 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 index b202b2541..00132641d 100644 --- 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 index 61b5772ec..0151e5849 100644 --- 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 getNullableString() { return _nullableString; } @@ -83,7 +77,6 @@ class TestHybridObject : public HybridObject { private: int _int; std::string _string; - TestEnum _enum; std::optional _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 index 20517f911..dfb675ae4 100644 --- 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) } }