-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Description
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
What performance issue do we need to solve?
e.g. memory consumption, storage read/write times, React native bridge concerns, inefficient React component rendering, etc.
- storage read/write times
- inefficient React component rendering
This component is one of the most used components and has more than 310 instances depending on chat size
The current way that the HOC works is that it will make a new Onyx connection per each withOnyx instance.
A benchmark reviewed that for a large chat the we're spending about 50sec for the Onyx.get related to retrieving the preferredLocale
What is the impact of this on end-users?
List specific user experiences that will be improved by solving this problem e.g. app boot time, time to for some interaction to complete, etc.
- app boot time
- chat switching time
List any benchmarks that show the severity of the issue
Please also provide exact steps taken to collect metrics above if any so we can independently verify the results.
I'm using Onyx.printMetrics() to monitor the Onyx.get calls that happen during init: https://github.com/Expensify/react-native-onyx#benchmarks
Results
| Device | Samsung Galaxy S7 Edge (2016) |
|---|---|
| OS | Android 8.0 |
| Total: | 123.80sec | Last | 17.49sec | ReportID | 71955477 |
|---|
| method | total time spent | max | min | avg | time last call completed | calls made |
|---|---|---|---|---|---|---|
| Onyx:get | 96.85sec | 2.92sec | 0.10ms | 144.77ms | 17.48sec | 669 |
- Full info in excel: https://docs.google.com/spreadsheets/d/1wGrVXsad6ayQZVqvyUmTuTb1cmeegdE0y-IFzhLiPKU/edit#gid=73551349&range=J8 (search for
preferredLocale)
Setup
Using this Expensify/App hash: b2582f3
Modify this file: src/Expensify.js
- For
Onyx.initaddcaptureMetrics: true - At the end of the file add
global.Onyx = Onyx;
Modify src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
- Make
getInitialReportScreenParamsalways return the same report - e.g.
return {reportID: '71955477'};
Start the app and connect with Flipper or a Hermes Debugger
Steps
- Reload the app
- Wait for processes to settle (about 5-10sec after LHN loads)
- you can use
Onyx.printMetrics({ format: 'string' }})when it starts to return the same value - everything is loaded
- Run
Onyx.printMetrics({ format: 'csv', raw: true }) - Copy the results to a temp file
- Import file as new sheet in excel
Proposed solution
Extract a LocaleContextProvider that wraps the App - the implementation would be pretty much identical to how the class WithLocalize works at the moment:
App/src/components/withLocalize.js
Line 49 in ad77386
| class WithLocalize extends React.Component { |
The difference would be that it will
render a React ContextProvider componentNow only the top level component that wraps the
App subscribes to OnyxThe HOC
withLocalize changes to use a LocaleContextConsumerIt uses the context to get translation related props and pass them to the
WrappedComponentEverything else stays the same
List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)
Note: These should be the same as the benchmarks collected before any changes.
I've done a quick test to verify there will be a significant reduction in Onyx.get time. It's demonstrated in the initial changes in the PR here: #4253
| Total: | 62.62sec | Last | 12.27sec | ReportID | 71955477 |
|---|
| method | total time spent | max | min | avg | time last call completed | calls made |
|---|---|---|---|---|---|---|
| Onyx:get | 40.44sec | 1.79sec | 0.08ms | 139.94ms | 12.27sec | 289 |
Platform:
Where is this issue occurring?
- Web
- iOS
- Android (Biggest impact)
- Desktop App
- Mobile Web
Version Number: 1.0.80-2
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: