Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.84k forks source link

[Performance] Cold start time is slow on Android #4027

Closed marcaaron closed 3 years ago

marcaaron commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Log into the app
  2. Kill the app
  3. Start up the app again

Expected Result:

App should take a reasonable amount of time to boot back up

Actual Result:

App takes too long to boot up

Proposals (PLEASE READ)

This issue mainly affects native platforms. We don't have a specific target in mind of "how long it should take" and of course this will vary from device to device. But any proposals to this issue should provide clear evidence that the proposal will reduce boot time / cold start.

Workaround:

N/A

Platform:

iOS Android

Version Number: Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL:

View all open jobs on Upwork

MelvinBot commented 3 years ago

Triggered auto assignment to @JmillsExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

marcaaron commented 3 years ago

@JmillsExpensify curious if you think it'd be valuable to maybe have a goal in mind for this and generally base that on some of our most used apps and how long they take to start? Otherwise, I think going for "general improvements" to boot time can work, but there are probably many factors that can move us closer to where we want to be.

marcaaron commented 3 years ago

Some conversation here on RAM bundles as it was suggested by @jsamr that we can use Repack for cross platform lazy loading of modules. This could in theory reduce the app start time. I like this idea, but needs some more investigation :)

JmillsExpensify commented 3 years ago

@marcaaron Catching up here. I'm with you. I think we should be aiming for comparison with best-in-class. Slack and WhatsApp are the favorites, but I'm sure there are others we could include if needed.

Then separately, posting on Upwork now.

JmillsExpensify commented 3 years ago

Hi contributors! If you are coming from our Upwork posting, welcome! Make sure to post your proposal in this issue and then someone from our team will approve so we can hire you. Thanks!

MelvinBot commented 3 years ago

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

rushatgabhane commented 3 years ago

We don't have a specific target in mind of "how long it should take" and of course this will vary from device to device

I was going through this research paper. If a screen refreshes within 1 second, then the users perceive it as fast. image

Our goal for boot time, for a flagship phone should be <1 second.

marcaaron commented 3 years ago

1 second seems like a good target to me. Did a few tests on Android, but pretty much tells us what we already know... 😅

App Cold Launch Time
Gmail < 1 second
Cash app < 1 second
WhatsApp < 1 second
Expensify.cash ~ 5 seconds
marcaaron commented 3 years ago

Ideas for more actionable issues we can breakout...

  1. Start benchmarking the cold start time so that we have a better idea of how long it takes (some ideas here)] TL;DR use Firebase.
  2. Improve code splitting
  3. Audit the amount of JS that is executing when the app first boots.
  4. Maybe defer some of that executing to the native layer?
  5. Seems like Peter's Onyx improvements might also help somewhat which means it is important to get set up with 1
marcaaron commented 3 years ago

@NikkiWines @JmillsExpensify Since we haven't gotten much action I'm going to close this Upwork posting so I can start investigating the plan laid out above. I think this is one of the most important things we can look at perf wise.

marcaaron commented 3 years ago

Looking into setting up a regular benchmark via Firebase that will record:

  1. Time it takes to boot the app (automatic with Firebase Performance package)
  2. Time it takes from launch til JS to be evaluated - can be set up easily with a native module

https://github.com/Expensify/Expensify/issues/171518

marcaaron commented 3 years ago

Firebase Performance is hooked up now (not yet on staging but will be soon and data should start to hit this dashboard)

2021-07-22_09-03-32

Still waiting for an iPhone to test on locally, but did a quick output of pre-main time in simulator for now (probably not super accurate, but still seems like there is no obvious issue there)

Total pre-main time: 376.31 milliseconds (100.0%)
         dylib loading time: 170.80 milliseconds (45.3%)
        rebase/binding time:  54.05 milliseconds (14.3%)
            ObjC setup time:  50.47 milliseconds (13.4%)
           initializer time: 100.97 milliseconds (26.8%)
           slowest intializers :
             libSystem.B.dylib :  10.76 milliseconds (2.8%)
                Expensify.cash : 112.61 milliseconds (29.9%)

I'm not an expert, but this seems to suggest that the app is launching fast, but takes a lot longer to become interactive - which makes sense. Will look to the JS next.

marcaaron commented 3 years ago

The going is a bit slow since to properly measure the impacts of a change on start up time we have to create release build locally - the real startup time is hard to measure when in "dev" mode because the JS bundle is served by metro and must be downloaded. Also still working with simulator on iOS.

My investigations are still early here, but chased a few leads...

Use Hermes on iOS

Apparently Hermes is enabled for iOS as of react-native 0.64, but we aren't using it yet.

enabling Hermes will result in improved start-up time, decreased memory usage, and smaller app size

On the one hand, this is a one line change so might be worth a shot. But on the other, there could be some unexpected consequences of switching to a different JS engine. Ultimately, we are using it on Android so probably we should use it on both platforms for consistency.

We are maybe "doing too much" when the app first boots ::waves hands::

Some random observations that might be worth looking into:

Other stuff / dead ends

Screen Shot 2021-07-22 at 1 28 15 PM

Screen Shot 2021-07-22 at 1 33 14 PM Screen Shot 2021-07-22 at 1 32 50 PM

Still have to look into...

marcaaron commented 3 years ago

Gonna send a PR to switch iOS over to Hermes initial testing looks promising! Not a silver bullet, but definitely improved boot time. The hermes build is on the right and was tested on an iPhone 8 against the production API.

https://user-images.githubusercontent.com/32969087/126815732-f8f8cedc-9cbd-4ffa-a66f-627e500ae439.mp4

AndrewGable commented 3 years ago

Wow- Looks great!

quinthar commented 3 years ago

Wow, nice!

On Fri, Jul 23, 2021 at 9:57 AM Marc Glasser @.***> wrote:

Gonna send a PR to switch iOS over to Hermes initial testing looks promising! Not a silver bullet, but definitely improved boot time. The hermes build is on the right and was tested on an iPhone 8 against the production API.

https://user-images.githubusercontent.com/32969087/126815732-f8f8cedc-9cbd-4ffa-a66f-627e500ae439.mp4

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-885772254, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUWKR7WUUWVHBNXJ5ULTZGNOXANCNFSM5AKOQNRQ .

MelvinBot commented 3 years ago

@marcaaron Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 3 years ago

Switching focus to Android startup time today. Did some testing over the weekend with both new staging versions and Android startup time feels much slower than iOS. Testing was done on an iPhone 8 compared and Galaxy S8. The galaxy isn't as powerful as the iPhone, but I wouldn't expect this much of a difference (about 9 seconds longer for the LHN to show on Android).

Most of the difference seems to be after the splash screen finishes loading - so the app itself is actually starting up pretty quickly (i.e. native layer + JS load), but whatever we do after that makes the UI hang for ages.

I'm looking into what could be making things take so long, but my current working theory is that we front load the initial load of the app with too many API requests. I've had some suspicions about this in the past, but going to try to build some release apks and do some benchmarks.

quinthar commented 3 years ago

Can't wait!

On Mon, Jul 26, 2021, 12:17 PM Marc Glasser @.***> wrote:

Switching focus to Android startup time today. Did some testing over the weekend with both new staging versions and Android startup time feels much slower than iOS. Testing was done on an iPhone 8 compared and Galaxy S8. The galaxy isn't as powerful as the iPhone, but I wouldn't expect this much of a difference (about 9 seconds longer for the LHN to show on Android).

Most of the difference seems to be after the splash screen finishes loading - so the app itself is actually starting up pretty quickly (i.e. native layer + JS load), but whatever we do after that makes the UI hang for ages.

I'm looking into what could be making things take so long, but my current working theory is that we front load the initial load of the app with too many API requests. I've had some suspicions about this in the past, but going to try to build some release apks and do some benchmarks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-886960906, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNURY26GICQ2NQEGDJY3TZWYETANCNFSM5AKOQNRQ .

marcaaron commented 3 years ago

I think I made a breakthrough here. Here's what I tried...

  1. Deferred a bunch of less critical front loaded API calls by about 500ms so that we can prioritize the "first paint" and just get something on the screen as quickly as possible
  2. Deferred the loading of the Report Screen (which actually loads off screen anyway in parallel) until after the Sidebar loads

Here's the before and after (left side is the changes made above - right side is staging)

https://user-images.githubusercontent.com/32969087/127048918-589973b6-1900-4a80-9b78-2db407212dca.mp4

quinthar commented 3 years ago

Wow, that's huge!!! Is there a way to eliminate that 500ms "magic number" and somehow more explicitly delay until after "first paint"? (A perfectly round 500 is probably not the ideal number, or at least, won't stay ideal on all platforms forever.)

On Mon, Jul 26, 2021 at 12:48 PM Marc Glasser @.***> wrote:

I think I made a breakthrough here. Here's what I tried...

  1. Deferred a bunch of less critical front loaded API calls by about 500ms so that we can prioritize the "first paint" and just get something on the screen as quickly as possible
  2. Deferred the loading of the Report Screen (which actually loads off screen anyway in parallel) until after the Sidebar loads

Here's the before and after (left side is the changes made above - right side is staging)

https://user-images.githubusercontent.com/32969087/127048918-589973b6-1900-4a80-9b78-2db407212dca.mp4

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-886978810, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUVZCNXNOK7NLXOKJSLTZW3XLANCNFSM5AKOQNRQ .

marcaaron commented 3 years ago

Agree, that was just a placeholder. I'm thinking we can make it event based instead. So e.g. once the sidebar has loaded it can emit an event and we can kick off any non-critical requests.

marcaaron commented 3 years ago

Edit: I mean non-mission critical requests :)

marcaaron commented 3 years ago

Ok actually I got excited about the API theory too soon and missed that delaying the API calls was NOT what caused the reduced load time. Delaying the report screen was actually the thing that did it.

I spent more time today benchmarking various minor tweaks on release builds only and a major chunk of the slow boot time is just time spent waiting for the report to load at the same time as the Sidebar.

So, if we defer loading the report until the sidebar has loaded then we can save a bit of time. Basically, it feels like our "slow cold start" problem is directly related to our "slow chat switch" problem. We're just also experiencing the "slow chat switch" when the app boots up.

quinthar commented 3 years ago

Haha that seems like a pretty obvious theory now that you say it.

On Tue, Jul 27, 2021, 9:31 PM Marc Glasser @.***> wrote:

Ok actually I got excited about the API theory too soon and missed that delaying the API calls was NOT what caused the reduced load time. Delaying the report screen was actually the thing that did it.

I spent more time today benchmarking various minor tweaks on release builds only and a major chunk of the slow boot time is just time spent waiting for the report to load at the same time as the Sidebar.

So, if we defer loading the report until the sidebar has loaded then we can save a bit of time. Basically, it feels like our "slow cold start" problem is directly related to our "slow chat switch" problem. We're just also experiencing the "slow chat switch" when the app boots up.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-888001138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNURYLVG2V4UY7R3QS33TZ6B2NANCNFSM5AKOQNRQ .

MelvinBot commented 3 years ago

@marcaaron Eep! 4 days overdue now. Issues have feelings too...

marcaaron commented 3 years ago

Going to update this issue to focus only on Android. I think at this point iOS is cold booting very fast.

Check this comparison...

iOS (iPhone 8)

https://user-images.githubusercontent.com/32969087/127958331-bb54c6cb-082e-445e-a716-409114c45b5b.mov

Android (Galaxy S8)

https://user-images.githubusercontent.com/32969087/127958361-e0f1e455-1cc2-42b4-92ba-411e86ddc651.mp4

Android is faster than it's ever been, but still feels like it hangs a bit compared to other apps on my device.

quinthar commented 3 years ago

Wow, so jealous for iOS! Let's get Android to that!

On Mon, Aug 2, 2021 at 9:35 PM Marc Glasser @.***> wrote:

Going to update this issue to focus only on Android. I think at this point iOS is cold booting very fast.

Check this comparison...

iOS (iPhone 8)

https://user-images.githubusercontent.com/32969087/127958331-bb54c6cb-082e-445e-a716-409114c45b5b.mov

Android (Galaxy S8)

https://user-images.githubusercontent.com/32969087/127958361-e0f1e455-1cc2-42b4-92ba-411e86ddc651.mp4

Android is faster than it's ever been, but still feels like it hangs a bit compared to other apps on my device.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-891516681, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUW2SSV4S4I5DMAHSELT25WYPANCNFSM5AKOQNRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

marcaaron commented 3 years ago

Did a quick experiment today to see if enabling Proguard on Android would help (noticed it was not actually enabled despite the presence of a recently edited config file). It didn't do too much (if anything) so I don't think optimizing the build is going to have much of an effect.

I'm not entirely sure what to look into next - but coming around to "it's an Android storage problem". Ran a bunch more benchmarking with release builds to try to identify if there are any bottlenecks in the code (specifically around Onyx connected components that are blocked while getting data from Onyx). Which seems to suggest that Onyx performance is not equivalent when comparing Android to iOS (times in ms).

Time spent waiting for data via Onyx Android iOS
Expensify.js 263 40
AuthScreens.js 17 2
MainDrawerNavigator.js 77 20
SidebarLinks.js 70 21
SidebarScreen.js 38 13
Total Blocking Time 465 96

It feels like we're just trying to read a bunch of data at once and then end up blocking all these components from rendering until a parent component finishes getting what it needs. The effects of that aren't felt as much on iOS for whatever reason.

quinthar commented 3 years ago

Did we upgrade our use of Sqlite to something that seems more optimized?

On Tue, Aug 3, 2021, 5:41 PM Marc Glasser @.***> wrote:

Did a quick experiment today to see if enabling Proguard on Android https://developer.android.com/studio/build/shrink-code would help (noticed it was not actually enabled despite the presence of a recently edited config file). It didn't do too much (if anything) so I don't think optimizing the build is going to have much of an effect.

I'm not entirely sure what to look into next - but coming around to "it's an Android storage problem". Ran a bunch more benchmarking with release builds to try to identify if there are any bottlenecks in the code (specifically around Onyx connected components that are blocked while getting data from Onyx). Which seems to suggest that Onyx performance is not equivalent when comparing Android to iOS (times in ms). Time spent waiting for data via Onyx Android iOS Expensify.js 263 40 AuthScreens.js 17 2 MainDrawerNavigator.js 77 20 SidebarLinks.js 70 21 SidebarScreen.js 38 13 Total Blocking Time 465 96

It feels like we're just trying to read a bunch of data at once and then end up blocking all these components from rendering until a parent component finishes getting what it needs. The effects of that aren't felt as much on iOS for whatever reason.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-892267808, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUQYJBQ66W5VZXPMKD3T3CEDNANCNFSM5AKOQNRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

marcaaron commented 3 years ago

Not yet! It could be the next thing to look into. Unsure how large of an effort that will be, but sounds like we might want something fairly custom based on the Slack conversations.

One idea could be to do a quick test and set up react-native-mmkv in Onyx. I am not suggesting we use it as it sounds like we want to RYO. But, it claims to be about 30x faster than AsyncStorage + uses the JSI instead of the bridge. If that's true, it might be a simple way to gauge whether it's worth investing in a custom storage solution over AsyncStorage. If we hook it up and all our performance issues go away then we'll know we're heading in the right direction.

quinthar commented 3 years ago

Ya, I still don't think any of those claims/tests actually mean anything. I'd much rather just improve what we've got using whatever supposed advantages mmkv has (because I think the raw storage performance of mmkv vs SQLite is equivalent -- and nearly instant -- and the only difference would be in how it manages the bridge).

On Wed, Aug 4, 2021 at 12:39 PM Marc Glasser @.***> wrote:

Not yet! It could be the next thing to look into. Unsure how large of an effort that will be, but sounds like we might want something fairly custom based on the Slack conversations.

One idea could be to do a quick test and set up react-native-mmkv https://github.com/mrousavy/react-native-mmkv in Onyx. I am not suggesting we use it as it sounds like we want to RYO. But, it claims to be about 30x faster than AsyncStorage + uses the JSI instead of the bridge. If that's true, it might be a simple way to gauge whether it's worth investing in a custom storage solution over AsyncStorage. If we hook it up and all our performance issues go away then we'll know we're heading in the right direction.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-892922359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNURD4RL7VFKEU66M3BLT3GJN7ANCNFSM5AKOQNRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

marcaaron commented 3 years ago

This is branching off of this comment and @kidroca's observations that a lot of keys are fetched when the app inits. While we are looking into optimizing storage stuff it still feels like some opportunities for optimization at a higher level is possible...

Stop reading report actions from storage on init

In this video, the left side skips fetching the report actions from storage on init

https://user-images.githubusercontent.com/32969087/128405509-b0acad7a-fa47-444d-bbc3-028b0b063cae.mp4

Stop reading reports on init?

The next thing I'd wonder is whether we really need to load all the reports from storage immediately on init e.g. it might be better to cache / store only what we need to build the LHN in a single object vs. aggregating a ton of individual report objects to create the sidebar, main drawer navigator, etc.

This one might be tricky to work out and I'm not sure if it's valuable yet but all the data we need for the LHN comes from the Get&returnValueList=reportSummaryList call. We currently take that data and store it on individual report objects, but maybe that data structure isn't serving us well when the app inits since we need to fetch something like 200+ keys instead of just 1 (Note: Just speculating here - haven't tested this theory yet).

Looking forward, the LHN will also need to be paginated in the future when reports can be in the tens of thousands so perhaps it makes sense to store a reportSummaries key instead of aggregating the many report keys.

quinthar commented 3 years ago

hilarious idea: what if we captured a screenshot of the app upon close, and then just... showed that whole screenshot when you open the app, and while we're sorting out the background? It would give the impression of super instant responsiveness.

Regardless, I think it's more important to open fast than to update the network fast. (Both matter, but I'd rather have a usable app that is out of date, than an unusable app that is ... still out of date.)

On Thu, Aug 5, 2021 at 12:23 PM Marc Glasser @.***> wrote:

This is branching off of this comment https://github.com/Expensify/react-native-onyx/issues/84#issuecomment-893709138 and @kidroca https://github.com/kidroca's observations that a lot of keys are fetched when the app inits. While we are looking into optimizing storage stuff it still feels like some opportunities for optimization at a higher level is possible...

Stop reading report actions from storage on init

  • We fetch all the report actions from storage when the app inits and probably can come up with a way to avoid that. They are the largest / most complex items we have. And we really only do this as a network optimization to compare the last message in storage with the latest on the report to see if there's anything we need to "catch up" on - but we don't really need this to happen immediately when the app inits. We might not need to do this at all if we "catch up" when navigating to a report - or another option would be to push this work to a less critical time.

    https://github.com/Expensify/App/blob/e88b7f984ab4e89e33844260c3a334622bf23023/src/libs/actions/ReportActions.js#L20-L22

In this video, the left side skips fetching the report actions from storage on init

https://user-images.githubusercontent.com/32969087/128405509-b0acad7a-fa47-444d-bbc3-028b0b063cae.mp4

Stop reading reports on init?

The next thing I'd wonder is whether we really need to load all the reports from storage immediately on init e.g. it might be better to cache / store only what we need to build the LHN in a single object vs. aggregating a ton of individual report objects to create the sidebar, main drawer navigator, etc.

This one might be tricky to work out and I'm not sure if it's valuable yet but all the data we need for the LHN comes from the Get&returnValueList=reportSummaryList call. We currently take that data and store it on individual report objects, but maybe that data structure isn't serving us well when the app inits since we need to fetch something like 200+ keys instead of just 1 (Note: Just speculating here - haven't tested this theory yet).

Looking forward, the LHN will also need to be paginated in the future when reports can be in the tens of thousands so perhaps it makes sense to store a reportSummaries key instead of aggregating the many report keys.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-893723847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNURM6PZCRJICUCBGZTLT3LQLRANCNFSM5AKOQNRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

marcaaron commented 3 years ago

hilarious idea: what if we captured a screenshot of the app upon close, and then just... showed that whole screenshot when you open the app, and while we're sorting out the background? It would give the impression of super instant responsiveness.

LOL, but maybe something like it? I think we can just...

The report summary list (if we start storing a minimal version of it) should tell us something about the order in which reports need to be shown then we can stagger the loading and rendering of anything not in view.

quinthar commented 3 years ago

I totally agree we can wait to read the 190 hidden chats until we switch to them. If the only reason we do it is to figure out how to sync, let's find a way to do that in a more optimized way -- maybe a single object that has the most recent comment from each, that we can create an API to receive as a single API call.

On Thu, Aug 5, 2021 at 12:47 PM Marc Glasser @.***> wrote:

hilarious idea: what if we captured a screenshot of the app upon close, and then just... showed that whole screenshot when you open the app, and while we're sorting out the background? It would give the impression of super instant responsiveness.

LOL, but maybe something like it? I think we can just...

  • load the minimum amount of data required to create the LHN (e.g. only about 8 chats are visible for me on init on Android so why bother loading another 190 that are totally hidden from view?)
  • continue retrieving the rest of the reports after that happens

The report summary list (if we start storing a minimal version of it) should tell us something about the order in which reports need to be shown then we can stagger the loading and rendering of anything not in view.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/4027#issuecomment-893742581, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUVUYNBN6YCXVUBVUITT3LTEVANCNFSM5AKOQNRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

kidroca commented 3 years ago

One more thing that can be optimized is the chat loaded under the LHN On web/desktop you see LHN and you last chat But on mobile this chat is loaded while you don't see it at all

It can be lazy loaded instead - either load/pre-render it after LHN is done, or load it when it is selected (only on small screens though)

marcaaron commented 3 years ago

It can be lazy loaded instead

Recently added some code to load this only after the sidebar is loaded

MelvinBot commented 3 years ago

@marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick!

MelvinBot commented 3 years ago

@marcaaron Eep! 4 days overdue now. Issues have feelings too...

marcaaron commented 3 years ago

Still looking into this. Have a draft that reduces the number of reportActions_* keys on init that I hope will help boot time.

marcaaron commented 3 years ago

Grabbed a JS profile on startup for Android and just leaving a breakdown of what is happening in the time between metro loading the bundle and the app displaying the sidebar...

Total time ~ 2.5 - 3 seconds

2021-08-12_09-21-16

Going block by block...

271 ms - renderApplication.. I think this is just react native starting…

211 ms - Looks like Onyx doing something there are some getItem calls - probably this is reading from AsyncStorage

184 ms - About half of this time is spent parsing JSON the other half rendering something (probably the Expensify.js component)

60 ms - SafeAreaProvider renders

74 ms - HTML render engine provider

128 ms - I’m a little unsure what this block of time is doing… invokeCallbackAndReturnFlushedQueue, something with callTimer. Those methods indicate that we are firing a callback in a JS timer (which requires some communication over the bridge - but doesn't look like anything really happens - maybe it's the network request queue doing the tick it does each second)

124 ms - This block looks pretty similar to the previous except there is a JSON parse which makes me think we have a network request.

172 ms - Navigation Container renders in this block

612 ms - Largest block here.. we can see several “sendDataToConnection” calls so we are rendering the MainDrawerNavigator, DrawerView, Header, etc - react navigation stuff.

431 ms - The Sidebar is rendering in this block

216 ms - The sidebar seems to render again for some reason (probably it doesn’t need to)

At this point the initial sidebar is “rendered” to the screen.

Here's the profile: sampling-profiler-trace4670664525343774149-converted.json.zip


Having a couple thoughts...

  1. We can try to limit the sidebar from rendering again and save that 200ms (not much but it's something)
  2. The react-navigation stuff is taking a pretty long time to init together with the sidebar (about 1 second) so anything we can do to make that faster would be good.

It would be nice to compare this to a profile on iOS and see whether the JS there is taking a similar amount of time. But it seems like profiling Hermes on iOS doesn't work at all.

marcaaron commented 3 years ago

I'm going to close out this issue and create a new one so we can get some more eyes on the problem. Now that there is a reliable way to test changes I think this would be better as a rolling job where we accept all proposals that are clear improvements (something like 500ms or more and can be verified by anyone).

kidroca commented 3 years ago

BTW I've tried removing screens/components to see how much different components affect loading, but I used the Timing:... logs

I get this after a fresh login and then app restart

Timing:expensify.cash.homepage_reports_loaded 5396

I've removed all the navigators and screens besidese LHN and the ReportScreen and the time didn't change much

The time dropped to 2833 only after I've removed the whole ReportScreen

Timing:expensify.cash.homepage_reports_loaded 2833

Even then I've seen the LHN maybe about a second earlier

I'll try to run the same with the new performance metrics and post on the new ticket This should give an idea on how much is there to be gained

marcaaron commented 3 years ago

Not sure if it's relevant but that particular timing log only completes when an API request finishes -> https://github.com/Expensify/App/blob/69ee2fe21bf825318a9e717096293aedc51bda4a/src/libs/actions/Report.js#L943