[HOLD] [$16,000] [Performance] Loading the personalDetailsList takes a long time for some users

[marcaaron]

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?

The query to get personalDetails can be quite slow when you have access to tons of user details like huge accounts do. We have optimized this query several times. The main problem here is that the app blocks on loading these details when they aren't critical to launching the app.

What is the impact of this on end-users?

TTFB for Get&returnValueList=personalDetailsList is pretty bad which can lead to very slow initializing of the app.

List any benchmarks that show the severity of the issue

Log search:

David getting personalDetailsList - 5581ms 😱 (most I sampled today are closer to 1500ms but that is proof it can be QUITE slow) - Logs Marc getting personalDetailsList - 31 ms - Logs

Proposed solution (if any)

We should load personal details "on demand", and cache them in some fashion. I think we should do everything we can to never block app load on an API call unless it is critical (like signing in).

The way we could rework all this could be something like:

1. Remove the current API call
2. Add a new non-blocking API call that only gets the personal details for reports in LHN. When the data comes in, it is stored in Onyx.
   1. If you are in focus mode, it will load all people from LHN first, but if you're in most recent mode, it would start with only the top 50 reports (assuming that things below the fold can be updated last).
3. Add a new non-blocking API call that gets all the personal details (what is currently done). When the data comes in, it is stored in Onyx (it's fine to delay this for later, like 30 seconds later or something)
4. The LHN will always default to showing the basic login (email address or phone number) and default avatar. Whenever personal details are updated in Onyx, then the LHN automatically re-renders to show the names and avatars
5. Update all personal details every 30 minutes

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

We can ask David to test this change to see if the app loads faster for him.

Platform:

Where is this issue occurring?

• [x] Web
• [x] iOS
• [x] Android
• [x] Desktop App
• [x] Mobile Web

Version Number: All Logs: N/A Notes/Photos/Videos: N/A Expensify/Expensify Issue URL: N/A

View all open jobs on Upwork

[MelvinBot]

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

[mountiny]

As far as I know, that is the known mega query that takes super long. I won't be able to look into this as I will be ooo most of the time in the coming weeks. I will ping in Slack and also create a duplicate in Expensify repo so this does not fall through cracks based on the process we would like to possibly follow in future.

[mountiny]

Internal issue create here.

Asked in Slack here.

[flodnv]

The last optimization was made here: https://github.com/Expensify/Auth/pull/6111, more details can be found in this slack channel.

5581ms : that server was having known problems we're working on at that time, so this is not normal operation. 1500ms : we can try finding ways to make this faster, it'll be challenging but certainly not impossible. What is our target @marcaaron ?

This is a backend issue that disproportionately affects users with large numbers of reports.

How exactly does it affect users? Is it blocking app load or something like that? Are we trying to get all personal details of everyone "connected" to you? If yes, why?

[marcaaron]

Lots of great questions @flodnv. FWIW I'm just reporting anything that looks remotely suspect here.

It gets called very early in the app init

https://github.com/Expensify/App/blob/53581fef99e0df99f5c93a5fb5be0dbd89d3925b/src/libs/Navigation/AppNavigator/AuthScreens.js#L119-L119

And the data is used to populate the sidebar with names, avatars, etc.

How exactly does it affect users? Is it blocking app load or something like that?

Since this is not slow for me the test I performed (that anyone can also try) is to noop (or add a long setTimeout) to the function here to see how the app looks in the time before it loads and then test against the production API to observe any bad effects. It just shows a white screen for a while...

Are we trying to get all personal details of everyone "connected" to you? If yes, why?

This I don't know.

What is our target @marcaaron ?

I'm not sure about this either. There might be other solutions here we're not considering yet and there's room for creativity.

e.g. coming up with a more specific command that returns exactly what we need or splitting things up so that we get the entire personal details list late, but some much smaller subset of user details early (basically, load what is critical first then load everything else later).

[flodnv]

Yes, I was thinking that we should load personal details "on demand", and cache them in some fashion. I think we should do everything we can to never block app load on an API call unless it is critical (like signing in) -- would you agree with that premise @marcaaron @iwiznia @tgolen @AndrewGable ?

The way we could rework all this could be something like:

1. Get rid of this API call blocking the whole UI
2. On first fresh app load, load the personal details of all avatars we need to display in the LHN. Cache them in some way.
3. In the background, fetch all personal details of everyone (eg the API call removed in 1). Cache them in some way.
4. On the next app load, use the cached personal details -- they are already loaded, so they can't be blocking the app load.
5. When the app is loaded, go to (3)

Would something like this be doable?

Writing this out also makes me realize: if I update my avatar, when will you see the updated one? After you reload your app?

[marcaaron]

On first fresh app load, load the personal details of all avatars we need to display in the LHN. Cache them in some way.

To clarify, this does happen already. But only after you are signed in (subsequent loads / refreshes of the site are fast). But we can still make the first sign in faster by loading the critical personal details quickly.

if I update my avatar, when will you see the updated one? After you reload your app?

Correct, personal details do not update in realtime IIRC.

[flodnv]

we can still make the first sign in faster by loading the critical personal details quickly.

Ok so can we easily get this done from an external contributor? I think this is a much better route than trying to optimize the query that we already super optimized -- I am not sure how much more juice we can get out of this exercise. When you'll have DMs and reports and policies with thousands of people, it is currently kind of impossible to make it near instant.

[marcaaron]

Yes of course, please feel free to update the labels, description, add design notes or anything else you think would help. I don't have any specific ideas for a solution, but sounds like I achieved my goal of drawing attention to the problem.

[tgolen]

The personal details list refreshes every 30 minutes on a timer, so that's when you would get the updated avatar. I'm fine with everything else mentioned in this issue.

On Tue, Mar 1, 2022 at 4:27 PM Marc Glasser @.***> wrote:

Yes of course, please feel free to update the labels, description, add design notes or anything else you think would help. I don't have any specific ideas for a solution, but sounds like I achieved my goal of drawing attention to the problem.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/7942#issuecomment-1056005304, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAB72MAPYFL3HWQ7BRR3U52YV3ANCNFSM5PSGAQDQ . You are receiving this because you were mentioned.Message ID: @.***>

[iwiznia]

I agree with the plan. To clarify, nothing should block the app from loading, so 2 would be non blocking, right? LHN would render with emails first and then when personal details are loaded into onyx, the component would update and render with the names, right?

[MelvinBot]

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[flodnv]

@marcaaron @tgolen @iwiznia I have updated the issue description with what we decided, please check it to make sure we all agree on what we want to do.

[marcaaron]

Yes, sounds great to me!

[tgolen]

I read through it and I'm a little confused on a couple of points (assume that I agree with all the rest that I don't comment on).

Cache them in some way

This is mentioned twice, but it just means they will be put into Onyx, right?

On first fresh app load

How will we know that this is a "first fresh app load"? Does this just mean that the app has loaded and there is no data in Onyx? How will we know that?

On the next app load

I think this means something more like "if the app is loaded and data already exists in Onyx"? How will we know that?

When the app is loaded, go to (3)

This really has my head spinning and I am not sure how to resolve the confusion, but it's some weird loop where I don't know what state this belongs to (first free app load or next app load or some other app load)

I think I would simplify it like this:

1. Remove the current API call
2. Add a new non-blocking API call that only gets the personal details for reports in LHN. When the data comes in, it is stored in Onyx.
3. Add a new non-blocking API call that gets the personal details for reports NOT in the LHN. When the data comes in, it is stored in Onyx (it's fine to delay this for later, like 30 seconds later or something)
4. The LHN will always default to showing the basic login (email address or phone number) and default avatar. Whenever personal details are updated in Onyx, then the LHN automatically re-renders to show the names and avatars
5. Update all personal details every 30 minutes

Note: I am a bit concerned that this only improves performance if someone is in focus mode. If you are not in focus mode, then the LHN has all reports, and 1 would have ALL results while 3 has zero results.

[flodnv]

This is mentioned twice, but it just means they will be put into Onyx, right?

I assume so, yes (if that makes sense to you)

How will we know that this is a "first fresh app load"? Does this just mean that the app has loaded and there is no data in Onyx? How will we know that?

Good question. Maybe we can just change it to "on app load" and it will still work just as good (and maybe in a simpler way)?

Heh, yes, your updated proposal is great 👍 Editing the OP now.

If you are not in focus mode, then the LHN has all reports

Hmmm, not necessarily / not always. In "most recent" mode, it is not necessarily a guarantee that you have everyone's avatar displaying in the LHN. Even if you do (which I think is really unlikely), it will still improve performance of startup since the API call will be non blocking.

[tgolen]

Maybe if you are in focus mode, it would load all people from LHN first, but if you're in most recent mode, it would start with only the top 10 reports or something (assuming that things below the fold can be updated last).

[quinthar]

FYI, I'm in focus mode.

On Tue, Mar 29, 2022 at 12:39 PM Tim Golen @.***> wrote:

Maybe if you are in focus mode, it would load all people from LHN first, but if you're in most recent mode, it would start with only the top 10 reports or something (assuming that things below the fold can be updated last).

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/7942#issuecomment-1082300821, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUTHW6L4HN7LZHPIQQ3VCNL6TANCNFSM5PSGAQDQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[iwiznia]

I don't get why the new step 3 loads details of reports not in the LHN instead of all details. Especially, keeping in mind that we want all details even of people were you share no reports with (so their data shows when starting a new chat for example)

[tgolen]

My main goal was to be sure step 3 doesn't want to repeat the same data requested in step 2. Rather than doing a subset of everything and then everything, I think it should be requesting a subset, and then requesting everything else that is not in that subset.

[iwiznia]

I think it should be requesting a subset, and then requesting everything else that is not in that subset.

I am fine with that, just note it's not what's written in the proposal. Also, maybe returning everything not in the subset is more complex or takes more time than just retrieving everything, so if it is, then let's just return everything.

[flodnv]

Maybe if you are in focus mode, it would load all people from LHN first, but if you're in most recent mode, it would start with only the top 10 reports or something (assuming that things below the fold can be updated last).

Agreed, adding this.

I am fine with that, just note it's not what's written in the proposal. Also, maybe returning everything not in the subset is more complex or takes more time than just retrieving everything, so if it is, then let's just return everything.

I agree returning everything is less error prone (and probably simpler) than the other option, updating this.

[adelekennedy]

@iwiznia @flodnv @tgolen Should this still be an external issue? Ready to open an upwork job if contributors should work on this!

[flodnv]

I think yes after @iwiznia & @tgolen give their 👍

[tgolen]

Yeah, I think it is OK to be external. There are no API changes that need to be done on the server for this proposal.

[iwiznia]

👍 from me, but don't we need a new api command or param to only return details of people in the LHN for step 2?

[flodnv]

I think we can use PersonalDetails_GetForEmails

[tgolen]

Yeah, I was assuming that the front-end would just be passing whatever emails it wants to get the details for.

[tgolen]

However, the thought of moving all that logic to Auth is really a good idea and I think that would be awesome!

[iwiznia]

Oh, did not think of using PersonalDetails_GetForEmails! I am unsure if that's worse than moving the logic to auth, in the end, the frontend is what decides what's in the LHN (based on the mode you have set), so it makes sense to me that it is the one responsible for asking the details of users it has in view.

[tgolen]

Yeah, I think that view mode is set as an NVP though, so I think Auth would still be able to know what should be in the LHN or not.

On Thu, Mar 31, 2022 at 1:03 PM Ionatan Wiznia @.***> wrote:

Oh, did not think of using PersonalDetails_GetForEmails! I am unsure if that's worse than moving the logic to auth, in the end, the frontend is what decides what's in the LHN (based on the mode you have set), so it makes sense to me that it is the one responsible for asking the details of users it has in view.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/7942#issuecomment-1084993475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAB3HWFSZOMT3XV6JQCLVCXZIRANCNFSM5PSGAQDQ . You are receiving this because you were mentioned.Message ID: @.***>

[iwiznia]

I see, anyway, I am not sure which one is better than the other, I think I lean slightly to the frontend as it feels more correct to be handled there

[adelekennedy]

thanks all! I'll open the job at $250 to start, open to go higher if we need!

[adelekennedy]

Internal External

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[melvin-bot[bot]]

Triggered auto assignment to @roryabraham (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Santhosh-Sellavel]

@roryabraham Can we double this one it's been 10days?

cc: @adelekennedy

[roryabraham]

Yes, I think we can double this

[adelekennedy]

Just got the Slack ping to double, updating now!

[adelekennedy]

doubling again - waiting for proposals

[adelekennedy]

still no proposals - moving forward to double

[Santhosh-Sellavel]

Waiting for a proposal!

[adelekennedy]

Doubling time

[adelekennedy]

and it's time to double again 😅 should this be an external issue @roryabraham? It seems like it's taking a while to get any bites

[adelekennedy]

we're at the doubling time 🤑 checking here do we have a sense why this isn't getting any bites?

[adelekennedy]

@roryabraham @Santhosh-Sellavel what are we thinking here - it seems like we're doubling this over and over

[iwiznia]

Maybe we should hold on this? We will be working towards basically refactoring all API commands and this would be included. We even talked about having one main API command that downloads all data needed on startup (like this one)

[Santhosh-Sellavel]

I think we shouldn't have double this one yet @adelekennedy. If possible should you down the JOB price to 8000$ itself it should be more than enough I think! cc: @roryabraham.

[adelekennedy]

should we put this on hold as @iwiznia suggested? at 8k we're still not getting any traction (10 days without activity) which makes me think the issue itself is too challenging or blocked at this point