From 281daf12222d3b5d92120b654d421e711d48fd0a Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Wed, 2 Jun 2021 16:53:27 -0700 Subject: [PATCH] Ship bridge RuntimeExecutor JSIExecutor flushing Summary: The RuntimeExecutor that Fabric gets from the bridge doesn't call JSIExecutor::flush(). In the legacy NativeModule system, we're supposed to flush the queue of NativeModule calls after every call into JavaScript. The lack of this flushing means that we execute NativeModule calls less frequently with Fabric enabled, and TurboModules disabled. It also means that [the microtask checkpoints we placed inside JSIExecutor::flush()](https://www.internalfb.com/code/fbsource/[62f69606ae81530f7d6f0cba8466ac604934c901]/xplat/js/react-native-github/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp?lines=427%2C445) won't be executed as frequently, with Fabric enabled. Changelog: [Android][Fixed] - Flush NativeModule calls with Fabric on Android, on every Native -> JS call. Reviewed By: JoshuaGross, mdvacca Differential Revision: D28620982 fbshipit-source-id: ae4d1c16c62b6d4a5089e63104ad97f4ed44c440 --- .../react/bridge/CatalystInstanceImpl.java | 7 +------ .../react/config/ReactFeatureFlags.java | 21 ------------------- .../jni/react/jni/CatalystInstanceImpl.cpp | 6 +++--- .../main/jni/react/jni/CatalystInstanceImpl.h | 3 +-- ReactCommon/cxxreact/Instance.cpp | 13 +++++------- ReactCommon/cxxreact/Instance.h | 2 +- 6 files changed, 11 insertions(+), 41 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 c0d8d225e31243..2691689cf7d10b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -549,12 +549,7 @@ public JavaScriptContextHolder getJavaScriptContextHolder() { return mJavaScriptContextHolder; } - @Override - public RuntimeExecutor getRuntimeExecutor() { - return getRuntimeExecutor(ReactFeatureFlags.enableRuntimeExecutorFlushing()); - } - - public native RuntimeExecutor getRuntimeExecutor(boolean shouldFlush); + public native RuntimeExecutor getRuntimeExecutor(); @Override public void addJSIModules(List jsiModules) { 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 9af8f0b54b1284..6c7f3e038cf2a6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -18,12 +18,6 @@ */ @DoNotStripAny public class ReactFeatureFlags { - - /** An interface used to compute flags on demand. */ - public interface FlagProvider { - boolean get(); - } - /** * Should this application use TurboModules? If yes, then any module that inherits {@link * com.facebook.react.turbomodule.core.interfaces.TurboModule} will NOT be passed in to C++ @@ -59,13 +53,6 @@ public interface FlagProvider { /** Feature flag to configure eager initialization of MapBuffer So file */ public static boolean enableEagerInitializeMapBufferSoFile = false; - /** Should the RuntimeExecutor call JSIExecutor::flush()? */ - private static FlagProvider enableRuntimeExecutorFlushingProvider = null; - - public static void setEnableRuntimeExecutorFlushingFlagProvider(FlagProvider provider) { - enableRuntimeExecutorFlushingProvider = provider; - } - private static boolean mapBufferSerializationEnabled = false; /** Enables or disables MapBuffer Serialization */ @@ -77,14 +64,6 @@ public static boolean isMapBufferSerializationEnabled() { return mapBufferSerializationEnabled; } - public static boolean enableRuntimeExecutorFlushing() { - if (enableRuntimeExecutorFlushingProvider != null) { - return enableRuntimeExecutorFlushingProvider.get(); - } - - return false; - } - /** Enables Fabric for LogBox */ public static boolean enableFabricInLogBox = false; diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 6027a63f49f15e..2a33fa3d0f8294 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(bool shouldFlush) { +CatalystInstanceImpl::getRuntimeExecutor() { if (!runtimeExecutor_) { - runtimeExecutor_ = jni::make_global(JRuntimeExecutor::newObjectCxxArgs( - instance_->getRuntimeExecutor(shouldFlush))); + runtimeExecutor_ = jni::make_global( + JRuntimeExecutor::newObjectCxxArgs(instance_->getRuntimeExecutor())); } return runtimeExecutor_; } diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index fff64a0a79db6f..c40a691232c6a2 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -93,8 +93,7 @@ class CatalystInstanceImpl : public jni::HybridClass { void jniCallJSCallback(jint callbackId, NativeArray *arguments); jni::alias_ref getJSCallInvokerHolder(); jni::alias_ref getNativeCallInvokerHolder(); - jni::alias_ref getRuntimeExecutor( - bool shouldFlush); + jni::alias_ref getRuntimeExecutor(); 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 13679112bbe257..f1150821a9659a 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -246,23 +246,20 @@ std::shared_ptr Instance::getJSCallInvoker() { return std::static_pointer_cast(jsCallInvoker_); } -RuntimeExecutor Instance::getRuntimeExecutor(bool shouldFlush) { +RuntimeExecutor Instance::getRuntimeExecutor() { std::weak_ptr weakNativeToJsBridge = nativeToJsBridge_; auto runtimeExecutor = - [weakNativeToJsBridge, - shouldFlush](std::function &&callback) { + [weakNativeToJsBridge]( + std::function &&callback) { if (auto strongNativeToJsBridge = weakNativeToJsBridge.lock()) { strongNativeToJsBridge->runOnExecutorQueue( - [callback = std::move(callback), - shouldFlush](JSExecutor *executor) { + [callback = std::move(callback)](JSExecutor *executor) { jsi::Runtime *runtime = (jsi::Runtime *)executor->getJavaScriptContext(); try { callback(*runtime); - if (shouldFlush) { - executor->flush(); - } + executor->flush(); } catch (jsi::JSError &originalError) { handleJSError(*runtime, originalError, true); } diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index dfde400a01d9e8..7cd11f9bdf50b4 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(bool shouldFlush); + RuntimeExecutor getRuntimeExecutor(); private: void callNativeModules(folly::dynamic &&calls, bool isEndOfBatch);