Improve systrace markers

Reviewed By: mhorowitz

Differential Revision: D4860135

fbshipit-source-id: ce963010883e6b9cc8e34f7ff01b4018cd195eba

ReactCommon/cxxreact/CxxNativeModule.cpp

@@ -7,7 +7,8 @@
 
 #include <folly/json.h>
 
-#include <cxxreact/JsArgumentHelpers.h>
+#include "JsArgumentHelpers.h"
+#include "SystraceSection.h"
 
 using facebook::xplat::module::CxxModule;
 
@@ -75,7 +76,7 @@ folly::dynamic CxxNativeModule::getConstants() {
   return constants;
 }
 
-void CxxNativeModule::invoke(unsigned int reactMethodId, folly::dynamic&& params) {
+void CxxNativeModule::invoke(unsigned int reactMethodId, folly::dynamic&& params, int callId) {
   if (reactMethodId >= methods_.size()) {
     throw std::invalid_argument(folly::to<std::string>("methodId ", reactMethodId,
         " out of range [0..", methods_.size(), "]"));
@@ -128,7 +129,13 @@ void CxxNativeModule::invoke(unsigned int reactMethodId, folly::dynamic&& params
   // stack.  I'm told that will be possible in the future.  TODO
   // mhorowitz #7128529: convert C++ exceptions to Java
 
-  messageQueueThread_->runOnQueue([method, params=std::move(params), first, second] () {
+  messageQueueThread_->runOnQueue([method, params=std::move(params), first, second, callId] () {
+  #ifdef WITH_FBSYSTRACE
+    if (callId != -1) {
+      fbsystrace_end_async_flow(TRACE_TAG_REACT_APPS, "native", callId);
+    }
+  #endif
+    SystraceSection s(method.name.c_str());
     try {
       method.func(std::move(params), first, second);
     } catch (const facebook::xplat::JsArgumentException& ex) {

ReactCommon/cxxreact/CxxNativeModule.h

@@ -27,7 +27,7 @@ public:
   std::string getName() override;
   std::vector<MethodDescriptor> getMethods() override;
   folly::dynamic getConstants() override;
-  void invoke(unsigned int reactMethodId, folly::dynamic&& params) override;
+  void invoke(unsigned int reactMethodId, folly::dynamic&& params, int callId) override;
   MethodCallResult callSerializableNativeHook(unsigned int hookId, folly::dynamic&& args) override;
 
 private:

ReactCommon/cxxreact/JSCExecutor.cpp

@@ -182,7 +182,7 @@ static bool canUseInspector(JSContextRef context) {
 #endif
 
 void JSCExecutor::initOnJSVMThread() throw(JSException) {
-  SystraceSection s("JSCExecutor.initOnJSVMThread");
+  SystraceSection s("JSCExecutor::initOnJSVMThread");
 
   #if defined(__APPLE__)
   const bool useCustomJSC = m_jscConfig.getDefault("UseCustomJSC", false).getBool();
@@ -353,20 +353,15 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr<const JSBigString> scrip
   } else
 #endif
   {
-    #ifdef WITH_FBSYSTRACE
-    fbsystrace_begin_section(
-      TRACE_TAG_REACT_CXX_BRIDGE,
-      "JSCExecutor::loadApplicationScript-createExpectingAscii");
-    #endif
-
-    ReactMarker::logMarker(ReactMarker::JS_BUNDLE_STRING_CONVERT_START);
-    String jsScript = jsStringFromBigString(m_context, *script);
-    ReactMarker::logMarker(ReactMarker::JS_BUNDLE_STRING_CONVERT_STOP);
-
-    #ifdef WITH_FBSYSTRACE
-    fbsystrace_end_section(TRACE_TAG_REACT_CXX_BRIDGE);
-    #endif
+    String jsScript;
+    {
+      SystraceSection s_("JSCExecutor::loadApplicationScript-createExpectingAscii");
+      ReactMarker::logMarker(ReactMarker::JS_BUNDLE_STRING_CONVERT_START);
+      jsScript = jsStringFromBigString(m_context, *script);
+      ReactMarker::logMarker(ReactMarker::JS_BUNDLE_STRING_CONVERT_STOP);
+    }
 
+    SystraceSection s_("JSCExecutor::loadApplicationScript-evaluateScript");
     evaluateScript(m_context, jsScript, jsSourceURL);
   }
 
@@ -457,9 +452,9 @@ void JSCExecutor::flush() {
 
 void JSCExecutor::callFunction(const std::string& moduleId, const std::string& methodId, const folly::dynamic& arguments) {
   SystraceSection s("JSCExecutor::callFunction");
+
   // This weird pattern is because Value is not default constructible.
   // The lambda is inlined, so there's no overhead.
-
   auto result = [&] {
     try {
       if (!m_callFunctionReturnResultAndFlushedQueueJS) {
@@ -524,8 +519,7 @@ Value JSCExecutor::callFunctionSyncWithValue(
 
 void JSCExecutor::setGlobalVariable(std::string propName, std::unique_ptr<const JSBigString> jsonValue) {
   try {
-    SystraceSection s("JSCExecutor.setGlobalVariable",
-                      "propName", propName);
+    SystraceSection s("JSCExecutor::setGlobalVariable", "propName", propName);
 
     auto valueToInject = Value::fromJSON(m_context, jsStringFromBigString(m_context, *jsonValue));
     Object::getGlobalObject(m_context).setProperty(propName.c_str(), valueToInject);

ReactCommon/cxxreact/ModuleRegistry.cpp

@@ -118,14 +118,7 @@ void ModuleRegistry::callNativeMethod(unsigned int moduleId, unsigned int method
     throw std::runtime_error(
       folly::to<std::string>("moduleId ", moduleId, " out of range [0..", modules_.size(), ")"));
   }
-
-#ifdef WITH_FBSYSTRACE
-  if (callId != -1) {
-    fbsystrace_end_async_flow(TRACE_TAG_REACT_APPS, "native", callId);
-  }
-#endif
-
-  modules_[moduleId]->invoke(methodId, std::move(params));
+  modules_[moduleId]->invoke(methodId, std::move(params), callId);
 }
 
 MethodCallResult ModuleRegistry::callSerializableNativeHook(unsigned int moduleId, unsigned int methodId, folly::dynamic&& params) {

ReactCommon/cxxreact/NativeModule.h

@@ -5,8 +5,8 @@
 #include <string>
 #include <vector>
 
-#include <folly/dynamic.h>
 #include <cxxreact/Executor.h>
+#include <folly/dynamic.h>
 
 namespace facebook {
 namespace react {
@@ -29,7 +29,7 @@ class NativeModule {
   virtual folly::dynamic getConstants() = 0;
   // TODO mhorowitz: do we need initialize()/onCatalystInstanceDestroy() in C++
   // or only Java?
-  virtual void invoke(unsigned int reactMethodId, folly::dynamic&& params) = 0;
+  virtual void invoke(unsigned int reactMethodId, folly::dynamic&& params, int callId) = 0;
   virtual MethodCallResult callSerializableNativeHook(unsigned int reactMethodId, folly::dynamic&& args) = 0;
 };
 

ReactCommon/cxxreact/NativeToJsBridge.cpp

@@ -124,27 +124,21 @@ void NativeToJsBridge::callFunction(
   int systraceCookie = -1;
   #ifdef WITH_FBSYSTRACE
   systraceCookie = m_systraceCookie++;
-  std::string tracingName = fbsystrace_is_tracing(TRACE_TAG_REACT_CXX_BRIDGE) ?
-    folly::to<std::string>("JSCall__", module, '_', method) : std::string();
-  SystraceSection s(tracingName.c_str());
   FbSystraceAsyncFlow::begin(
       TRACE_TAG_REACT_CXX_BRIDGE,
-      tracingName.c_str(),
+      "JSCall",
       systraceCookie);
-  #else
-  std::string tracingName;
   #endif
 
-  runOnExecutorQueue([module = std::move(module), method = std::move(method), arguments = std::move(arguments), tracingName = std::move(tracingName), systraceCookie]
+  runOnExecutorQueue([module = std::move(module), method = std::move(method), arguments = std::move(arguments), systraceCookie]
     (JSExecutor* executor) {
       #ifdef WITH_FBSYSTRACE
       FbSystraceAsyncFlow::end(
           TRACE_TAG_REACT_CXX_BRIDGE,
-          tracingName.c_str(),
+          "JSCall",
           systraceCookie);
-      SystraceSection s(tracingName.c_str());
+      SystraceSection s("NativeToJsBridge::callFunction", "module", module, "method", method);
       #endif
-
       // This is safe because we are running on the executor's thread: it won't
       // destruct until after it's been unregistered (which we check above) and
       // that will happen on this thread
@@ -169,9 +163,8 @@ void NativeToJsBridge::invokeCallback(double callbackId, folly::dynamic&& argume
           TRACE_TAG_REACT_CXX_BRIDGE,
           "<callback>",
           systraceCookie);
-      SystraceSection s("NativeToJsBridge.invokeCallback");
+      SystraceSection s("NativeToJsBridge::invokeCallback");
       #endif
-
       executor->invokeCallback(callbackId, arguments);
     });
 }