Performance markers logged from C++ no longer work in Android

[karanjthakkar]

🐛 Bug Report

From 0.58.0 onwards, the four most important perf markers used for measuring RN startup time on Android no longer work:

NATIVE_MODULE_SETUP_START
NATIVE_MODULE_SETUP_END
RUN_JS_BUNDLE_START
RUN_JS_BUNDLE_STOP

This is the commit where the refactor from JSCExecutor to JSIExecutor took place: 749b18d

---

On further investigation, the reason behind this is the hasLogger boolean here never gets a truthy value. This is because the address &ReactMarker::logTaggedMarker here is different from the one here.

If we instead use JReactMarker::logMarker here, then the metrics work fine.

cc-ing: @mhorowitz and @axe-fb who seem most involved with these changes. @leanmazzu for visibility.

To Reproduce

1. Init a new RN project on any version above 0.58

2. Add the following code to start logging markers:

   ReactMarker.addListener(new ReactMarker.MarkerListener() {
     @Override
     public void logMarker(
             ReactMarkerConstants name, @Nullable String tag, int instanceKey) {
       Log.d("ReactNativeEvent", "name: "+name.name() + " tag: "+tag);
     }
   });

3. Check logcat. The above mentioned 4 markers won't be logged.

Expected Behavior

It should log the relevant markers.

Code Example

N/A

Environment

  React Native Environment Info:
    System:
      OS: macOS 10.14.1
      CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
      Memory: 26.55 MB / 16.00 GB
      Shell: 5.3 - /bin/zsh
    Binaries:
      Node: 10.13.0 - ~/.nvm/versions/node/v10.13.0/bin/node
      npm: 6.4.1 - ~/.nvm/versions/node/v10.13.0/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
      Android SDK:
        API Levels: 23, 24, 25, 26, 27, 28
        Build Tools: 23.0.1, 25.0.0, 25.0.2, 26.0.2, 26.0.3, 27.0.3, 28.0.3
        System Images: android-26 | Google APIs Intel x86 Atom_64, android-26 | Google Play Intel x86 Atom, android-27 | Google APIs Intel x86 Atom, android-28 | Google APIs Intel x86 Atom
    IDEs:
      Android Studio: 3.3 AI-182.5107.16.33.5264788
      Xcode: 10.1/10B61 - /usr/bin/xcodebuild
    npmPackages:
      react: 16.8.3 => 16.8.3
      react-native: 0.59.0-rc.3 => 0.59.0-rc.3
    npmGlobalPackages:
      react-native-cli: 2.0.1

[leanmazzu]

You can check out this commit

This is what worked for us and the fix is based on what @karanjthakkar mentioned above. With this change in place now all the events are being logged.

[mhorowitz]

JSIExecutor is used on iOS and Android The change suggested will break on iOS, which cannot depend on Android-specific code.

If the address of ReactMarker::logTaggedMarker is inconsistent, there is likely a build error causing the variable to be defined in more than one shared object, which violates the C++ One Definition Rule. I do not know much about gradle, so I can't be of much help changing that.

Another strategy would be to pass the LogTaggedMarker instance to JSIExecutor's ctor, instead of using a global, but this would touch a lot more code.

[karanjthakkar]

@mhorowitz Thanks for giving more context. We only have this change for Android so it won't affect the iOS side of things for us but yes we need to find a more platform agnostic fix.

Just to understand things a bit better:

there is likely a build error causing the variable to be defined in more than one shared object

Does this mean that the build process internally for FB gets this to work correctly but in OSS its broken in a subtle way? Could you possibly cc someone who would know more about this or have more insight on how would one go about investigating this issue?

Another strategy would be to pass the LogTaggedMarker instance to JSIExecutor's ctor, instead of using a global, but this would touch a lot more code.

If this change were to be done, would you be happy to accept a PR for this? Without a solution that resolves this issue upstream, I'm afraid things are going to get super unmaintainable for us relying on a private fork everytime we upgrade to a new version.

[karanjthakkar]

Another strategy would be to pass the LogTaggedMarker instance to JSIExecutor's ctor, instead of using a global, but this would touch a lot more code.

@mhorowitz I attempted this here: karanjthakkar@98ae572 What are your thoughts? I have tested it and this now works on both Android and iOS. What are your thoughts?

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[karanjthakkar]

This is still pending merge of a fix.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[karanjthakkar]

Not stale. Still broken and has an open PR.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.