From 689233b018bd533a7eecd38e38a7fb84b849cf88 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 20 Sep 2019 10:48:50 -0700 Subject: [PATCH] Implement async method dispatch Summary: Now that all the plumbing is done, this diff finally implements async method dispatch on the NativeModule thread. Reviewed By: mdvacca Differential Revision: D17480605 fbshipit-source-id: 992aab99954c488a0327144d84a1668a2b158d04 --- .../core/platform/android/JavaTurboModule.cpp | 73 +++++++++++++++---- .../core/platform/android/JavaTurboModule.h | 8 +- 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp index 7573893de9d..2f931adc0d3 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp @@ -219,7 +219,7 @@ std::vector getMethodArgTypesFromSignature( // needs to be done again // TODO (axe) Reuse existing implementation as needed - the exist in // MethodInvoker.cpp -std::vector JavaTurboModule::convertJSIArgsToJNIArgs( +JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs( JNIEnv *env, jsi::Runtime &rt, std::string methodName, @@ -237,8 +237,21 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( methodName, count, expectedArgumentCount); } - auto jargs = - std::vector(valueKind == PromiseKind ? count + 1 : count); + JNIArgs jniArgs(valueKind == PromiseKind ? count + 1 : count); + auto &jargs = jniArgs.args_; + auto &globalRefs = jniArgs.globalRefs_; + + auto makeGlobalIfNecessary = + [&globalRefs, env, valueKind](jobject obj) -> jobject { + if (valueKind == VoidKind) { + jobject globalObj = env->NewGlobalRef(obj); + globalRefs.push_back(globalObj); + env->DeleteLocalRef(obj); + return globalObj; + } + + return obj; + }; jclass booleanClass = nullptr; jclass doubleClass = nullptr; @@ -295,8 +308,8 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( jmethodID doubleConstructor = env->GetMethodID(doubleClass, "", "(D)V"); - jarg->l = - env->NewObject(doubleClass, doubleConstructor, arg->getNumber()); + jarg->l = makeGlobalIfNecessary( + env->NewObject(doubleClass, doubleConstructor, arg->getNumber())); continue; } @@ -312,8 +325,8 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( jmethodID booleanConstructor = env->GetMethodID(booleanClass, "", "(Z)V"); - jarg->l = - env->NewObject(booleanClass, booleanConstructor, arg->getBool()); + jarg->l = makeGlobalIfNecessary( + env->NewObject(booleanClass, booleanConstructor, arg->getBool())); continue; } @@ -323,7 +336,8 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( "string", argIndex, methodName, arg, &rt); } - jarg->l = env->NewStringUTF(arg->getString(rt).utf8(rt).c_str()); + jarg->l = makeGlobalIfNecessary( + env->NewStringUTF(arg->getString(rt).utf8(rt).c_str())); continue; } @@ -336,7 +350,7 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( auto dynamicFromValue = jsi::dynamicFromValue(rt, *arg); auto jParams = ReadableNativeArray::newObjectCxxArgs(std::move(dynamicFromValue)); - jarg->l = jParams.release(); + jarg->l = makeGlobalIfNecessary(jParams.release()); continue; } @@ -347,7 +361,8 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( } jsi::Function fn = arg->getObject(rt).getFunction(rt); - jarg->l = createJavaCallbackFromJSIFunction(fn, rt, jsInvoker).release(); + jarg->l = makeGlobalIfNecessary( + createJavaCallbackFromJSIFunction(fn, rt, jsInvoker).release()); continue; } @@ -360,12 +375,12 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( auto dynamicFromValue = jsi::dynamicFromValue(rt, *arg); auto jParams = ReadableNativeMap::createWithContents(std::move(dynamicFromValue)); - jarg->l = jParams.release(); + jarg->l = makeGlobalIfNecessary(jParams.release()); continue; } } - return jargs; + return jniArgs; } jsi::Value convertFromJMapToValue(JNIEnv *env, jsi::Runtime &rt, jobject arg) { @@ -404,7 +419,15 @@ jsi::Value JavaTurboModule::invokeJavaMethod( * object. For normal returns, we just create the return object. */ unsigned int maxReturnObjects = 3; - unsigned int estimatedLocalRefCount = argCount + maxReturnObjects + buffer; + + /** + * When the return type is void, all JNI LocalReferences are converted to + * GlobalReferences. The LocalReferences are then promptly deleted + * after the conversion. + */ + unsigned int actualArgCount = valueKind == VoidKind ? 0 : argCount; + unsigned int estimatedLocalRefCount = + actualArgCount + maxReturnObjects + buffer; /** * This will push a new JNI stack frame for the LocalReferences in this @@ -438,7 +461,7 @@ jsi::Value JavaTurboModule::invokeJavaMethod( std::vector methodArgTypes = getMethodArgTypesFromSignature(methodSignature); - std::vector jargs = convertJSIArgsToJNIArgs( + JNIArgs jniArgs = convertJSIArgsToJNIArgs( env, runtime, methodName, @@ -448,10 +471,28 @@ jsi::Value JavaTurboModule::invokeJavaMethod( jsInvoker_, valueKind); + auto &jargs = jniArgs.args_; + auto &globalRefs = jniArgs.globalRefs_; + switch (valueKind) { case VoidKind: { - env->CallVoidMethodA(instance, methodID, jargs.data()); - FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); + nativeInvoker_->invokeAsync( + [jargs, globalRefs, methodID, instance_ = instance_]() 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(); + + env->CallVoidMethodA(instance_.get(), methodID, jargs.data()); + FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); + + for (auto globalRef : globalRefs) { + env->DeleteGlobalRef(globalRef); + } + }); return jsi::Value::undefined(); } diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h index a4a970e46c5..896a1c39bc1 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h @@ -19,6 +19,12 @@ namespace facebook { namespace react { +struct JNIArgs { + JNIArgs(size_t count) : args_(count) {} + std::vector args_; + std::vector globalRefs_; +}; + struct JTurboModule : jni::JavaClass { static auto constexpr kJavaDescriptor = "Lcom/facebook/react/turbomodule/core/interfaces/TurboModule;"; @@ -59,7 +65,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { jsi::Function &function, jsi::Runtime &rt, std::shared_ptr jsInvoker); - std::vector convertJSIArgsToJNIArgs( + JNIArgs convertJSIArgsToJNIArgs( JNIEnv *env, jsi::Runtime &rt, std::string methodName,