From 84aad620d808f96976de68bde2c5b0bff0ca2ff2 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 13 Nov 2023 14:12:46 -0800 Subject: [PATCH 1/3] Release long lived JSI objects ASAP --- .../JSDispatcherWriter.cpp | 6 + .../JSDispatcherWriter.h | 1 + .../TurboModulesProvider.cpp | 132 ++++++++++++------ 3 files changed, 99 insertions(+), 40 deletions(-) diff --git a/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp b/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp index 0d934013fa5..577880dc848 100644 --- a/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp +++ b/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp @@ -33,6 +33,12 @@ JSDispatcherWriter::JSDispatcherWriter( std::weak_ptr jsiRuntimeHolder) noexcept : m_jsDispatcher(jsDispatcher), m_jsiRuntimeHolder(std::move(jsiRuntimeHolder)) {} +JSDispatcherWriter::~JSDispatcherWriter() { + if (auto jsiRuntimeHolder = m_jsiRuntimeHolder.lock()) { + jsiRuntimeHolder->allowRelease(); + } +} + void JSDispatcherWriter::WithResultArgs( Mso::Functor handler) noexcept { diff --git a/vnext/Microsoft.ReactNative/JSDispatcherWriter.h b/vnext/Microsoft.ReactNative/JSDispatcherWriter.h index 1599e48905a..50b2cb1c107 100644 --- a/vnext/Microsoft.ReactNative/JSDispatcherWriter.h +++ b/vnext/Microsoft.ReactNative/JSDispatcherWriter.h @@ -14,6 +14,7 @@ namespace winrt::Microsoft::ReactNative { // In case if writing is done outside of JSDispatcher, it uses DynamicWriter to create // folly::dynamic which then is written to JsiWriter in JSDispatcher. struct JSDispatcherWriter : winrt::implements { + ~JSDispatcherWriter(); JSDispatcherWriter( IReactDispatcher const &jsDispatcher, std::weak_ptr jsiRuntimeHolder) noexcept; diff --git a/vnext/Microsoft.ReactNative/TurboModulesProvider.cpp b/vnext/Microsoft.ReactNative/TurboModulesProvider.cpp index 105dac727b0..6c179ca2dad 100644 --- a/vnext/Microsoft.ReactNative/TurboModulesProvider.cpp +++ b/vnext/Microsoft.ReactNative/TurboModulesProvider.cpp @@ -221,11 +221,46 @@ class TurboModuleImpl : public facebook::react::TurboModule { VerifyElseCrash(argCount > 1); if (auto strongLongLivedObjectCollection = longLivedObjectCollection.lock()) { auto jsiRuntimeHolder = LongLivedJsiRuntime::CreateWeak(strongLongLivedObjectCollection, rt); + auto weakCallback1 = LongLivedJsiFunction::CreateWeak( + strongLongLivedObjectCollection, rt, args[argCount - 2].getObject(rt).getFunction(rt)); + auto weakCallback2 = LongLivedJsiFunction::CreateWeak( + strongLongLivedObjectCollection, rt, args[argCount - 1].getObject(rt).getFunction(rt)); + method( winrt::make(rt, args, argCount - 2), winrt::make(jsDispatcher, jsiRuntimeHolder), - MakeCallback(rt, strongLongLivedObjectCollection, args[argCount - 2]), - MakeCallback(rt, strongLongLivedObjectCollection, args[argCount - 1])); + [weakCallback1, weakCallback2, jsiRuntimeHolder](const IJSValueWriter &writer) noexcept { + writer.as()->WithResultArgs( + [weakCallback1, weakCallback2, jsiRuntimeHolder]( + facebook::jsi::Runtime &rt, facebook::jsi::Value const *args, size_t count) { + if (auto callback1 = weakCallback1.lock()) { + callback1->Value().call(rt, args, count); + callback1->allowRelease(); + } + if (auto callback2 = weakCallback2.lock()) { + callback2->allowRelease(); + } + if (auto runtimeHolder = jsiRuntimeHolder.lock()) { + runtimeHolder->allowRelease(); + } + }); + }, + [weakCallback1, weakCallback2, jsiRuntimeHolder](const IJSValueWriter &writer) noexcept { + writer.as()->WithResultArgs( + [weakCallback1, weakCallback2, jsiRuntimeHolder]( + facebook::jsi::Runtime &rt, facebook::jsi::Value const *args, size_t count) { + if (auto callback2 = weakCallback2.lock()) { + callback2->Value().call(rt, args, count); + callback2->allowRelease(); + } + if (auto callback1 = weakCallback1.lock()) { + callback1->allowRelease(); + } + if (auto runtimeHolder = jsiRuntimeHolder.lock()) { + runtimeHolder->allowRelease(); + } + }); + }); } return facebook::jsi::Value::undefined(); }); @@ -247,49 +282,65 @@ class TurboModuleImpl : public facebook::react::TurboModule { auto argWriter = winrt::make(jsDispatcher, jsiRuntimeHolder); return facebook::react::createPromiseAsJSIValue( rt, - [method, argReader, argWriter, strongLongLivedObjectCollection]( + [method, argReader, argWriter, strongLongLivedObjectCollection, jsiRuntimeHolder]( facebook::jsi::Runtime &runtime, std::shared_ptr promise) { + auto weakResolve = LongLivedJsiFunction::CreateWeak( + strongLongLivedObjectCollection, runtime, std::move(promise->resolve_)); + auto weakReject = LongLivedJsiFunction::CreateWeak( + strongLongLivedObjectCollection, runtime, std::move(promise->reject_)); method( argReader, argWriter, - [weakResolve = LongLivedJsiFunction::CreateWeak( - strongLongLivedObjectCollection, runtime, std::move(promise->resolve_))]( - const IJSValueWriter &writer) { - writer.as()->WithResultArgs([weakResolve]( - facebook::jsi::Runtime &runtime, - facebook::jsi::Value const *args, - size_t argCount) { - VerifyElseCrash(argCount == 1); - if (auto resolveHolder = weakResolve.lock()) { - resolveHolder->Value().call(runtime, args[0]); - } - }); + [weakResolve, weakReject, jsiRuntimeHolder](const IJSValueWriter &writer) { + writer.as()->WithResultArgs( + [weakResolve, weakReject, jsiRuntimeHolder]( + facebook::jsi::Runtime &runtime, + facebook::jsi::Value const *args, + size_t argCount) { + VerifyElseCrash(argCount == 1); + if (auto resolveHolder = weakResolve.lock()) { + resolveHolder->Value().call(runtime, args[0]); + resolveHolder->allowRelease(); + } + if (auto rejectHolder = weakReject.lock()) { + rejectHolder->allowRelease(); + } + if (auto runtimeHolder = jsiRuntimeHolder.lock()) { + runtimeHolder->allowRelease(); + } + }); }, - [weakReject = LongLivedJsiFunction::CreateWeak( - strongLongLivedObjectCollection, runtime, std::move(promise->reject_))]( - const IJSValueWriter &writer) { - writer.as()->WithResultArgs([weakReject]( - facebook::jsi::Runtime &runtime, - facebook::jsi::Value const *args, - size_t argCount) { - VerifyElseCrash(argCount == 1); - if (auto rejectHolder = weakReject.lock()) { - // To match the Android and iOS TurboModule behavior we create the Error object for - // the Promise rejection the same way as in updateErrorWithErrorData method. - // See react-native/Libraries/BatchedBridge/NativeModules.js for details. - auto error = runtime.global() - .getPropertyAsFunction(runtime, "Error") - .callAsConstructor(runtime, {}); - auto &errorData = args[0]; - if (errorData.isObject()) { - runtime.global() - .getPropertyAsObject(runtime, "Object") - .getPropertyAsFunction(runtime, "assign") - .call(runtime, error, errorData.getObject(runtime)); - } - rejectHolder->Value().call(runtime, args[0]); - } - }); + [weakResolve, weakReject, jsiRuntimeHolder](const IJSValueWriter &writer) { + writer.as()->WithResultArgs( + [weakResolve, weakReject, jsiRuntimeHolder]( + facebook::jsi::Runtime &runtime, + facebook::jsi::Value const *args, + size_t argCount) { + VerifyElseCrash(argCount == 1); + if (auto rejectHolder = weakReject.lock()) { + // To match the Android and iOS TurboModule behavior we create the Error object + // for the Promise rejection the same way as in updateErrorWithErrorData method. + // See react-native/Libraries/BatchedBridge/NativeModules.js for details. + auto error = runtime.global() + .getPropertyAsFunction(runtime, "Error") + .callAsConstructor(runtime, {}); + auto &errorData = args[0]; + if (errorData.isObject()) { + runtime.global() + .getPropertyAsObject(runtime, "Object") + .getPropertyAsFunction(runtime, "assign") + .call(runtime, error, errorData.getObject(runtime)); + } + rejectHolder->Value().call(runtime, args[0]); + rejectHolder->allowRelease(); + } + if (auto resolveHolder = weakResolve.lock()) { + resolveHolder->allowRelease(); + } + if (auto runtimeHolder = jsiRuntimeHolder.lock()) { + runtimeHolder->allowRelease(); + } + }); }); }); } @@ -347,6 +398,7 @@ class TurboModuleImpl : public facebook::react::TurboModule { [weakCallback](facebook::jsi::Runtime &rt, facebook::jsi::Value const *args, size_t count) { if (auto callback = weakCallback.lock()) { callback->Value().call(rt, args, count); + callback->allowRelease(); } }); }; From e245ae0e9109d723f623412e30827c1c2b683bcc Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 13 Nov 2023 14:13:04 -0800 Subject: [PATCH 2/3] Change files --- ...ative-windows-86ce2be3-b89f-4ddb-9476-45f3d6c9cd21.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/react-native-windows-86ce2be3-b89f-4ddb-9476-45f3d6c9cd21.json diff --git a/change/react-native-windows-86ce2be3-b89f-4ddb-9476-45f3d6c9cd21.json b/change/react-native-windows-86ce2be3-b89f-4ddb-9476-45f3d6c9cd21.json new file mode 100644 index 00000000000..a33f57e58ce --- /dev/null +++ b/change/react-native-windows-86ce2be3-b89f-4ddb-9476-45f3d6c9cd21.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Release long lived JSI objects ASAP", + "packageName": "react-native-windows", + "email": "vmorozov@microsoft.com", + "dependentChangeType": "patch" +} From 31a2b4ee37c027fbdb48e3be13437c8c2c67e20b Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 13 Nov 2023 18:07:09 -0800 Subject: [PATCH 3/3] Fix scenario BG thread scenario --- .../JSDispatcherWriter.cpp | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp b/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp index 577880dc848..264600596ef 100644 --- a/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp +++ b/vnext/Microsoft.ReactNative/JSDispatcherWriter.cpp @@ -55,17 +55,18 @@ void JSDispatcherWriter::WithResultArgs( VerifyElseCrash(!m_jsiWriter); folly::dynamic dynValue = m_dynamicWriter->TakeValue(); VerifyElseCrash(dynValue.isArray()); - m_jsDispatcher.Post([handler, dynValue = std::move(dynValue), weakJsiRuntimeHolder = m_jsiRuntimeHolder]() { - if (auto jsiRuntimeHolder = weakJsiRuntimeHolder.lock()) { - std::vector args; - args.reserve(dynValue.size()); - auto &runtime = jsiRuntimeHolder->Runtime(); - for (auto const &item : dynValue) { - args.emplace_back(facebook::jsi::valueFromDynamic(runtime, item)); - } - handler(runtime, args.data(), args.size()); - } - }); + m_jsDispatcher.Post( + [handler, dynValue = std::move(dynValue), weakJsiRuntimeHolder = m_jsiRuntimeHolder, self = get_strong()]() { + if (auto jsiRuntimeHolder = weakJsiRuntimeHolder.lock()) { + std::vector args; + args.reserve(dynValue.size()); + auto &runtime = jsiRuntimeHolder->Runtime(); + for (auto const &item : dynValue) { + args.emplace_back(facebook::jsi::valueFromDynamic(runtime, item)); + } + handler(runtime, args.data(), args.size()); + } + }); } }