Skip to content

Commit

Permalink
Execute sync methods on JS thread
Browse files Browse the repository at this point in the history
Summary:
There are two ways to make a NativeModule method execute synchronously:
- Declare the NativeModule method to be synchronous (i.e: use `RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD`).
- Make the NativeModule synchronous (i.e: make its method queue `RCTJSThread`). This way, all its methods are synchronous.

`RCTNativeModule` executes all synchronous methods on the JS thread:
- Executing an async methods on a sync module: https://git.io/JfRPj
- Executing a sync method: https://git.io/JfRXe

However, in TurboModules we block the JS thread, execute the method on the NativeModule's method queue, and then unblock the JS thread. While this approach is thread-safe, and arguably the correct way to dispatch sync methods, it's also much slower than the alternative. Therefore, this diff migrates the legacy behaviour to the TurboModule system.

## Special Case: getConstants()
When an ObjC NativeModule requires main queue setup, and it exports constants, we execute its `constantsToExport` method on the main queue (see: [RCTModuleData gatherConstants](https://github.com/facebook/react-native/blob/c8d678abcf93fd3f6daf4bebfdf25937995c1fdf/React/Base/RCTModuleData.mm#L392-L402)). I replicated this behaviour with TurboModules.

Changelog:
[iOS][Fixed] - Execute ObjC TurboModule async method calls on JS thread for sync modules

Reviewed By: fkgozali

Differential Revision: D21602096

fbshipit-source-id: 42d07b7ad000abeac27091dc3ec440e3836d2eae
  • Loading branch information
RSNara authored and facebook-github-bot committed Jun 3, 2020
1 parent d7ac21c commit 976f51a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 7 deletions.
12 changes: 6 additions & 6 deletions ReactCommon/turbomodule/core/platform/ios/RCTTurboModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ class JSI_EXPORT ObjCTurboModule : public TurboModule {
id<RCTTurboModule> instance;
std::shared_ptr<CallInvoker> jsInvoker;
std::shared_ptr<CallInvoker> nativeInvoker;
// Does the NativeModule dispatch async methods to the JS thread?
bool isSyncModule;
};

Expand All @@ -61,16 +60,20 @@ class JSI_EXPORT ObjCTurboModule : public TurboModule {
void setMethodArgConversionSelector(NSString *methodName, int argIndex, NSString *fnName);

private:
// Does the NativeModule dispatch async methods to the JS thread?
const bool isSyncModule_;

/**
* TODO(ramanpreet):
* Investigate an optimization that'll let us get rid of this NSMutableDictionary.
*/
NSMutableDictionary<NSString *, NSMutableArray *> *methodArgConversionSelectors_;
NSDictionary<NSString *, NSArray<NSString *> *> *methodArgumentTypeNames_;
const bool isSyncModule_;

bool isMethodSync(TurboModuleMethodValueKind returnType);
BOOL hasMethodArgConversionSelector(NSString *methodName, int argIndex);
SEL getMethodArgConversionSelector(NSString *methodName, int argIndex);
NSString *getArgumentTypeName(NSString *methodName, int argIndex);

NSInvocation *getMethodInvocation(
jsi::Runtime &runtime,
TurboModuleMethodValueKind returnType,
Expand All @@ -86,9 +89,6 @@ class JSI_EXPORT ObjCTurboModule : public TurboModule {
NSInvocation *inv,
NSMutableArray *retainedObjectsForInvocation);

BOOL hasMethodArgConversionSelector(NSString *methodName, int argIndex);
SEL getMethodArgConversionSelector(NSString *methodName, int argIndex);

using PromiseInvocationBlock = void (^)(RCTPromiseResolveBlock resolveWrapper, RCTPromiseRejectBlock rejectWrapper);
jsi::Value
createPromise(jsi::Runtime &runtime, std::shared_ptr<react::CallInvoker> jsInvoker, PromiseInvocationBlock invoke);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ static int32_t getUniqueId()
};

if (wasMethodSync) {
nativeInvoker_->invokeSync([block]() -> void { block(); });
block();
} else {
asyncCallCounter = getUniqueId();
TurboModulePerfLogger::asyncMethodCallDispatch(moduleName, methodName);
Expand Down

0 comments on commit 976f51a

Please sign in to comment.