Instrument sync and async method calls (#28893)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28893

`JSIExecutor::callSerializableNativeHook` converts the arguments from `JSI::Value` to `folly::dynamic`. Then, `RCTNativeModule` converts the arguments from `folly::dynamic` to ObjC data structures in its `static invokeInner` function.

Therefore, I decided to start the sync markers inside `JSIExecutor::callSerializableNativeHook`, which required me to expose these two methode `ModuleRegistry::getModuleName` and `ModuleRegistry::getModuleSyncMethodName`. This shouldn't modify performance because we eagerly generate a NativeModule's methods when it's first required. So, at worst, this is doing a cache lookup.

Changelog: [Internal]

Reviewed By: PeteTheHeat

Differential Revision: D21443610

fbshipit-source-id: 67cf563b0b06153e56e63ba7e186eea31eafc853

React/CxxModule/RCTNativeModule.h

@@ -16,6 +16,7 @@ class RCTNativeModule : public NativeModule {
   RCTNativeModule(RCTBridge *bridge, RCTModuleData *moduleData);
 
   std::string getName() override;
+  std::string getSyncMethodName(unsigned int methodId) override;
   std::vector<MethodDescriptor> getMethods() override;
   folly::dynamic getConstants() override;
   void invoke(unsigned int methodId, folly::dynamic &&params, int callId)

React/CxxModule/RCTNativeModule.mm

@@ -15,16 +15,26 @@
 #import <React/RCTLog.h>
 #import <React/RCTProfile.h>
 #import <React/RCTUtils.h>
+#import <ReactCommon/NativeModulePerfLogger.h>
 
 #ifdef WITH_FBSYSTRACE
 #include <fbsystrace.h>
 #endif
 
+namespace {
+enum SchedulingContext { Sync, Async };
+}
+
 namespace facebook {
 namespace react {
 
-static MethodCallResult
-invokeInner(RCTBridge *bridge, RCTModuleData *moduleData, unsigned int methodId, const folly::dynamic &params);
+static MethodCallResult invokeInner(
+    RCTBridge *bridge,
+    RCTModuleData *moduleData,
+    unsigned int methodId,
+    const folly::dynamic &params,
+    int callId,
+    SchedulingContext context);
 
 RCTNativeModule::RCTNativeModule(RCTBridge *bridge, RCTModuleData *moduleData)
     : m_bridge(bridge), m_moduleData(moduleData)
@@ -36,6 +46,11 @@ std::string RCTNativeModule::getName()
   return [m_moduleData.name UTF8String];
 }
 
+std::string RCTNativeModule::getSyncMethodName(unsigned int methodId)
+{
+  return m_moduleData.methods[methodId].JSMethodName;
+}
+
 std::vector<MethodDescriptor> RCTNativeModule::getMethods()
 {
   std::vector<MethodDescriptor> descs;
@@ -58,13 +73,26 @@ folly::dynamic RCTNativeModule::getConstants()
 
 void RCTNativeModule::invoke(unsigned int methodId, folly::dynamic &&params, int callId)
 {
+  const char *moduleName = [m_moduleData.name UTF8String];
+  const char *methodName = m_moduleData.methods[methodId].JSMethodName;
+
+  dispatch_queue_t queue = m_moduleData.methodQueue;
+  const bool isSyncModule = queue == RCTJSThread;
+
+  if (isSyncModule) {
+    NativeModulePerfLogger::getInstance().syncMethodCallStart(moduleName, methodName);
+    NativeModulePerfLogger::getInstance().syncMethodCallArgConversionStart(moduleName, methodName);
+  } else {
+    NativeModulePerfLogger::getInstance().asyncMethodCallStart(moduleName, methodName);
+  }
+
   // capture by weak pointer so that we can safely use these variables in a callback
   __weak RCTBridge *weakBridge = m_bridge;
   __weak RCTModuleData *weakModuleData = m_moduleData;
   // The BatchedBridge version of this buckets all the callbacks by thread, and
   // queues one block on each.  This is much simpler; we'll see how it goes and
   // iterate.
-  dispatch_block_t block = [weakBridge, weakModuleData, methodId, params = std::move(params), callId] {
+  dispatch_block_t block = [weakBridge, weakModuleData, methodId, params = std::move(params), callId, isSyncModule] {
 #ifdef WITH_FBSYSTRACE
     if (callId != -1) {
       fbsystrace_end_async_flow(TRACE_TAG_REACT_APPS, "native", callId);
@@ -72,13 +100,14 @@ void RCTNativeModule::invoke(unsigned int methodId, folly::dynamic &&params, int
 #else
     (void)(callId);
 #endif
-    invokeInner(weakBridge, weakModuleData, methodId, std::move(params));
+    invokeInner(weakBridge, weakModuleData, methodId, std::move(params), callId, isSyncModule ? Sync : Async);
   };
 
-  dispatch_queue_t queue = m_moduleData.methodQueue;
-  if (queue == RCTJSThread) {
+  if (isSyncModule) {
     block();
+    NativeModulePerfLogger::getInstance().syncMethodCallReturnConversionEnd(moduleName, methodName);
   } else if (queue) {
+    NativeModulePerfLogger::getInstance().asyncMethodCallDispatch(moduleName, methodName);
     dispatch_async(queue, block);
   }
 
@@ -90,17 +119,36 @@ void RCTNativeModule::invoke(unsigned int methodId, folly::dynamic &&params, int
         m_moduleData.name);
   }
 #endif
+
+  if (isSyncModule) {
+    NativeModulePerfLogger::getInstance().syncMethodCallEnd(moduleName, methodName);
+  } else {
+    NativeModulePerfLogger::getInstance().asyncMethodCallEnd(moduleName, methodName);
+  }
 }
 
 MethodCallResult RCTNativeModule::callSerializableNativeHook(unsigned int reactMethodId, folly::dynamic &&params)
 {
-  return invokeInner(m_bridge, m_moduleData, reactMethodId, params);
+  return invokeInner(m_bridge, m_moduleData, reactMethodId, params, 0, Sync);
 }
 
-static MethodCallResult
-invokeInner(RCTBridge *bridge, RCTModuleData *moduleData, unsigned int methodId, const folly::dynamic &params)
+static MethodCallResult invokeInner(
+    RCTBridge *bridge,
+    RCTModuleData *moduleData,
+    unsigned int methodId,
+    const folly::dynamic &params,
+    int callId,
+    SchedulingContext context)
 {
   if (!bridge || !bridge.valid || !moduleData) {
+    if (context == Sync) {
+      /**
+       * NOTE: moduleName and methodName are "". This shouldn't be an issue because there can only be one ongoing sync
+       * call at a time, and when we call syncMethodCallFail, that one call should terminate. This is also an
+       * exceptional scenario, so it shouldn't occur often.
+       */
+      NativeModulePerfLogger::getInstance().syncMethodCallFail("N/A", "N/A");
+    }
     return folly::none;
   }
 
@@ -109,11 +157,46 @@ invokeInner(RCTBridge *bridge, RCTModuleData *moduleData, unsigned int methodId,
     RCTLogError(@"Unknown methodID: %ud for module: %@", methodId, moduleData.name);
   }
 
+  const char *moduleName = [moduleData.name UTF8String];
+  const char *methodName = moduleData.methods[methodId].JSMethodName;
+
+  if (context == Async) {
+    NativeModulePerfLogger::getInstance().asyncMethodCallExecutionStart(moduleName, methodName, (int32_t)callId);
+    NativeModulePerfLogger::getInstance().asyncMethodCallExecutionArgConversionStart(
+        moduleName, methodName, (int32_t)callId);
+  }
+
   NSArray *objcParams = convertFollyDynamicToId(params);
+
+  if (context == Sync) {
+    NativeModulePerfLogger::getInstance().syncMethodCallArgConversionEnd(moduleName, methodName);
+  }
+
   @try {
+    if (context == Sync) {
+      NativeModulePerfLogger::getInstance().syncMethodCallExecutionStart(moduleName, methodName);
+    } else {
+      NativeModulePerfLogger::getInstance().asyncMethodCallExecutionArgConversionEnd(
+          moduleName, methodName, (int32_t)callId);
+    }
+
     id result = [method invokeWithBridge:bridge module:moduleData.instance arguments:objcParams];
+
+    if (context == Sync) {
+      NativeModulePerfLogger::getInstance().syncMethodCallExecutionEnd(moduleName, methodName);
+      NativeModulePerfLogger::getInstance().syncMethodCallReturnConversionStart(moduleName, methodName);
+    } else {
+      NativeModulePerfLogger::getInstance().asyncMethodCallExecutionEnd(moduleName, methodName, (int32_t)callId);
+    }
+
     return convertIdToFollyDynamic(result);
   } @catch (NSException *exception) {
+    if (context == Sync) {
+      NativeModulePerfLogger::getInstance().syncMethodCallFail(moduleName, methodName);
+    } else {
+      NativeModulePerfLogger::getInstance().asyncMethodCallExecutionFail(moduleName, methodName, (int32_t)callId);
+    }
+
     // Pass on JS exceptions
     if ([exception.name hasPrefix:RCTFatalExceptionName]) {
       @throw exception;

ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.cpp

@@ -50,6 +50,26 @@ std::string JavaNativeModule::getName() {
   return getNameMethod(wrapper_)->toStdString();
 }
 
+std::string JavaNativeModule::getSyncMethodName(unsigned int reactMethodId) {
+  if (reactMethodId >= syncMethods_.size()) {
+    throw std::invalid_argument(folly::to<std::string>(
+        "methodId ",
+        reactMethodId,
+        " out of range [0..",
+        syncMethods_.size(),
+        "]"));
+  }
+
+  auto &methodInvoker = syncMethods_[reactMethodId];
+
+  if (!methodInvoker.hasValue()) {
+    throw std::invalid_argument(folly::to<std::string>(
+        "methodId ", reactMethodId, " is not a recognized sync method"));
+  }
+
+  return methodInvoker->getMethodName();
+}
+
 std::vector<MethodDescriptor> JavaNativeModule::getMethods() {
   std::vector<MethodDescriptor> ret;
   syncMethods_.clear();
@@ -69,6 +89,7 @@ std::vector<MethodDescriptor> JavaNativeModule::getMethods() {
           syncMethods_.begin() + methodIndex,
           MethodInvoker(
               desc->getMethod(),
+              methodName,
               desc->getSignature(),
               getName() + "." + methodName,
               true));
@@ -148,6 +169,7 @@ NewJavaNativeModule::NewJavaNativeModule(
     auto name = desc->getName();
     methods_.emplace_back(
         desc->getMethod(),
+        desc->getName(),
         desc->getSignature(),
         moduleName + "." + name,
         type == "syncHook");

ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.h

@@ -69,6 +69,7 @@ class JavaNativeModule : public NativeModule {
         messageQueueThread_(std::move(messageQueueThread)) {}
 
   std::string getName() override;
+  std::string getSyncMethodName(unsigned int reactMethodId) override;
   folly::dynamic getConstants() override;
   std::vector<MethodDescriptor> getMethods() override;
   void invoke(unsigned int reactMethodId, folly::dynamic &&params, int callId)

ReactAndroid/src/main/jni/react/jni/MethodInvoker.cpp

@@ -190,10 +190,12 @@ std::size_t countJsArgs(const std::string &signature) {
 
 MethodInvoker::MethodInvoker(
     alias_ref<JReflectMethod::javaobject> method,
+    std::string methodName,
     std::string signature,
     std::string traceName,
     bool isSync)
     : method_(method->getMethodID()),
+      methodName_(methodName),
       signature_(signature),
       jsArgCount_(countJsArgs(signature) - 2),
       traceName_(std::move(traceName)),
@@ -203,6 +205,10 @@ MethodInvoker::MethodInvoker(
       << "Non-sync hooks cannot have a non-void return type";
 }
 
+std::string MethodInvoker::getMethodName() const {
+  return methodName_;
+}
+
 MethodCallResult MethodInvoker::invoke(
     std::weak_ptr<Instance> &instance,
     alias_ref<JBaseJavaModule::javaobject> module,

ReactAndroid/src/main/jni/react/jni/MethodInvoker.h

@@ -37,6 +37,7 @@ class MethodInvoker {
  public:
   MethodInvoker(
       jni::alias_ref<JReflectMethod::javaobject> method,
+      std::string methodName,
       std::string signature,
       std::string traceName,
       bool isSync);
@@ -46,12 +47,15 @@ class MethodInvoker {
       jni::alias_ref<JBaseJavaModule::javaobject> module,
       const folly::dynamic &params);
 
+  std::string getMethodName() const;
+
   bool isSyncHook() const {
     return isSync_;
   }
 
  private:
   jmethodID method_;
+  std::string methodName_;
   std::string signature_;
   std::size_t jsArgCount_;
   std::string traceName_;

ReactCommon/cxxreact/CxxNativeModule.cpp

@@ -58,6 +58,18 @@ std::string CxxNativeModule::getName() {
   return name_;
 }
 
+std::string CxxNativeModule::getSyncMethodName(unsigned int reactMethodId) {
+  if (reactMethodId >= methods_.size()) {
+    throw std::invalid_argument(folly::to<std::string>(
+        "methodId ",
+        reactMethodId,
+        " out of range [0..",
+        methods_.size(),
+        "]"));
+  }
+  return methods_[reactMethodId].name;
+}
+
 std::vector<MethodDescriptor> CxxNativeModule::getMethods() {
   lazyInit();
 

ReactCommon/cxxreact/CxxNativeModule.h

@@ -37,6 +37,7 @@ class RN_EXPORT CxxNativeModule : public NativeModule {
         messageQueueThread_(messageQueueThread) {}
 
   std::string getName() override;
+  std::string getSyncMethodName(unsigned int methodId) override;
   std::vector<MethodDescriptor> getMethods() override;
   folly::dynamic getConstants() override;
   void invoke(unsigned int reactMethodId, folly::dynamic &&params, int callId)

ReactCommon/cxxreact/ModuleRegistry.cpp

@@ -195,6 +195,26 @@ folly::Optional<ModuleConfig> ModuleRegistry::getConfig(
   }
 }
 
+std::string ModuleRegistry::getModuleName(unsigned int moduleId) {
+  if (moduleId >= modules_.size()) {
+    throw std::runtime_error(folly::to<std::string>(
+        "moduleId ", moduleId, " out of range [0..", modules_.size(), ")"));
+  }
+
+  return modules_[moduleId]->getName();
+}
+
+std::string ModuleRegistry::getModuleSyncMethodName(
+    unsigned int moduleId,
+    unsigned int methodId) {
+  if (moduleId >= modules_.size()) {
+    throw std::runtime_error(folly::to<std::string>(
+        "moduleId ", moduleId, " out of range [0..", modules_.size(), ")"));
+  }
+
+  return modules_[moduleId]->getSyncMethodName(methodId);
+}
+
 void ModuleRegistry::callNativeMethod(
     unsigned int moduleId,
     unsigned int methodId,

ReactCommon/cxxreact/ModuleRegistry.h

@@ -59,6 +59,11 @@ class RN_EXPORT ModuleRegistry {
       unsigned int methodId,
       folly::dynamic &&args);
 
+  std::string getModuleName(unsigned int moduleId);
+  std::string getModuleSyncMethodName(
+      unsigned int moduleId,
+      unsigned int methodName);
+
  private:
   // This is always populated
   std::vector<std::unique_ptr<NativeModule>> modules_;

ReactCommon/cxxreact/NativeModule.h

@@ -31,6 +31,7 @@ class NativeModule {
  public:
   virtual ~NativeModule() {}
   virtual std::string getName() = 0;
+  virtual std::string getSyncMethodName(unsigned int methodId) = 0;
   virtual std::vector<MethodDescriptor> getMethods() = 0;
   virtual folly::dynamic getConstants() = 0;
   virtual void

ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp

@@ -72,6 +72,7 @@ JSIExecutor::JSIExecutor(
       delegate_(delegate),
       nativeModules_(std::make_shared<JSINativeModules>(
           delegate ? delegate->getModuleRegistry() : nullptr)),
+      moduleRegistry_(delegate ? delegate->getModuleRegistry() : nullptr),
       scopedTimeoutInvoker_(scopedTimeoutInvoker),
       runtimeInstaller_(runtimeInstaller) {
   runtime_->global().setProperty(
@@ -443,16 +444,54 @@ Value JSIExecutor::nativeCallSyncHook(const Value *args, size_t count) {
         folly::to<std::string>("method parameters should be array"));
   }
 
+  unsigned int moduleId = static_cast<unsigned int>(args[0].getNumber());
+  unsigned int methodId = static_cast<unsigned int>(args[1].getNumber());
+  std::string moduleName;
+  std::string methodName;
+
+  if (moduleRegistry_) {
+    moduleName = moduleRegistry_->getModuleName(moduleId);
+    methodName = moduleRegistry_->getModuleSyncMethodName(moduleId, methodId);
+
+    NativeModulePerfLogger::getInstance().syncMethodCallStart(
+        moduleName.c_str(), methodName.c_str());
+
+    NativeModulePerfLogger::getInstance().syncMethodCallArgConversionStart(
+        moduleName.c_str(), methodName.c_str());
+  }
+
   MethodCallResult result = delegate_->callSerializableNativeHook(
-      *this,
-      static_cast<unsigned int>(args[0].getNumber()), // moduleId
-      static_cast<unsigned int>(args[1].getNumber()), // methodId
-      dynamicFromValue(*runtime_, args[2])); // args
+      *this, moduleId, methodId, dynamicFromValue(*runtime_, args[2]));
+
+  /**
+   * Note:
+   * In RCTNativeModule, folly::none is returned from callSerializableNativeHook
+   * when executing a NativeModule method fails. Therefore, it's safe to not
+   * terminate the syncMethodCall when folly::none is returned.
+   *
+   * TODO: In JavaNativeModule, folly::none is returned when the synchronous
+   * NativeModule method has the void return type. Change this to return
+   * folly::dynamic(nullptr) instead, so that folly::none is reserved for
+   * exceptional scenarios.
+   *
+   * TODO: Investigate CxxModule infra to see if folly::none is used for
+   * returns in exceptional scenarios.
+   **/
 
   if (!result.hasValue()) {
     return Value::undefined();
   }
-  return valueFromDynamic(*runtime_, result.value());
+
+  Value returnValue = valueFromDynamic(*runtime_, result.value());
+
+  if (moduleRegistry_) {
+    NativeModulePerfLogger::getInstance().syncMethodCallReturnConversionEnd(
+        moduleName.c_str(), methodName.c_str());
+    NativeModulePerfLogger::getInstance().syncMethodCallEnd(
+        moduleName.c_str(), methodName.c_str());
+  }
+
+  return returnValue;
 }
 
 #if DEBUG

ReactCommon/jsiexecutor/jsireact/JSIExecutor.h

@@ -123,6 +123,7 @@ class JSIExecutor : public JSExecutor {
   std::shared_ptr<jsi::Runtime> runtime_;
   std::shared_ptr<ExecutorDelegate> delegate_;
   std::shared_ptr<JSINativeModules> nativeModules_;
+  std::shared_ptr<ModuleRegistry> moduleRegistry_;
   std::once_flag bindFlag_;
   std::unique_ptr<RAMBundleRegistry> bundleRegistry_;
   JSIScopedTimeoutInvoker scopedTimeoutInvoker_;