From 5c4f145e33d92969f8a86284360a5a2f09308500 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Thu, 17 Dec 2020 17:22:11 -0800 Subject: [PATCH] Roll out TurboModule Promise Async Dispatch Summary: ## Context The legacy NativeModule infra implements promise methods using [async NativeModule method calls](https://fburl.com/diffusion/tpkff6vg). In the TurboModule infra, promise methods [treat as sync methods](https://fburl.com/diffusion/yde7xw71), and executed directly on the JavaScript thread. This experiment makes TurboModule promise methods async, and dispatches them to the NativeModules thread, when they're executed from JavaScript. Reviewed By: fkgozali Differential Revision: D25623192 fbshipit-source-id: 2b50d771c5272af3b6edf150054bb3e80cab0040 --- .../turbomodule/core/TurboModuleManager.java | 2 - .../jni/ReactCommon/TurboModuleManager.cpp | 3 - .../core/jni/ReactCommon/TurboModuleManager.h | 1 - .../android/ReactCommon/JavaTurboModule.cpp | 104 +++++++----------- .../android/ReactCommon/JavaTurboModule.h | 6 - 5 files changed, 40 insertions(+), 76 deletions(-) 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 000770aec1d..2d254d69ec4 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 @@ -61,7 +61,6 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { (CallInvokerHolderImpl) jsCallInvokerHolder, (CallInvokerHolderImpl) nativeCallInvokerHolder, delegate, - ReactFeatureFlags.enableTurboModulePromiseAsyncDispatch, ReactFeatureFlags.useTurboModuleJSCodegen); installJSIBindings(); @@ -295,7 +294,6 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { CallInvokerHolderImpl jsCallInvokerHolder, CallInvokerHolderImpl nativeCallInvokerHolder, TurboModuleManagerDelegate tmmDelegate, - boolean enablePromiseAsyncDispatch, boolean enableTurboModuleJSCodegen); 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 a6d90b615d8..7a69cc6dc4a 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 @@ -43,13 +43,10 @@ jni::local_ref TurboModuleManager::initHybrid( jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, jni::alias_ref delegate, - bool enablePromiseAsyncDispatch, bool enableJSCodegen) { auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker(); auto nativeCallInvoker = nativeCallInvokerHolder->cthis()->getCallInvoker(); - JavaTurboModule::enablePromiseAsyncDispatch(enablePromiseAsyncDispatch); - return makeCxxInstance( jThis, runtimeExecutor->cthis()->get(), 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 92716800d24..384c5952d68 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 @@ -33,7 +33,6 @@ class TurboModuleManager : public jni::HybridClass { jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, jni::alias_ref delegate, - bool enablePromiseAsyncDispatch, bool enableJSCodegen); static void registerNatives(); diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp index f7af1f517f5..25fb83470bf 100644 --- a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp +++ b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp @@ -51,11 +51,6 @@ JavaTurboModule::~JavaTurboModule() { }); } -bool JavaTurboModule::isPromiseAsyncDispatchEnabled_ = false; -void JavaTurboModule::enablePromiseAsyncDispatch(bool enable) { - isPromiseAsyncDispatchEnabled_ = enable; -} - JavaTurboModule::JavaTurboModule( const InitParams ¶ms, TurboModuleSchema &&schema) @@ -322,8 +317,7 @@ JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs( auto makeGlobalIfNecessary = [&globalRefs, env, valueKind](jobject obj) -> jobject { - if (valueKind == VoidKind || - (valueKind == PromiseKind && isPromiseAsyncDispatchEnabled_)) { + if (valueKind == VoidKind || valueKind == PromiseKind) { jobject globalObj = env->NewGlobalRef(obj); globalRefs.push_back(globalObj); env->DeleteLocalRef(obj); @@ -491,9 +485,7 @@ jsi::Value JavaTurboModule::invokeJavaMethod( const char *methodName = methodNameStr.c_str(); const char *moduleName = name_.c_str(); - bool isMethodSync = - !(valueKind == VoidKind || - (valueKind == PromiseKind && isPromiseAsyncDispatchEnabled_)); + bool isMethodSync = !(valueKind == VoidKind || valueKind == PromiseKind); if (isMethodSync) { TMPL::syncMethodCallStart(moduleName, methodName); @@ -834,60 +826,48 @@ jsi::Value JavaTurboModule::invokeJavaMethod( const char *moduleName = moduleNameStr.c_str(); const char *methodName = methodNameStr.c_str(); - if (isPromiseAsyncDispatchEnabled_) { - jobject globalPromise = env->NewGlobalRef(promise); + jobject globalPromise = env->NewGlobalRef(promise); - globalRefs.push_back(globalPromise); - env->DeleteLocalRef(promise); + globalRefs.push_back(globalPromise); + env->DeleteLocalRef(promise); - jargs[argCount].l = globalPromise; - TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName); - TMPL::asyncMethodCallDispatch(moduleName, methodName); + jargs[argCount].l = globalPromise; + TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName); + TMPL::asyncMethodCallDispatch(moduleName, methodName); - nativeInvoker_->invokeAsync( - [jargs, - globalRefs, - methodID, - instance_ = instance_, - moduleNameStr, - methodNameStr, - id = getUniqueId()]() mutable -> void { - /** - * TODO(ramanpreet): Why do we have to require the - * environment again? Why does JNI crash when we use the env - * from the upper scope? - */ - JNIEnv *env = jni::Environment::current(); - const char *moduleName = moduleNameStr.c_str(); - const char *methodName = methodNameStr.c_str(); + nativeInvoker_->invokeAsync( + [jargs, + globalRefs, + methodID, + instance_ = instance_, + moduleNameStr, + methodNameStr, + id = getUniqueId()]() mutable -> void { + /** + * TODO(ramanpreet): Why do we have to require the + * environment again? Why does JNI crash when we use the env + * from the upper scope? + */ + JNIEnv *env = jni::Environment::current(); + const char *moduleName = moduleNameStr.c_str(); + const char *methodName = methodNameStr.c_str(); - TMPL::asyncMethodCallExecutionStart( + TMPL::asyncMethodCallExecutionStart( + moduleName, methodName, id); + env->CallVoidMethodA(instance_.get(), methodID, jargs.data()); + try { + FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); + } catch (...) { + TMPL::asyncMethodCallExecutionFail( moduleName, methodName, id); - env->CallVoidMethodA( - instance_.get(), methodID, jargs.data()); - try { - FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); - } catch (...) { - TMPL::asyncMethodCallExecutionFail( - moduleName, methodName, id); - throw; - } + throw; + } - for (auto globalRef : globalRefs) { - env->DeleteGlobalRef(globalRef); - } - TMPL::asyncMethodCallExecutionEnd( - moduleName, methodName, id); - }); - - } else { - jargs[argCount].l = promise; - TMPL::syncMethodCallArgConversionEnd(moduleName, methodName); - TMPL::syncMethodCallExecutionStart(moduleName, methodName); - env->CallVoidMethodA(instance_.get(), methodID, jargs.data()); - TMPL::syncMethodCallExecutionEnd(moduleName, methodName); - TMPL::syncMethodCallReturnConversionStart(moduleName, methodName); - } + for (auto globalRef : globalRefs) { + env->DeleteGlobalRef(globalRef); + } + TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id); + }); return jsi::Value::undefined(); }); @@ -896,12 +876,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod( Promise.callAsConstructor(runtime, promiseConstructorArg); checkJNIErrorForMethodCall(); - if (isPromiseAsyncDispatchEnabled_) { - TMPL::asyncMethodCallEnd(moduleName, methodName); - } else { - TMPL::syncMethodCallReturnConversionEnd(moduleName, methodName); - TMPL::syncMethodCallEnd(moduleName, methodName); - } + TMPL::asyncMethodCallEnd(moduleName, methodName); + return promise; } default: diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h index 2e15fea7176..259924a16a1 100644 --- a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h +++ b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h @@ -55,7 +55,6 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { const jsi::Value *args, size_t argCount); - static void enablePromiseAsyncDispatch(bool enable); jsi::Value get(jsi::Runtime &runtime, const jsi::PropNameID &propName) override; @@ -64,11 +63,6 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { std::shared_ptr nativeInvoker_; folly::Optional turboModuleSchema_; - /** - * Experiments - */ - static bool isPromiseAsyncDispatchEnabled_; - JNIArgs convertJSIArgsToJNIArgs( JNIEnv *env, jsi::Runtime &rt,