iOS - App is extremely slow to load conversations

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Expected Result:

Conversations should be loaded quickly.

Actual Result:

Conversation are taking ~5 seconds to load even when navigating out and back again to the same conversation.

Action Performed:

1. Open cash app in IOS
2. Navigate to a conversation
3. Navigate back to LHN
4. Navigate back to the same conversation.

Workaround:

Unknown.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.54-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

RPReplay_Final1622049740.MP4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @coleaeason https://expensify.slack.com/archives/C01GTK53T8Q/p1622001718242700

iOS is kind of unusable right now. Every time I switch a chat I get a spinning loader for 5-10 and I can see it loading and rendering messages for that chat under the spinner; but it does this for chats I was JUST in.

[MelvinBot]

Triggered auto assignment to @Jag96 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Jag96]

Looking into what caused this as this seems like a regression, some additional notes:

• I can reproduce this locally on the iOS simulator
• After signing out and signing back in, the DM loads at a normal speed again
• Tapping out of a DM and back into the same DM takes longer to load than the previous time

[isagoico]

This was reproducible on iPad too. Here are the device logs
Device logs Ipad.txt

[marcaaron]

I don't see anything unusual in those logs. But also can't reproduce this in simulator.

I'm noticing from the video shared above that there are a number of attachments in the chat we are trying to load and the spinner doesn't go away until they are all loaded. So perhaps fetching those images is blocking the JS thread?

[marcaaron]

Images did not seem to have an effect, but adding a PDF made things go a bit slower.
Hmm no nevermind seems like it is just randomly slow sometimes 😓

I'm testing against the production API to rule out local dev environment slowness.

[Jag96]

I tried changing the ThumbnailImage and ImageView components to just render empty <View> components but I still see the same slowness. It also looks like the issue is happening on reports with and without IOUs, so it has to be something else.

I see Timing:expensify.cash.switch_report.cold logged out in the console, so I had a look at https://graphs.expensify.com/grafana/d/yj2EobAGz/expensify-cash-timing?orgId=1, it looks like it gets a bit steep around 5/10-5/12 so having a look for PRs around that time that would stand out.

[Jag96]

After some testing it looks like the issue starts happening on version 1.0.41-7, looking to see what PR could be the cause and confirming on my physical device that 1.0.41-6 doesn't have this issue.

[Jag96]

Looks like the culprit is #2320, testing and looking into what is causing the slowness

[Jag96]

I've removed this subscription and it seems to have made loading chats much faster and more consistent on my simulator and my physical device. Going to have somebody else test to confirm.

[Jag96]

I've created a PR that will reduce the slowness here: #3169. @yuwenmemon please have a look, if we want to move forward with this fix it will break the draftComment feature and a new fix will have to be implemented.

[yuwenmemon]

Interesting - but the draftComment isn't really changing that often, is it? I wonder what's going on that's causing it to update the prop so often...

[Jag96]

I believe the main issue is that we're creating a subscription for each message, so for chats with lots of messages loading up the chat takes longer.

[kidroca]

I believe the main issue is that we're creating a subscription for each message, so for chats with lots of messages loading up the chat takes longer.

Spot on!

Furthermore any edits made would capture an empty draft message in storage, which means, even though the message is empty Onyx would try to retrieve it from storage

Expensify.cash.-.Google.Chrome.2021-05-27.10-54-21.mp4

It's rather expensive to store and retrieve each action's draft message separately, why not store them:

• a) under a single map report_drafts_{reportID} -> { actionID: message } - the map would contain only messages that have a draft
• b) or keep it in the action item: message, originalMessage, draftMessage

---

This would be improved significantly by the Onyx optimization I'm working on - you can try it out on this branch: https://github.com/kidroca/Expensify.cash/tree/kidroca/sample-onyx-benchmark it's running v1.0.53
You might not need to revert the code if this works

[isagoico]

@JmillsExpensify Experienced some issues that seems related to this performance issue.

1. Back button is not responding after loading a long conversation.
2. Global create button is not responsive

I was unable to reproduce both issues on my device but I think it's because I don't have super long conversations in my testing accounts.

---

Linking the slack thread here. https://expensify.slack.com/archives/C01GTK53T8Q/p1622129132329200

[roryabraham]

It's rather expensive to store and retrieve each action's draft message separately, why not store them:

a) under a single map report_drafts_{reportID} -> { actionID: message } - the map would contain only messages that have a draft
b) or keep it in the action item: message, originalMessage, draftMessage

Both of these suggestions sound like good ideas to me – I think b would maybe lead to a simpler overall solution.

[roryabraham]

During the review of this PR, we found another performance improvement with regard to #2320 that can be made in addition to the ones suggested above.

Two ReportActionContextMenu components exist for each ReportActionItem, and each of them uses withOnyx to subscribe to the SESSION, which is only used right here. So that's a lot of Onyx subscribers! As part of this PR (which is almost ready), we're creating a separate library function in reportUtils.js called canEditReportAction which uses Onyx.connect directly to subscribe to the SESSION. So effectively, this should replace potentially hundreds of onyx subscriptions with just one. 👍

So maybe a good takeaway from this is that we should be careful when adding new Onyx subscriptions, particularly uses of withOnyx on low-level components that have many instances, and always try to think of ways we can eliminate duplicate subscriptions.

[roryabraham]

I don't want to submit a pull request without benchmarking, but I found another similar instance where we could remove duplicate Onyx subscriptions to try and improve overall app performance:

ThumbnailImage, AnchorWithAuthToken, and AttachmentModal all use withOnyx to subscribe to the session. In each component, the session prop is only used once to add the auth token to a URL using the addAuthTokenToURL library function. If we used Onyx.connect directly in that library function instead, we could reduce a number of duplicate Onyx subscriptions. Doing so could also simplify the implementation of AnchorForCommentsOnly

[parasharrajat]

@roryabraham, Although suggestion looks good but I feel that these optimization will be unnecessary after fixes here #2762.

[roryabraham]

Very excited for the Onyx improvements, and I agree my suggestion above will likely be negligible in comparison, but I'm wondering if people agree that in general it's a "best practice" to eliminate unnecessary duplicate Onyx subscriptions.

[parasharrajat]

If are following trend of Redux then it suggests to move the subscription down the tree and avoid passing props from Parent to Child. And it makes sense to me, whole point is to remove the props chain.

[roryabraham]

If are following trend of Redux then it suggests to move the subscription down the tree and avoid passing props from Parent to Child. And it makes sense to me, whole point is to remove the props chain.

Makes sense to me too! Let me clarify my suggested best practice:

"When there are many instances of a component (or components) that use withOnyx to subscribe to the same key, in order to then pass data to a common library function, it is better to use Onyx.connect directly in that library function and eliminate the unnecessary duplicate Onyx subscriptions."

In both of the examples I found, I'm not suggesting moving the subscription up the component tree. Instead, we're eliminating the use of withOnyx on a low-level component (creating many subscribers) in preference of a single subscription in a separate library.

[parasharrajat]

Ohk. Yeah that could be good overall.

[mallenexpensify]

Stopping by to say that it took ~2 mins to load a chat on iOS right now. (possibly unrelated... desktop just updated and is better/faster than in a LONG time).

[Jag96]

Left a comment on this PR with some results from testing, as @parasharrajat said earlier I think adding the Onyx cache is going to help a lot with speeding this up, but I agree we should still be mindful about duplicate onyx subscriptions.

[kidroca]

If are following trend of Redux then it suggests to move the subscription down the tree and avoid passing props from Parent to Child. And it makes sense to me, whole point is to remove the props chain.

That may be so but there are exceptions.
FlatList or any huge lists that render similar items - the item component should be as lean as possible
For each item withOnyx creates a new class component wrapping with life cycles, states, as well as the Onyx related callbacks and data - a lot of orchestration per each item

"When there are many instances of a component (or components) that use withOnyx to subscribe to the same key, in order to then pass data to a common library function, it is better to use Onyx.connect directly in that library function and eliminate the unnecessary duplicate Onyx subscriptions."

Keep in mind that each Onyx.connect call used in a library function is static and ATM would never release/disconnect

For example nothing is ever clearing allReports here
https://github.com/Expensify/Expensify.cash/blob/8b979de2cf00deafbba85c5c139f6d78cd1fd3f2/src/libs/actions/Report.js#L50-L58

Compared to data coming from a parent component - the parent component would release resources as well as the onyx connection when it unmounts
The ReportActionsList unmounts and remounts for each chat switch

It might not be a problem for a simple data like session and personalDetails, but if this approach becomes a pattern there might be problems

---

With all that said there a still room for improvement in how connect and withOnyx work, and virtualization has a big impact as well e.g. pre-render 200 items vs only 100.

---

Opened a ticket regarding further Onyx.connect improvements Expensify/react-native-onyx#78

[Jag96]

Looking at this thread it seems that the slowness issues on mobile have been alleviated. Going to close this, if we want to optimize subscriptions we can create a new issue, but I feel like between Expensify/react-native-onyx#76 and Expensify/react-native-onyx#78 we're ok for now.