Skip to content

Commit

Permalink
Ship bridge RuntimeExecutor JSIExecutor flushing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
RSNara authored and Setito committed Jul 17, 2021
1 parent 50e505d commit 10b67f7
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<JSIModuleSpec> jsiModules) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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++
Expand Down Expand Up @@ -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 */
Expand All @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,10 @@ CatalystInstanceImpl::getNativeCallInvokerHolder() {
}

jni::alias_ref<JRuntimeExecutor::javaobject>
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_;
}
Expand Down
3 changes: 1 addition & 2 deletions ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ class CatalystInstanceImpl : public jni::HybridClass<CatalystInstanceImpl> {
void jniCallJSCallback(jint callbackId, NativeArray *arguments);
jni::alias_ref<CallInvokerHolder::javaobject> getJSCallInvokerHolder();
jni::alias_ref<CallInvokerHolder::javaobject> getNativeCallInvokerHolder();
jni::alias_ref<JRuntimeExecutor::javaobject> getRuntimeExecutor(
bool shouldFlush);
jni::alias_ref<JRuntimeExecutor::javaobject> getRuntimeExecutor();
void setGlobalVariable(std::string propName, std::string &&jsonValue);
jlong getJavaScriptContext();
void handleMemoryPressure(int pressureLevel);
Expand Down
13 changes: 5 additions & 8 deletions ReactCommon/cxxreact/Instance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,23 +246,20 @@ std::shared_ptr<CallInvoker> Instance::getJSCallInvoker() {
return std::static_pointer_cast<CallInvoker>(jsCallInvoker_);
}

RuntimeExecutor Instance::getRuntimeExecutor(bool shouldFlush) {
RuntimeExecutor Instance::getRuntimeExecutor() {
std::weak_ptr<NativeToJsBridge> weakNativeToJsBridge = nativeToJsBridge_;

auto runtimeExecutor =
[weakNativeToJsBridge,
shouldFlush](std::function<void(jsi::Runtime & runtime)> &&callback) {
[weakNativeToJsBridge](
std::function<void(jsi::Runtime & runtime)> &&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);
}
Expand Down
2 changes: 1 addition & 1 deletion ReactCommon/cxxreact/Instance.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 10b67f7

Please sign in to comment.