Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow using multiple performance profilers #76

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

ElviraBurchik
Copy link
Contributor

@ElviraBurchik ElviraBurchik commented Jun 8, 2022

Motivation

The motivation behind this change is to allow using the library in brownfield apps with multiple entry points Native <-> React Native.

Description

Currently if multiple PerformanceProfiler instances are used in an app, they will “mix” render completion events. Because useNativeRenderCompletionEvents listens only RENDER_COMPLETION_EVENT_NAME and does not differentiate between 2 contexts running at the same time. That results in ScreenProfilerNotStartedError when a screen is pushed in the other context.

This PR changes the way native onRenderCompleted event gets reported.
Before PerformanceProfiler would listen to all RENDER_COMPLETION_EVENT_NAME events reported by native views and pass them to StateController. Now listening to onRenderCompleted event is moved down to PerformanceMeasureView.tsx level and became an instance-level event, instead of global one. Now when events don't go up to PerformanceProfiler, they don't get mixed up when we have multiple Profilers running.

Packages affected:

  • react-native-performance

Test plan

It's not easy to reproduce the issue in fixture app. I suggest to first run fixture and confirm that nothing is broken, and after that test the fix in Shopify Mobile.

Testing in the fixture app:

  1. Do rm -rf fixture/dist && yarn up && yarn ios/android to run a clean build with the fix
  2. Go to Nested Context Screen - you are already inside the nested ProfilerContext and should see no errors in logs
  3. Press the button to open a new screen inside the nested ProfilerContext - again, you should see no errors in logs
testing-nested-profiler.mov

Tips:

  • Try on both iOS and Android
  • Try to open other screens before navigating to Nested Context Screen, or after.
  • Try to change the LogLevel for the main ProfilerContext to LogLevel.Error, but set a lower level (Debug) for the nested profiler in NestedContextScreen: <PerformanceProfiler logLevel={LogLevel.Debug}>. You should see no performance logs before entering nested context, and start getting them once opening Nested Context Screen

Checklist

  • I have added a decision record entry, PR includes changes to monorepo setup that may require explanation

@ElviraBurchik ElviraBurchik self-assigned this Jun 8, 2022
Copy link

@frankfka frankfka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes re: rendering callbacks make sense to me - have not 🎩 but i'd love to do this locally - what are the steps?

@ElviraBurchik ElviraBurchik marked this pull request as ready for review June 13, 2022 11:22
Copy link
Contributor

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩 'd and works well 👍

@@ -53,7 +70,7 @@ const App = () => {
return (
<>
<ApolloProvider client={apolloClient}>
<PerformanceProfiler logLevel={LogLevel.Debug}>
<PerformanceProfiler logLevel={LogLevel.Error}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should at least keep LogLevel.Warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely, I used it for testing and forgot to remove 👍

- set LogLevel Debug for a global profiler
- add a note about not using NestedContextScreen as an example
@frankfka
Copy link

I was able to test this on Shopify Mobile for Android & iOS - this change fixed the reported issue :)

@ElviraBurchik ElviraBurchik merged commit a9b6a97 into main Jun 14, 2022
@ElviraBurchik ElviraBurchik deleted the fix/multiple-perf-profilers branch June 14, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants