Android: Log basic initialization performance numbers.

[tido64]

Summary

I need to measure React Native initialization performance on Android. On iOS, I simply pulled numbers from RCTPerformanceLogger when needed. The Android equivalent is missing.

Changelog

[Android] [Added] - Basic initialization performance logging.

Test Plan

Tests were added.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[hey99xx]

Do we need this as part of RN core? I've found creating a custom ReactMarker.MarkerListener implementation sufficient, and I can save all events, times, thread ids etc to JSON to disk and analyze with Chrome tracing as explained in http://blog.nparashuram.com/2018/11/react-native-performance-playbook-part-i.html

If you want to go ahead with what's on iOS, one difference there is RCTBridge owns RCTPerformanceLogger, so the events are focused. On Android if you have multiple ReactInstanceManagers your events will be all over the place as all ReactPerformanceLoggers would be receiving events for ReactInstanceManager they don't care about. That's where String tag, int instanceKey could be handy.

[tido64]

I do believe we need it in core. For context, we have high-value components that get integrated into numerous purely native apps. Some of the apps initialize React Native on app startup, when the app is idle, and some on demand. We don't want to make intrusive changes to all of these apps (we're talking ~20 apps we would have to update), I think it makes sense for React Native to log like it is done on iOS.

I'm not entirely sure about your comment about instanceKey. Can you please elaborate? When I look at the code, it seems to always pass 0 unless the marker comes from a native module.

  @DoNotStrip
  public static void logMarker(ReactMarkerConstants name) {
    logMarker(name, null, 0);
  }

[hey99xx]

We don't want to make intrusive changes to all of these apps

Won't you have to call something like reactNativeHost.getReactPerformanceLogger().getDuration() in all those apps? Is that ok?

For markers, it uses instanceKey/tag for only some of them. For example in your code you use RUN_JS_BUNDLE_END to detect script execution end, but that marker is tagged with script name:

react-native/ReactAndroid/src/main/jni/react/jni/JReactMarker.cpp

Lines 38 to 39 in 1151c09

	case ReactMarker::RUN_JS_BUNDLE_STOP:
	JReactMarker::logMarker("RUN_JS_BUNDLE_END", tag);

react-native/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp

Line 132 in 6646460

ReactMarker::RUN_JS_BUNDLE_STOP, scriptName.c_str());

Maybe it does not matter if you have a single bundle and don't need to match pairs, not sure. I'll leave it up to you and maintainers. This code does not break custom ReactMarker listeners so I don't mind it.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[mdvacca]

CC @axe-fb

[axe-fb]

IMHO, we dont need this in core, especially since this can be extracted in the app itself.

[tido64]

@axe-fb The reason I think it makes sense to have it in core is because every app will have to implement this by themselves, and all will do it a bit differently. I think it's especially valueable if you own a component that is integrated in many hybrid apps without any guarantees for how or when React Native initializes. This means you can grab the numbers yourself without asking the apps themselves to make any intrusive changes. We've had issues with this in the past in that people have been complaining about perf issues, and increases in ANR. Without these numbers, we're basically flying blind and can't help them address issues.

That said, I'm open to suggestions on any improvements or alternative solutions you might have.

[axe-fb]

How is mData exposed to the apps ? Whats the API to consume this ?

[axe-fb]

What happens when app is reloaded in DEV mode ? Will the markers continue to aggregate ?

[axe-fb]

Why can't this be a singleton ? I am not sure if we are using this instance variable anywhere else.

[axe-fb]

Why are we only marking these ? Why not just pick up all markers that RN gives ?

[axe-fb]

I think that this can be a library ? I still don't understand how existing apps could use this data - could you please explain that ?

[mdvacca]

@tido64 Thank you for working on this PR!
I agree with @axe-fb that in this case it would be better to not include this PR into the core of RN.

React Native already exposes all the required APIs to measure performance in RN Android and our objective is to keep the Core of the framework as small as possible, exposing clear APIs that should be used or extended by ReactNative apps.
In this case I recommend you to create a library that allow you to reuse this code across multiple React Native Android apps.

[mdvacca]

.