From 907228849e7d941e4e49442e5008948c68fb79bf Mon Sep 17 00:00:00 2001 From: Fabrizio Cucci Date: Mon, 11 Mar 2024 06:59:31 -0700 Subject: [PATCH 1/2] Scope LongLivedObjectCollection per runtime [2/n] (#43409) Summary: Changelog: [General] [Breaking] - Make `LongLivedObject` constructor accept a `Runtime` reference. # Context Approach 1 as described in [RFC post](https://fb.workplace.com/groups/615693552291894/permalink/1693347124526526/). # This diff * Embed `Runtime` reference in `LongLivedObject` to keep supporting `allowRelease` method (and update extending classes accordingly) * Update MSFT fork accordingly Reviewed By: RSNara Differential Revision: D54638813 --- .../ReactCommon/react/bridging/CallbackWrapper.h | 5 ++--- .../ReactCommon/react/bridging/LongLivedObject.h | 4 +++- packages/react-native/ReactCommon/react/bridging/Promise.h | 6 ++++-- .../nativemodule/core/ReactCommon/TurboModuleUtils.cpp | 4 +++- .../react/nativemodule/core/ReactCommon/TurboModuleUtils.h | 1 - 5 files changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h b/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h index 228f0948d8f3..336b9854de77 100644 --- a/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h +++ b/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h @@ -24,12 +24,11 @@ class CallbackWrapper : public LongLivedObject { jsi::Function&& callback, jsi::Runtime& runtime, std::shared_ptr jsInvoker) - : callback_(std::move(callback)), - runtime_(runtime), + : LongLivedObject(runtime), + callback_(std::move(callback)), jsInvoker_(std::move(jsInvoker)) {} jsi::Function callback_; - jsi::Runtime& runtime_; std::shared_ptr jsInvoker_; public: diff --git a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h index f8ca6671da90..0a9cdc8c1959 100644 --- a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h +++ b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -31,8 +32,9 @@ class LongLivedObject { virtual void allowRelease(); protected: - LongLivedObject() = default; + explicit LongLivedObject(jsi::Runtime& runtime) : runtime_(runtime) {} virtual ~LongLivedObject() = default; + jsi::Runtime& runtime_; }; /** diff --git a/packages/react-native/ReactCommon/react/bridging/Promise.h b/packages/react-native/ReactCommon/react/bridging/Promise.h index 058faea13d1f..35873952f677 100644 --- a/packages/react-native/ReactCommon/react/bridging/Promise.h +++ b/packages/react-native/ReactCommon/react/bridging/Promise.h @@ -34,7 +34,8 @@ class AsyncPromise { }, jsInvoker)); - auto promiseHolder = std::make_shared(promise.asObject(rt)); + auto promiseHolder = + std::make_shared(rt, promise.asObject(rt)); LongLivedObjectCollection::get().add(promiseHolder); // The shared state can retain the promise holder weakly now. @@ -71,7 +72,8 @@ class AsyncPromise { private: struct PromiseHolder : LongLivedObject { - PromiseHolder(jsi::Object p) : promise(std::move(p)) {} + PromiseHolder(jsi::Runtime& runtime, jsi::Object p) + : LongLivedObject(runtime), promise(std::move(p)) {} jsi::Object promise; }; diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.cpp index 328c3f71a584..2a7bbb514296 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.cpp @@ -63,7 +63,9 @@ jsi::Array deepCopyJSIArray(jsi::Runtime& rt, const jsi::Array& arr) { } Promise::Promise(jsi::Runtime& rt, jsi::Function resolve, jsi::Function reject) - : runtime_(rt), resolve_(std::move(resolve)), reject_(std::move(reject)) {} + : LongLivedObject(rt), + resolve_(std::move(resolve)), + reject_(std::move(reject)) {} void Promise::resolve(const jsi::Value& result) { resolve_.call(runtime_, result); diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h index 786c4cdb3cec..abc56408aee8 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h @@ -26,7 +26,6 @@ struct Promise : public LongLivedObject { void resolve(const jsi::Value& result); void reject(const std::string& error); - jsi::Runtime& runtime_; jsi::Function resolve_; jsi::Function reject_; }; From 19ab8f52e2455970d6cb260a1a72a7ad91214f0f Mon Sep 17 00:00:00 2001 From: Fabrizio Cucci Date: Mon, 11 Mar 2024 06:59:31 -0700 Subject: [PATCH 2/2] Scope LongLivedObjectCollection per runtime [3/n] Summary: Changelog: [General] [Breaking] - Make `LongLivedObjectCollection::get` accept a Runtime reference as parameter. # Context Approach 1 as described in [RFC post](https://fb.workplace.com/groups/615693552291894/permalink/1693347124526526/). # This diff * Replace the `LongLivedObjectCollection` singleton with a map from `Runtime -> LongLivedObjectCollection` so that each RN instance has its own collection. * Update MSFT fork accordingly Reviewed By: javache, RSNara Differential Revision: D54649209 --- .../react/bridging/CallbackWrapper.h | 2 +- .../react/bridging/LongLivedObject.cpp | 19 +++++++++++++++---- .../react/bridging/LongLivedObject.h | 2 +- .../ReactCommon/react/bridging/Promise.h | 2 +- .../react/bridging/tests/BridgingTest.cpp | 2 +- .../react/bridging/tests/BridgingTest.h | 4 ++-- .../core/ReactCommon/TurboModuleBinding.cpp | 12 ++++++++---- .../core/ReactCommon/TurboModuleBinding.h | 2 ++ 8 files changed, 31 insertions(+), 14 deletions(-) diff --git a/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h b/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h index 336b9854de77..122d9f75b7db 100644 --- a/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h +++ b/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h @@ -38,7 +38,7 @@ class CallbackWrapper : public LongLivedObject { std::shared_ptr jsInvoker) { auto wrapper = std::shared_ptr(new CallbackWrapper( std::move(callback), runtime, std::move(jsInvoker))); - LongLivedObjectCollection::get().add(wrapper); + LongLivedObjectCollection::get(runtime).add(wrapper); return wrapper; } diff --git a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp index 410f52d22ea9..64a93c1b7ec5 100644 --- a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp +++ b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp @@ -6,13 +6,24 @@ */ #include "LongLivedObject.h" +#include namespace facebook::react { // LongLivedObjectCollection -LongLivedObjectCollection& LongLivedObjectCollection::get() { - static LongLivedObjectCollection instance; - return instance; + +LongLivedObjectCollection& LongLivedObjectCollection::get( + jsi::Runtime& runtime) { + static std::unordered_map> + instances; + void* key = static_cast(&runtime); + auto entry = instances.find(key); + if (entry == instances.end()) { + entry = + instances.emplace(key, std::make_shared()) + .first; + } + return *(entry->second); } void LongLivedObjectCollection::add(std::shared_ptr so) { @@ -43,7 +54,7 @@ size_t LongLivedObjectCollection::size() const { // LongLivedObject void LongLivedObject::allowRelease() { - LongLivedObjectCollection::get().remove(this); + LongLivedObjectCollection::get(runtime_).remove(this); } } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h index 0a9cdc8c1959..d043a0a9a919 100644 --- a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h +++ b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h @@ -42,7 +42,7 @@ class LongLivedObject { */ class LongLivedObjectCollection { public: - static LongLivedObjectCollection& get(); + static LongLivedObjectCollection& get(jsi::Runtime& runtime); LongLivedObjectCollection() = default; LongLivedObjectCollection(const LongLivedObjectCollection&) = delete; diff --git a/packages/react-native/ReactCommon/react/bridging/Promise.h b/packages/react-native/ReactCommon/react/bridging/Promise.h index 35873952f677..5a049c8574e5 100644 --- a/packages/react-native/ReactCommon/react/bridging/Promise.h +++ b/packages/react-native/ReactCommon/react/bridging/Promise.h @@ -36,7 +36,7 @@ class AsyncPromise { auto promiseHolder = std::make_shared(rt, promise.asObject(rt)); - LongLivedObjectCollection::get().add(promiseHolder); + LongLivedObjectCollection::get(rt).add(promiseHolder); // The shared state can retain the promise holder weakly now. state_->promiseHolder = promiseHolder; diff --git a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp index e89c4eeaa2b1..45beaeff7b4e 100644 --- a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp +++ b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp @@ -316,7 +316,7 @@ TEST_F(BridgingTest, asyncCallbackInvalidation) { [](jsi::Runtime& rt, jsi::Function& f) { f.call(rt, "hello"); }); // LongLivedObjectCollection goes away before callback is executed - LongLivedObjectCollection::get().clear(); + LongLivedObjectCollection::get(rt).clear(); flushQueue(); diff --git a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h index ffc4b49d67be..7fef3c9726a2 100644 --- a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h +++ b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h @@ -43,14 +43,14 @@ class BridgingTest : public ::testing::Test { rt(*runtime) {} ~BridgingTest() { - LongLivedObjectCollection::get().clear(); + LongLivedObjectCollection::get(rt).clear(); } void TearDown() override { flushQueue(); // After flushing the invoker queue, we shouldn't leak memory. - EXPECT_EQ(0, LongLivedObjectCollection::get().size()); + EXPECT_EQ(0, LongLivedObjectCollection::get(rt).size()); } jsi::Value eval(const std::string& js) { diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp index 360ad45f9da8..e4d1a6e79e5a 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp @@ -67,9 +67,11 @@ class BridgelessNativeModuleProxy : public jsi::HostObject { */ TurboModuleBinding::TurboModuleBinding( + jsi::Runtime& runtime, TurboModuleProviderFunctionType&& moduleProvider, std::shared_ptr longLivedObjectCollection) - : moduleProvider_(std::move(moduleProvider)), + : runtime_(runtime), + moduleProvider_(std::move(moduleProvider)), longLivedObjectCollection_(std::move(longLivedObjectCollection)) {} void TurboModuleBinding::install( @@ -85,7 +87,7 @@ void TurboModuleBinding::install( jsi::PropNameID::forAscii(runtime, "__turboModuleProxy"), 1, [binding = TurboModuleBinding( - std::move(moduleProvider), longLivedObjectCollection)]( + runtime, std::move(moduleProvider), longLivedObjectCollection)]( jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, @@ -102,7 +104,9 @@ void TurboModuleBinding::install( bool rnTurboInterop = legacyModuleProvider != nullptr; auto turboModuleBinding = legacyModuleProvider ? std::make_unique( - std::move(legacyModuleProvider), longLivedObjectCollection) + runtime, + std::move(legacyModuleProvider), + longLivedObjectCollection) : nullptr; auto nativeModuleProxy = std::make_shared( std::move(turboModuleBinding)); @@ -119,7 +123,7 @@ TurboModuleBinding::~TurboModuleBinding() { if (longLivedObjectCollection_) { longLivedObjectCollection_->clear(); } else { - LongLivedObjectCollection::get().clear(); + LongLivedObjectCollection::get(runtime_).clear(); } } diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h index 0556c697f1e7..d45a72f25ca2 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h @@ -34,6 +34,7 @@ class TurboModuleBinding { nullptr); TurboModuleBinding( + jsi::Runtime& runtime, TurboModuleProviderFunctionType&& moduleProvider, std::shared_ptr longLivedObjectCollection); @@ -49,6 +50,7 @@ class TurboModuleBinding { jsi::Value getModule(jsi::Runtime& runtime, const std::string& moduleName) const; + jsi::Runtime& runtime_; TurboModuleProviderFunctionType moduleProvider_; std::shared_ptr longLivedObjectCollection_; };