-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Performance] Cold start time is slow on Android #4027
Comments
Triggered auto assignment to @JmillsExpensify ( |
@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. |
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 :) |
@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. |
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! |
Triggered auto assignment to @NikkiWines ( |
I was going through this research paper. Our goal for boot time, for a flagship phone should be <1 second. |
1 second seems like a good target to me. Did a few tests on Android, but pretty much tells us what we already know... 😅
|
Ideas for more actionable issues we can breakout...
|
@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. |
Looking into setting up a regular benchmark via Firebase that will record:
https://github.com/Expensify/Expensify/issues/171518 |
Firebase Performance is hooked up now (not yet on staging but will be soon and data should start to hit this dashboard) 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)
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. |
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
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
Still have to look into...
|
@marcaaron Eep! 4 days overdue now. Issues have feelings too... |
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) iOS.Cold.Start.-.after.improvements.movAndroid (Galaxy S8) Android.-.Cold.Boot.-.After.Improvements.mp4Android is faster than it's ever been, but still feels like it hangs a bit compared to other apps on my device. |
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
<#4027 (comment)>, 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>
.
|
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
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. |
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
<#4027 (comment)>, 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>
.
|
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 |
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
<#4027 (comment)>, 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>
.
|
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 without.report.actions.mp4Stop 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 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 |
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
<Expensify/react-native-onyx#84 (comment)>
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
<#4027 (comment)>, 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>
.
|
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. |
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
<#4027 (comment)>, 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>
.
|
One more thing that can be optimized is the chat loaded under the LHN 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) |
Recently added some code to load this only after the sidebar is loaded |
@marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@marcaaron Eep! 4 days overdue now. Issues have feelings too... |
Still looking into this. Have a draft that reduces the number of |
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 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: Having a couple thoughts...
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. |
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). |
BTW I've tried removing screens/components to see how much different components affect loading, but I used the I get this after a fresh login and then app restart
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
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 |
Not sure if it's relevant but that particular timing log only completes when an API request finishes -> App/src/libs/actions/Report.js Line 943 in 69ee2fe
|
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:
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
The text was updated successfully, but these errors were encountered: