-
Notifications
You must be signed in to change notification settings - Fork 54
feat: allow using multiple performance profilers #76
Conversation
frankfka
left a comment
There was a problem hiding this 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?
fortmarek
left a comment
There was a problem hiding this 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 👍
fixture/src/App.tsx
Outdated
| <> | ||
| <ApolloProvider client={apolloClient}> | ||
| <PerformanceProfiler logLevel={LogLevel.Debug}> | ||
| <PerformanceProfiler logLevel={LogLevel.Error}> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
I was able to test this on Shopify Mobile for Android & iOS - this change fixed the reported issue :) |
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
PerformanceProfilerinstances are used in an app, they will “mix” render completion events. BecauseuseNativeRenderCompletionEventslistens onlyRENDER_COMPLETION_EVENT_NAMEand does not differentiate between 2 contexts running at the same time. That results inScreenProfilerNotStartedErrorwhen a screen is pushed in the other context.This PR changes the way native
onRenderCompletedevent gets reported.Before
PerformanceProfilerwould listen to allRENDER_COMPLETION_EVENT_NAMEevents reported by native views and pass them toStateController. Now listening toonRenderCompletedevent is moved down toPerformanceMeasureView.tsxlevel and became an instance-level event, instead of global one. Now when events don't go up toPerformanceProfiler, they don't get mixed up when we have multiple Profilers running.Packages affected:
react-native-performanceTest plan
It's not easy to reproduce the issue in
fixtureapp. I suggest to first runfixtureand confirm that nothing is broken, and after that test the fix in Shopify Mobile.Testing in the fixture app:
rm -rf fixture/dist && yarn up && yarn ios/androidto run a clean build with the fixNested Context Screen- you are already inside the nestedProfilerContextand should see no errors in logsProfilerContext- again, you should see no errors in logstesting-nested-profiler.mov
Tips:
Nested Context Screen, or after.LogLevelfor the mainProfilerContexttoLogLevel.Error, but set a lower level (Debug) for the nested profiler inNestedContextScreen:<PerformanceProfiler logLevel={LogLevel.Debug}>. You should see no performance logs before entering nested context, and start getting them once openingNested Context ScreenChecklist