From 034c6dfe34d240cf7c6314e767716317fa554351 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 2 Aug 2021 11:19:47 -0700 Subject: [PATCH] Stop sharing LongLivedObjectCollection with the bridge Summary: This is the Android analogue to D30019833. Changelog: [Internal] Reviewed By: p-sun Differential Revision: D30029295 fbshipit-source-id: 13df0dfb915697eeedcc527dcdb6c246e89afb0c --- .../react/config/ReactFeatureFlags.java | 20 ++++++++ .../turbomodule/core/TurboModuleManager.java | 9 +++- .../jni/ReactCommon/TurboModuleManager.cpp | 47 ++++++++++++++++--- .../core/jni/ReactCommon/TurboModuleManager.h | 12 ++++- .../android/ReactCommon/JavaTurboModule.cpp | 31 ++++++++---- .../android/ReactCommon/JavaTurboModule.h | 12 ++++- 6 files changed, 110 insertions(+), 21 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 5e4574f4481..9955932d4ff 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -28,6 +28,26 @@ public class ReactFeatureFlags { /** Should we dispatch TurboModule methods with promise returns to the NativeModules thread? */ public static volatile boolean enableTurboModulePromiseAsyncDispatch = false; + /** + * Experiment: + * + *

Bridge and Bridgeless mode can run concurrently. This means that there can be two + * TurboModule systems alive at the same time. + * + *

The TurboModule system stores all JS callbacks in a global LongLivedObjectCollection. This + * collection is cleared when the JS VM is torn down. Implication: Tearing down the bridge JSVM + * invalidates the bridgeless JSVM's callbacks, and vice versa. + * + *

useGlobalCallbackCleanupScopeUsingRetainJSCallback => Use a retainJSCallbacks lambda to + * store jsi::Functions into the global LongLivedObjectCollection + * + *

useTurboModuleManagerCallbackCleanupScope => Use a retainJSCallbacks labmda to store + * jsi::Functions into a LongLivedObjectCollection owned by the TurboModuleManager + */ + public static boolean useGlobalCallbackCleanupScopeUsingRetainJSCallback = false; + + public static boolean useTurboModuleManagerCallbackCleanupScope = false; + /** This feature flag enables logs for Fabric */ public static boolean enableFabricLogs = false; diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java index fe0399529bd..6a592ae02fb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java @@ -16,6 +16,7 @@ import com.facebook.proguard.annotations.DoNotStrip; import com.facebook.react.bridge.CxxModuleWrapper; import com.facebook.react.bridge.JSIModule; import com.facebook.react.bridge.RuntimeExecutor; +import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.turbomodule.core.interfaces.CallInvokerHolder; import com.facebook.react.turbomodule.core.interfaces.TurboModule; import com.facebook.react.turbomodule.core.interfaces.TurboModuleRegistry; @@ -58,7 +59,9 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { runtimeExecutor, (CallInvokerHolderImpl) jsCallInvokerHolder, (CallInvokerHolderImpl) nativeCallInvokerHolder, - delegate); + delegate, + ReactFeatureFlags.useGlobalCallbackCleanupScopeUsingRetainJSCallback, + ReactFeatureFlags.useTurboModuleManagerCallbackCleanupScope); installJSIBindings(); mEagerInitModuleNames = @@ -290,7 +293,9 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { RuntimeExecutor runtimeExecutor, CallInvokerHolderImpl jsCallInvokerHolder, CallInvokerHolderImpl nativeCallInvokerHolder, - TurboModuleManagerDelegate tmmDelegate); + TurboModuleManagerDelegate tmmDelegate, + boolean useGlobalCallbackCleanupScopeUsingRetainJSCallback, + boolean useTurboModuleManagerCallbackCleanupScope); private native void installJSIBindings(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp index 5878375bd36..ce7a43e0c91 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp @@ -26,20 +26,44 @@ TurboModuleManager::TurboModuleManager( RuntimeExecutor runtimeExecutor, std::shared_ptr jsCallInvoker, std::shared_ptr nativeCallInvoker, - jni::alias_ref delegate) + jni::alias_ref delegate, + bool useGlobalCallbackCleanupScopeUsingRetainJSCallback, + bool useTurboModuleManagerCallbackCleanupScope) : javaPart_(jni::make_global(jThis)), runtimeExecutor_(runtimeExecutor), jsCallInvoker_(jsCallInvoker), nativeCallInvoker_(nativeCallInvoker), delegate_(jni::make_global(delegate)), - turboModuleCache_(std::make_shared()) {} + turboModuleCache_(std::make_shared()) { + if (useGlobalCallbackCleanupScopeUsingRetainJSCallback) { + longLivedObjectCollection_ = nullptr; + retainJSCallback_ = [](jsi::Function &&callback, + jsi::Runtime &runtime, + std::shared_ptr jsInvoker) { + return CallbackWrapper::createWeak( + std::move(callback), runtime, jsInvoker); + }; + } else if (useTurboModuleManagerCallbackCleanupScope) { + longLivedObjectCollection_ = std::make_shared(); + retainJSCallback_ = [longLivedObjectCollection = + longLivedObjectCollection_]( + jsi::Function &&callback, + jsi::Runtime &runtime, + std::shared_ptr jsInvoker) { + return CallbackWrapper::createWeak( + longLivedObjectCollection, std::move(callback), runtime, jsInvoker); + }; + } +} jni::local_ref TurboModuleManager::initHybrid( jni::alias_ref jThis, jni::alias_ref runtimeExecutor, jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, - jni::alias_ref delegate) { + jni::alias_ref delegate, + bool useGlobalCallbackCleanupScopeUsingRetainJSCallback, + bool useTurboModuleManagerCallbackCleanupScope) { auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker(); auto nativeCallInvoker = nativeCallInvokerHolder->cthis()->getCallInvoker(); @@ -48,7 +72,9 @@ jni::local_ref TurboModuleManager::initHybrid( runtimeExecutor->cthis()->get(), jsCallInvoker, nativeCallInvoker, - delegate); + delegate, + useGlobalCallbackCleanupScopeUsingRetainJSCallback, + useTurboModuleManagerCallbackCleanupScope); } void TurboModuleManager::registerNatives() { @@ -70,7 +96,8 @@ void TurboModuleManager::installJSIBindings() { jsCallInvoker_ = std::weak_ptr(jsCallInvoker_), nativeCallInvoker_ = std::weak_ptr(nativeCallInvoker_), delegate_ = jni::make_weak(delegate_), - javaPart_ = jni::make_weak(javaPart_)]( + javaPart_ = jni::make_weak(javaPart_), + retainJSCallback = retainJSCallback_]( const std::string &name) -> std::shared_ptr { auto turboModuleCache = turboModuleCache_.lock(); auto jsCallInvoker = jsCallInvoker_.lock(); @@ -131,7 +158,8 @@ void TurboModuleManager::installJSIBindings() { .moduleName = name, .instance = moduleInstance, .jsInvoker = jsCallInvoker, - .nativeInvoker = nativeCallInvoker}; + .nativeInvoker = nativeCallInvoker, + .retainJSCallback = retainJSCallback}; auto turboModule = delegate->cthis()->getTurboModule(name, params); turboModuleCache->insert({name, turboModule}); @@ -142,7 +170,12 @@ void TurboModuleManager::installJSIBindings() { return nullptr; }; - TurboModuleBinding::install(runtime, std::move(turboModuleProvider)); + if (longLivedObjectCollection_) { + TurboModuleBinding::install( + runtime, std::move(turboModuleProvider), longLivedObjectCollection_); + } else { + TurboModuleBinding::install(runtime, std::move(turboModuleProvider)); + } }); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h index a8b05fda4d5..6b8fd0669b8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -32,7 +33,9 @@ class TurboModuleManager : public jni::HybridClass { jni::alias_ref runtimeExecutor, jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, - jni::alias_ref delegate); + jni::alias_ref delegate, + bool useGlobalCallbackCleanupScopeUsingRetainJSCallback, + bool useTurboModuleManagerCallbackCleanupScope); static void registerNatives(); private: @@ -43,6 +46,9 @@ class TurboModuleManager : public jni::HybridClass { std::shared_ptr nativeCallInvoker_; jni::global_ref delegate_; + JSCallbackRetainer retainJSCallback_; + std::shared_ptr longLivedObjectCollection_; + using TurboModuleCache = std::unordered_map>; @@ -60,7 +66,9 @@ class TurboModuleManager : public jni::HybridClass { RuntimeExecutor runtimeExecutor, std::shared_ptr jsCallInvoker, std::shared_ptr nativeCallInvoker, - jni::alias_ref delegate); + jni::alias_ref delegate, + bool useGlobalCallbackCleanupScopeUsingRetainJSCallback, + bool useTurboModuleManagerCallbackCleanupScope); }; } // namespace react diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp index 1942bbfeac8..18abcc3085b 100644 --- a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp +++ b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp @@ -30,7 +30,8 @@ namespace react { JavaTurboModule::JavaTurboModule(const InitParams ¶ms) : TurboModule(params.moduleName, params.jsInvoker), instance_(jni::make_global(params.instance)), - nativeInvoker_(params.nativeInvoker) {} + nativeInvoker_(params.nativeInvoker), + retainJSCallback_(params.retainJSCallback) {} JavaTurboModule::~JavaTurboModule() { /** @@ -54,11 +55,13 @@ JavaTurboModule::~JavaTurboModule() { namespace { jni::local_ref createJavaCallbackFromJSIFunction( + JSCallbackRetainer retainJSCallback, jsi::Function &&function, jsi::Runtime &rt, std::shared_ptr jsInvoker) { - auto weakWrapper = - react::CallbackWrapper::createWeak(std::move(function), rt, jsInvoker); + auto weakWrapper = retainJSCallback != nullptr + ? retainJSCallback(std::move(function), rt, jsInvoker) + : react::CallbackWrapper::createWeak(std::move(function), rt, jsInvoker); // This needs to be a shared_ptr because: // 1. It cannot be unique_ptr. std::function is copyable but unique_ptr is @@ -251,7 +254,8 @@ JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs( const jsi::Value *args, size_t count, std::shared_ptr jsInvoker, - TurboModuleMethodValueKind valueKind) { + TurboModuleMethodValueKind valueKind, + JSCallbackRetainer retainJSCallback) { unsigned int expectedArgumentCount = valueKind == PromiseKind ? methodArgTypes.size() - 1 : methodArgTypes.size(); @@ -386,7 +390,8 @@ JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs( jsi::Function fn = arg->getObject(rt).getFunction(rt); jarg->l = makeGlobalIfNecessary( - createJavaCallbackFromJSIFunction(std::move(fn), rt, jsInvoker) + createJavaCallbackFromJSIFunction( + retainJSCallback, std::move(fn), rt, jsInvoker) .release()); continue; } @@ -533,7 +538,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod( args, argCount, jsInvoker_, - valueKind); + valueKind, + retainJSCallback_); if (isMethodSync && valueKind != PromiseKind) { TMPL::syncMethodCallArgConversionEnd(moduleName, methodName); @@ -744,7 +750,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod( methodID, moduleNameStr = name_, methodNameStr, - env]( + env, + retainJSCallback = retainJSCallback_]( jsi::Runtime &runtime, const jsi::Value &thisVal, const jsi::Value *promiseConstructorArgs, @@ -761,10 +768,16 @@ jsi::Value JavaTurboModule::invokeJavaMethod( runtime); auto resolve = createJavaCallbackFromJSIFunction( - std::move(resolveJSIFn), runtime, jsInvoker_) + retainJSCallback, + std::move(resolveJSIFn), + runtime, + jsInvoker_) .release(); auto reject = createJavaCallbackFromJSIFunction( - std::move(rejectJSIFn), runtime, jsInvoker_) + retainJSCallback, + std::move(rejectJSIFn), + runtime, + jsInvoker_) .release(); jclass jPromiseImpl = diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h index 77a067701fc..4c597584476 100644 --- a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h +++ b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h @@ -7,9 +7,11 @@ #pragma once +#include #include #include +#include #include #include #include @@ -30,6 +32,11 @@ struct JTurboModule : jni::JavaClass { "Lcom/facebook/react/turbomodule/core/interfaces/TurboModule;"; }; +using JSCallbackRetainer = std::function( + jsi::Function &&callback, + jsi::Runtime &runtime, + std::shared_ptr jsInvoker)>; + class JSI_EXPORT JavaTurboModule : public TurboModule { public: // TODO(T65603471): Should we unify this with a Fabric abstraction? @@ -38,6 +45,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { jni::alias_ref instance; std::shared_ptr jsInvoker; std::shared_ptr nativeInvoker; + JSCallbackRetainer retainJSCallback; }; JavaTurboModule(const InitParams ¶ms); @@ -54,6 +62,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { private: jni::global_ref instance_; std::shared_ptr nativeInvoker_; + JSCallbackRetainer retainJSCallback_; JNIArgs convertJSIArgsToJNIArgs( JNIEnv *env, @@ -63,7 +72,8 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { const jsi::Value *args, size_t count, std::shared_ptr jsInvoker, - TurboModuleMethodValueKind valueKind); + TurboModuleMethodValueKind valueKind, + JSCallbackRetainer retainJSCallbacks); }; } // namespace react