From c0ec82e61eb464b38b912ce54f2678e308ad8d10 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 23 Apr 2021 18:14:35 -0700 Subject: [PATCH] Add flushing to RuntimeExecutor Summary: ## Motivation With the bridge, every call into JS flushes the queue of NativeModule calls. Fabric bypasses this mechanism, because it uses a RuntimeExecutor that schedules work directly on the JavaScript thread. This diff makes Fabric's RuntimeExecutor also flush the queue of NativeModule calls. This likely won't fix anything in Fabric, because we don't execute any async NativeModule calls on Fb4a. However, this is necessary for the drainMicrotask work we're doing in D27729702 (https://github.com/facebook/react-native/commit/73108477589a18cecb303ef556fa3da02f8ca1b8), because (1) we need to drain the Hermes microtask queue on every call from C++ -> JavaScript (2) we drain the microtask queue [inside JSIExecutor::flush()](https://github.com/facebook/react-native/blob/de477a0df6da770e579892d4875a8995c430ebdf/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp#L427,L445). Changelog: [Android][Fixed] - Flush JSIExecutor in Fabric's RuntimeExecutor Reviewed By: JoshuaGross Differential Revision: D27975839 fbshipit-source-id: 27f031fb36593253da116a033e30998475eb1473 --- .../facebook/react/bridge/CatalystInstanceImpl.java | 6 +++++- .../src/main/jni/react/jni/CatalystInstanceImpl.cpp | 6 +++--- .../src/main/jni/react/jni/CatalystInstanceImpl.h | 3 ++- ReactCommon/cxxreact/Instance.cpp | 13 ++++++++----- ReactCommon/cxxreact/Instance.h | 2 +- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index cdc1eb26a36..a75c7d39100 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -550,7 +550,11 @@ public class CatalystInstanceImpl implements CatalystInstance { } @Override - public native RuntimeExecutor getRuntimeExecutor(); + public RuntimeExecutor getRuntimeExecutor() { + return getRuntimeExecutor(false); + } + + public native RuntimeExecutor getRuntimeExecutor(boolean shouldFlush); @Override public void addJSIModules(List jsiModules) { diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 2a33fa3d0f8..6027a63f49f 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -346,10 +346,10 @@ CatalystInstanceImpl::getNativeCallInvokerHolder() { } jni::alias_ref -CatalystInstanceImpl::getRuntimeExecutor() { +CatalystInstanceImpl::getRuntimeExecutor(bool shouldFlush) { if (!runtimeExecutor_) { - runtimeExecutor_ = jni::make_global( - JRuntimeExecutor::newObjectCxxArgs(instance_->getRuntimeExecutor())); + runtimeExecutor_ = jni::make_global(JRuntimeExecutor::newObjectCxxArgs( + instance_->getRuntimeExecutor(shouldFlush))); } return runtimeExecutor_; } diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index c40a691232c..fff64a0a79d 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -93,7 +93,8 @@ class CatalystInstanceImpl : public jni::HybridClass { void jniCallJSCallback(jint callbackId, NativeArray *arguments); jni::alias_ref getJSCallInvokerHolder(); jni::alias_ref getNativeCallInvokerHolder(); - jni::alias_ref getRuntimeExecutor(); + jni::alias_ref getRuntimeExecutor( + bool shouldFlush); void setGlobalVariable(std::string propName, std::string &&jsonValue); jlong getJavaScriptContext(); void handleMemoryPressure(int pressureLevel); diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index 8e837596f71..13679112bbe 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -246,20 +246,23 @@ std::shared_ptr Instance::getJSCallInvoker() { return std::static_pointer_cast(jsCallInvoker_); } -// TODO: Unify with JS CallInvoker -RuntimeExecutor Instance::getRuntimeExecutor() { +RuntimeExecutor Instance::getRuntimeExecutor(bool shouldFlush) { std::weak_ptr weakNativeToJsBridge = nativeToJsBridge_; auto runtimeExecutor = - [weakNativeToJsBridge]( - std::function &&callback) { + [weakNativeToJsBridge, + shouldFlush](std::function &&callback) { if (auto strongNativeToJsBridge = weakNativeToJsBridge.lock()) { strongNativeToJsBridge->runOnExecutorQueue( - [callback = std::move(callback)](JSExecutor *executor) { + [callback = std::move(callback), + shouldFlush](JSExecutor *executor) { jsi::Runtime *runtime = (jsi::Runtime *)executor->getJavaScriptContext(); try { callback(*runtime); + if (shouldFlush) { + executor->flush(); + } } catch (jsi::JSError &originalError) { handleJSError(*runtime, originalError, true); } diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index 7cd11f9bdf5..dfde400a01d 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -134,7 +134,7 @@ class RN_EXPORT Instance { /** * RuntimeExecutor is used by Fabric to access the jsi::Runtime. */ - RuntimeExecutor getRuntimeExecutor(); + RuntimeExecutor getRuntimeExecutor(bool shouldFlush); private: void callNativeModules(folly::dynamic &&calls, bool isEndOfBatch);