Fix missing perf marker logging

karanjthakkar · Mar 9, 2019 · 98ae572 · 98ae572
1 parent f741d33
commit 98ae572
Show file tree

Hide file tree

Showing 12 changed files with 50 additions and 39 deletions.
diff --git a/React/CxxBridge/JSCExecutorFactory.h b/React/CxxBridge/JSCExecutorFactory.h
@@ -8,22 +8,26 @@
 #pragma once
 
 #include <jsireact/JSIExecutor.h>
+#include <cxxreact/ReactMarker.h>
 
 namespace facebook {
 namespace react {
 
 class JSCExecutorFactory : public JSExecutorFactory {
 public:
   explicit JSCExecutorFactory(
-      JSIExecutor::RuntimeInstaller runtimeInstaller)
-      : runtimeInstaller_(std::move(runtimeInstaller)) {}
+      JSIExecutor::RuntimeInstaller runtimeInstaller,
+      ReactMarker::LogTaggedMarker logTaggedMarker)
+      : runtimeInstaller_(std::move(runtimeInstaller)),
+      logTaggedMarker_(logTaggedMarker) {}
 
   std::unique_ptr<JSExecutor> createJSExecutor(
       std::shared_ptr<ExecutorDelegate> delegate,
       std::shared_ptr<MessageQueueThread> jsQueue) override;
 
 private:
   JSIExecutor::RuntimeInstaller runtimeInstaller_;
+  ReactMarker::LogTaggedMarker logTaggedMarker_;
 };
 
 } // namespace react

diff --git a/React/CxxBridge/JSCExecutorFactory.mm b/React/CxxBridge/JSCExecutorFactory.mm
@@ -25,7 +25,8 @@
         [NSString stringWithUTF8String:message.c_str()]);
     },
     JSIExecutor::defaultTimeoutInvoker,
-    std::move(runtimeInstaller_));
+    std::move(runtimeInstaller_),
+    logTaggedMarker_);
 }
 
 } // namespace react

diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm
@@ -99,9 +99,9 @@ static bool isRAMBundle(NSData *script) {
   return parseTypeFromHeader(header) == ScriptTag::RAMBundle;
 }
 
-static void registerPerformanceLoggerHooks(RCTPerformanceLogger *performanceLogger) {
+static ReactMarker::LogTaggedMarker registerPerformanceLoggerHooks(RCTPerformanceLogger *performanceLogger) {
   __weak RCTPerformanceLogger *weakPerformanceLogger = performanceLogger;
-  ReactMarker::logTaggedMarker = [weakPerformanceLogger](const ReactMarker::ReactMarkerId markerId, const char *__unused tag) {
+  return [weakPerformanceLogger](const ReactMarker::ReactMarkerId markerId, const char *__unused tag) {
     switch (markerId) {
       case ReactMarker::RUN_JS_BUNDLE_START:
         [weakPerformanceLogger markStartForTag:RCTPLScriptExecution];
@@ -208,8 +208,6 @@ - (instancetype)initWithParentBridge:(RCTBridge *)bridge
     _parentBridge = bridge;
     _performanceLogger = [bridge performanceLogger];
 
-    registerPerformanceLoggerHooks(_performanceLogger);
-
     RCTLogInfo(@"Initializing %@ (parent: %@, executor: %@)", self, bridge, [self executorClass]);
 
     /**
@@ -326,7 +324,8 @@ - (void)start
       executorFactory = [cxxDelegate jsExecutorFactoryForBridge:self];
     }
     if (!executorFactory) {
-      executorFactory = std::make_shared<JSCExecutorFactory>(nullptr);
+      executorFactory = std::make_shared<JSCExecutorFactory>(nullptr,
+        registerPerformanceLoggerHooks(_performanceLogger));
     }
   } else {
     id<RCTJavaScriptExecutor> objcExecutor = [self moduleForClass:self.executorClass];

diff --git a/ReactAndroid/src/main/java/com/facebook/react/jscexecutor/OnLoad.cpp b/ReactAndroid/src/main/java/com/facebook/react/jscexecutor/OnLoad.cpp
@@ -11,6 +11,7 @@
 #include <react/jni/JReactMarker.h>
 #include <react/jni/JSLogging.h>
 #include <react/jni/ReadableNativeMap.h>
+#include <cxxreact/ReactMarker.h>
 
 namespace facebook {
 namespace react {
@@ -19,6 +20,7 @@ namespace {
 
 class JSCExecutorFactory : public JSExecutorFactory {
 public:
+  JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}
   std::unique_ptr<JSExecutor> createJSExecutor(
       std::shared_ptr<ExecutorDelegate> delegate,
       std::shared_ptr<MessageQueueThread> jsQueue) override {
@@ -29,8 +31,12 @@ class JSCExecutorFactory : public JSExecutorFactory {
         reactAndroidLoggingHook(message, logLevel);
       },
       JSIExecutor::defaultTimeoutInvoker,
-      nullptr);
+      nullptr,
+      logTaggedMarker_);
   }
+
+private:
+  ReactMarker::LogTaggedMarker logTaggedMarker_;
 };
 
 }
@@ -47,9 +53,8 @@ class JSCExecutorHolder
     // This is kind of a weird place for stuff, but there's no other
     // good place for initialization which is specific to JSC on
     // Android.
-    JReactMarker::setLogPerfMarkerIfNeeded();
     // TODO mhorowitz T28461666 fill in some missing nice to have glue
-    return makeCxxInstance(folly::make_unique<JSCExecutorFactory>());
+    return makeCxxInstance(folly::make_unique<JSCExecutorFactory>(JReactMarker::setLogPerfMarkerIfNeeded()));
   }
 
   static void registerNatives() {

diff --git a/ReactAndroid/src/main/jni/react/jni/JReactMarker.cpp b/ReactAndroid/src/main/jni/react/jni/JReactMarker.cpp
@@ -11,11 +11,8 @@
 namespace facebook {
 namespace react {
 
-void JReactMarker::setLogPerfMarkerIfNeeded() {
-  static std::once_flag flag {};
-  std::call_once(flag, [](){
-    ReactMarker::logTaggedMarker = JReactMarker::logPerfMarker;
-  });
+ReactMarker::LogTaggedMarker JReactMarker::setLogPerfMarkerIfNeeded() {
+  return JReactMarker::logPerfMarker;
 }
 
 void JReactMarker::logMarker(const std::string& marker) {

diff --git a/ReactAndroid/src/main/jni/react/jni/JReactMarker.h b/ReactAndroid/src/main/jni/react/jni/JReactMarker.h
@@ -16,7 +16,7 @@ namespace react {
 class JReactMarker : public facebook::jni::JavaClass<JReactMarker> {
 public:
   static constexpr auto kJavaDescriptor = "Lcom/facebook/react/bridge/ReactMarker;";
-  static void setLogPerfMarkerIfNeeded();
+  static ReactMarker::LogTaggedMarker setLogPerfMarkerIfNeeded();
 
 private:
   static void logMarker(const std::string& marker);

diff --git a/ReactCommon/cxxreact/ReactMarker.cpp b/ReactCommon/cxxreact/ReactMarker.cpp
@@ -20,10 +20,6 @@ LogTaggedMarker logTaggedMarker = nullptr;
 #pragma clang diagnostic pop
 #endif
 
-void logMarker(const ReactMarkerId markerId) {
-  logTaggedMarker(markerId, nullptr);
-}
-
 }
 }
 }
diff --git a/ReactCommon/cxxreact/ReactMarker.h b/ReactCommon/cxxreact/ReactMarker.h
@@ -39,8 +39,6 @@ typedef void(*LogTaggedMarker)(const ReactMarkerId, const char* tag);
 
 extern RN_EXPORT LogTaggedMarker logTaggedMarker;
 
-extern RN_EXPORT void logMarker(const ReactMarkerId markerId);
-
 }
 }
 }
diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp
@@ -58,13 +58,15 @@ JSIExecutor::JSIExecutor(
     std::shared_ptr<ExecutorDelegate> delegate,
     Logger logger,
     const JSIScopedTimeoutInvoker& scopedTimeoutInvoker,
-    RuntimeInstaller runtimeInstaller)
+    RuntimeInstaller runtimeInstaller,
+    ReactMarker::LogTaggedMarker logTaggedMarker)
     : runtime_(runtime),
       delegate_(delegate),
-      nativeModules_(delegate ? delegate->getModuleRegistry() : nullptr),
+      nativeModules_(delegate ? delegate->getModuleRegistry() : nullptr, logTaggedMarker),
       logger_(logger),
       scopedTimeoutInvoker_(scopedTimeoutInvoker),
-      runtimeInstaller_(runtimeInstaller) {
+      runtimeInstaller_(runtimeInstaller),
+      logTaggedMarker_(logTaggedMarker) {
   runtime_->global().setProperty(
       *runtime, "__jsiExecutorDescription", runtime->description());
 }
@@ -144,18 +146,18 @@ void JSIExecutor::loadApplicationScript(
     runtimeInstaller_(*runtime_);
   }
 
-  bool hasLogger(ReactMarker::logTaggedMarker);
+  bool hasLogger(logTaggedMarker_);
   std::string scriptName = simpleBasename(sourceURL);
   if (hasLogger) {
-    ReactMarker::logTaggedMarker(
+    logTaggedMarker_(
         ReactMarker::RUN_JS_BUNDLE_START, scriptName.c_str());
   }
   runtime_->evaluateJavaScript(
       std::make_unique<BigStringBuffer>(std::move(script)), sourceURL);
   flush();
   if (hasLogger) {
-    ReactMarker::logMarker(ReactMarker::CREATE_REACT_CONTEXT_STOP);
-    ReactMarker::logTaggedMarker(
+    logTaggedMarker_(ReactMarker::CREATE_REACT_CONTEXT_STOP, nullptr);
+    logTaggedMarker_(
         ReactMarker::RUN_JS_BUNDLE_STOP, scriptName.c_str());
   }
 }
@@ -182,7 +184,7 @@ void JSIExecutor::registerBundle(
     uint32_t bundleId,
     const std::string& bundlePath) {
   const auto tag = folly::to<std::string>(bundleId);
-  ReactMarker::logTaggedMarker(
+  logTaggedMarker_(
       ReactMarker::REGISTER_JS_SEGMENT_START, tag.c_str());
   if (bundleRegistry_) {
     bundleRegistry_->registerBundle(bundleId, bundlePath);
@@ -192,7 +194,7 @@ void JSIExecutor::registerBundle(
         std::make_unique<BigStringBuffer>(std::move(script)),
         JSExecutor::getSyntheticBundlePath(bundleId, bundlePath));
   }
-  ReactMarker::logTaggedMarker(
+  logTaggedMarker_(
       ReactMarker::REGISTER_JS_SEGMENT_STOP, tag.c_str());
 }
 

diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h
@@ -13,6 +13,7 @@
 #include <jsi/jsi.h>
 #include <functional>
 #include <mutex>
+#include <cxxreact/ReactMarker.h>
 
 namespace facebook {
 namespace react {
@@ -78,7 +79,8 @@ class JSIExecutor : public JSExecutor {
       std::shared_ptr<ExecutorDelegate> delegate,
       Logger logger,
       const JSIScopedTimeoutInvoker& timeoutInvoker,
-      RuntimeInstaller runtimeInstaller);
+      RuntimeInstaller runtimeInstaller,
+      ReactMarker::LogTaggedMarker);
   void loadApplicationScript(
       std::unique_ptr<const JSBigString> script,
       std::string sourceURL) override;
@@ -124,6 +126,7 @@ class JSIExecutor : public JSExecutor {
   Logger logger_;
   JSIScopedTimeoutInvoker scopedTimeoutInvoker_;
   RuntimeInstaller runtimeInstaller_;
+  ReactMarker::LogTaggedMarker logTaggedMarker_;
 
   folly::Optional<jsi::Function> callFunctionReturnFlushedQueue_;
   folly::Optional<jsi::Function> invokeCallbackAndReturnFlushedQueue_;

diff --git a/ReactCommon/jsiexecutor/jsireact/JSINativeModules.cpp b/ReactCommon/jsiexecutor/jsireact/JSINativeModules.cpp
@@ -19,8 +19,10 @@ namespace facebook {
 namespace react {
 
 JSINativeModules::JSINativeModules(
-    std::shared_ptr<ModuleRegistry> moduleRegistry)
-    : m_moduleRegistry(std::move(moduleRegistry)) {}
+    std::shared_ptr<ModuleRegistry> moduleRegistry,
+    ReactMarker::LogTaggedMarker logTaggedMarker)
+    : m_moduleRegistry(std::move(moduleRegistry)),
+    logTaggedMarker_(logTaggedMarker) {}
 
 Value JSINativeModules::getModule(Runtime& rt, const PropNameID& name) {
   if (!m_moduleRegistry) {
@@ -54,9 +56,9 @@ void JSINativeModules::reset() {
 folly::Optional<Object> JSINativeModules::createModule(
     Runtime& rt,
     const std::string& name) {
-  bool hasLogger(ReactMarker::logTaggedMarker);
+  bool hasLogger(logTaggedMarker_);
   if (hasLogger) {
-    ReactMarker::logTaggedMarker(
+    logTaggedMarker_(
         ReactMarker::NATIVE_MODULE_SETUP_START, name.c_str());
   }
 
@@ -80,7 +82,7 @@ folly::Optional<Object> JSINativeModules::createModule(
       moduleInfo.asObject(rt).getPropertyAsObject(rt, "module"));
 
   if (hasLogger) {
-    ReactMarker::logTaggedMarker(
+    logTaggedMarker_(
         ReactMarker::NATIVE_MODULE_SETUP_STOP, name.c_str());
   }
 

diff --git a/ReactCommon/jsiexecutor/jsireact/JSINativeModules.h b/ReactCommon/jsiexecutor/jsireact/JSINativeModules.h
@@ -9,6 +9,7 @@
 #include <string>
 
 #include <cxxreact/ModuleRegistry.h>
+#include <cxxreact/ReactMarker.h>
 #include <folly/Optional.h>
 #include <jsi/jsi.h>
 
@@ -20,14 +21,17 @@ namespace react {
  */
 class JSINativeModules {
  public:
-  explicit JSINativeModules(std::shared_ptr<ModuleRegistry> moduleRegistry);
+  explicit JSINativeModules(
+    std::shared_ptr<ModuleRegistry> moduleRegistry, 
+    ReactMarker::LogTaggedMarker);
   jsi::Value getModule(jsi::Runtime& rt, const jsi::PropNameID& name);
   void reset();
 
  private:
   folly::Optional<jsi::Function> m_genNativeModuleJS;
   std::shared_ptr<ModuleRegistry> m_moduleRegistry;
   std::unordered_map<std::string, jsi::Object> m_objects;
+  ReactMarker::LogTaggedMarker logTaggedMarker_;
 
   folly::Optional<jsi::Object> createModule(
       jsi::Runtime& rt,