[Tracking] Callstack performance audit of the App

[mountiny]

Tracking issue for @adhorodyski and Callstack team performing performance audit of the app.

This is a performance audit of the Expensify app done by Callstack, following the DMAIC methodology. It will consist of 5 phases and result in a report summarising the whole audit.

In the first phase we have defined the following metrics:

• APK size in MB (App Size)
• JS bundle size in MB (App Size)
• App start time in MS (TTI)
• Report screen load time in MS (TTI)
• Sending a message in MS (TTI)
• Average FPS on LHN (FPS)
• Average FPS on Report screen load (FPS, CPU usage)
• Search screen load time in MS (TTI)

[adhorodyski]

Adam from Callstack here, I'd like to work on this.

[adhorodyski]

This is a performance audit of the Expensify app done by Callstack, following the DMAIC methodology. It will consist of 5 phases and result in a report summarising the whole audit.

In the first phase we have defined the following metrics:

• APK size in MB (App Size)
• JS bundle size in MB (App Size)
• App start time in MS (TTI)
• Report screen load time in MS (TTI)
• Sending a message in MS (TTI)
• Average FPS on LHN (FPS)
• Average FPS on Report screen load (FPS, CPU usage)
• Search screen load time in MS (TTI)

[adhorodyski]

Current status:

• @adhorodyski is working on the JS bundle size in MB metric
• @hurali97 is working on the APK size in MB metric

We've conducted the measurements (will post shortly) and are finishing up analysing them, you can expect separate comments with the findings.

[adhorodyski]

Metric: JS bundle size in MB
Phase: Measurements
Commit hash: c09a3fe

Platform	Size	Command	Artifact
Android	10.34 MB	npx react-native-bundle-visualizer@3.1.0 --platform android
iOS	10.92 MB	npx react-native-bundle-visualizer@3.1.0 --platform ios

[adhorodyski]

Metric: JS bundle size in MB
Phase: Analysis
Commit hash: c09a3fe

Below is the breakdown of our findings from the analysis process. To sum this up, if we take the associated actions the JS bundle size can be reduced by at least ~11%.

Name	Platforms	Before	After	% Impact	Notes	Actions
date-fns locale imports	All	915 KB	15 KB	8%	Locales are being imported from the date-fns/locale module using named imports. This causes unused ones to end up in the bundle.	Importing only 'en-GB' and 'es' separately from their respective paths, eg. date-fns/locale/es to only include the ones in use.
@formatjs polyfills	Native	353 KB	0 B	3%	Hermes ships Intl polyfills starting from RN 0.65 on Android and 0.70 on iOS.	For native platforms, polyfills bundled with Hermes can be used.
underscore	All	20 KB	0 B	-	Both lodash and underscore utilities are being used.	Underscore can be removed in favor of lodash.
assets/images	All	Android - 1.93 MB, iOS - 2.57 MB	-	-	High res (svg), but high size images are being used regardless the usecase.	Static assets that are not being animated can be optimised by serving in other, more optimised formats.
assets/emojis	All	425 KB	-	-	Current approach to emojis is custom made (maintenance and size cost).	Custom code can be replaced with the node-emoji dependency. A contribution to emojilib will be needed to support ‘es’ locale.

Unused / deprecated / vulnerable dependencies

Additionally, some unused/deprecated/vulnerable dependencies have been identified in the process.

Name	Notes	Actions
npm audit	49 vulnerabilities (26 moderate, 21 high, 2 critical)	Apply npm audit fix to fix vulnerabilities.
unused dependencies	-	Remove: @react-native-firebase/analytics, fbjs, save, react-native-image-pan-zoom, react-native-image-size
unused devDependencies	-	Remove: @babel/plugin-proposal-export-namespace-from, @vercel/ncc, diff-so-fancy, react-native-clean-project (replace with RN CLI clean)
metro-react-native-babel-preset	Soon to be deprecated plugin is being used.	Swap metro-react-native-babel-preset with @react-native/babel-preset before migrating to RN@0.73+.

[adhorodyski]

@mountiny Happy 2024!! 😊
Please feel free to invite anyone to the discussion here - we can now pick prioritised items from the list above to take actions right now or decide what's already taking place/can be postponed. We'd follow the same for all the metrics :)

[hurali97]

Metric: APK Size in MB
Phase: Measurement
Commit hash: 6365aeb

Measurements:

Baseline:

We gathered the following metrics on baseline ( without any improvements ) :

	APK Size MB	Download Size MB
All architectures	126.6	123

These metrics are gathered by generating a release APK using ENVFILE=.env.production ./gradlew assembleProductionRelease and then opening that APK in the APK Analyzer in Android Studio.

• All architectures means arm64-v8a, armeabi-v7a, x86_64 and x86. By default an APK has all these 4 architectures included.

If we take metrics for one architecture like arm64-v8a, here are the results:

	APK Size MB	Download Size MB
arm64-v8a	47.3	44.1

Given that, if we are able to reduce the size of the APK for all architectures, it's perceived that each architecture separately would benefit from it.

[hurali97]

Metric: APK Size in MB
Phase: Analysis
Commit hash: 6365aeb

Optimisations Round 1:

We will start with enabling the Proguard and removing any resources that aren't used in the app and in the app's library.

shrinkResources true
minifyEnabled true

Once this is done, we will measure the affect of it on the APK:

	APK Size MB	Download Size MB
All architectures	117.8	114.1

The above measurements uses the default proguard-android.txt which is generated by android build system itself.

With Optimisations Round 2:

We applied all the optimisations from previous round with one change. And that’s we used the default proguard-android-optimize.txt which is also generated by android build system but it’s different than proguard-android.txt as it has a few optimisations flags.

	APK Size MB	Download Size MB
All architectures	116.3	112.6

This is a great result, saving around 10-11 MB as compared to the results in Measurements phase, seems like magic 🪄

There are other things to consider here:

• Enabling Proguard requires extensive testing of the whole app even more when we use optimised version of Proguard.
• It seems that enabling Proguard is trivial as it's only 2-3 liner but there are a handful of issues which are already taken care of and are not stated here. But for an example, the images didn't appear in the app and the reason for that is expo-modules was being ripped off and couldn't show images. To fix that we whitelisted the whole package for Proguard. Moving forward, we will take a closer look at it and only whitelist what is essential in expo-modules package to make the app work as expected.

[roryabraham]

Importing only 'en-GB' and 'es' separately from their respective paths, eg. date-fns/locale/es to only include the ones in use

Ok, this is very interesting, since we have a best practice in place that we should do import * as MyLib from '@libs/MyLib';

When we introduced this best practice, I thought that we determined it would not have an effect on bundle size because babel would strip out the unused imports and end up only importing the named imports that are actually used in the final bundle.

If this is not true, then I'd surmise that our current best practice might actually a bad practice?

In short, I agree with the change to use default imports from the date-fns/locale sub-modules directly, but I would also like to discuss if there's a new best practice we can extract from that to help optimize our bundle size more holistically.

[roryabraham]

For native platforms, polyfills bundled with Hermes can be used

I'm all for removing any polyfills we don't need anymore. Just keep in mind all the JS engines we're exposed to – not only Hermes but also the modern browser landscape.

[roryabraham]

Underscore can be removed in favor of lodash

This is already happening with the TypeScript migration, so 👎🏼 to any new coordinated effort to ditch underscore for now. Once all files are migrated to TypeScript we should have no more uses of underscore, at which point it can be removed.

[roryabraham]

Static assets that are not being animated can be optimised by serving in other, more optimised formats

Nah, let's just stick with svg.

[roryabraham]

Custom code can be replaced with the node-emoji dependency

It's not clear what impact this would have, positive or negative. So unless there's some clear improvement can be made, and it's shown to be significant "worth it", I don't think we should discuss this.

Also, I know in the future we'll want to support custom emojis, that would probably be a better time to discuss overhauls of our emoji implementation.

[roryabraham]

Apply npm audit fix to fix vulnerabilities

Yeah, sounds good. We should probably think about "chorifying" that – i.e: creating a recurring chore, say, every month or every quarter, that's assigned to someone to run npm audit and update vulnerable packages.

This would be a good one to bring to slack with a problem/solution. We already have robust code for creating and auto-assigning chores, so that part is pretty trivial if we agree that this would be a valuable chore to introduce. Would be a happy to discuss a P/S about this in slack.

But yes, in the meantime we can patch known vulnerabilities, but I want to make sure that proper testing is happening so we should manually upgrade them one-at-a-time and thoroughly test relevant functionality, rather than upgrading them all in one fell swoop with npm audit fix and hoping that everything works.

[roryabraham]

👍🏼👍🏼 to removing any unused dependencies, but I'm skeptical of your analysis. For example, ncc is not unused. It's used to compile GitHub Actions here.

Maybe it would make sense to create a separate package.json to separate out GitHubActions-specific dependencies, but honestly doesn't seem worth it because they're all dev dependencies anyways and shouldn't really affect production bundle sizes.

[roryabraham]

Swap metro-react-native-babel-preset with @react-native/babel-preset before migrating to RN@0.73+.

This change is already included in the 0.73 upgrade PR here, so I think we should be covered there.

[roryabraham]

We will start with enabling the Proguard and removing any resources that aren't used in the app and in the app's library

Unfortunately I'm not familiar with this Android thing. Are there any potential downsides?

Edit: Honestly, this makes me a bit nervous:

It seems that enabling Proguard is trivial as it's only 2-3 liner but there are a handful of issues which are already taken care of and are not stated here

It sounds like it could be a common source of development headaches in a code space (i.e: Android native bundling) that most React Native developers (myself included) aren't accustomed to thinking about. So I'm not sure if I'm on board or not. I'm not sure 10MB in the native bundle is worth it – does it have any effect on startup time? Reducing the JS bundle size seems more worth it because it will mean the website will load faster.

[roryabraham]

Hey @mczernek @staszekscp – curious if you have any experience with or recommendation regarding enabling Proguard?

[mczernek]

On the one hand, proguard is as natural as air in native Android development - using it is natural and standard pattern once you go to production.

On the other, I don't see gains being huge here and yet I can see some risks. Since proguard is not so natural in RN ecosystem, some libraries might not be ready for it. There are cases when it can actually delete used code and resources if they are not referenced directly. As mentioned that would require extensive testing of whole app - risk it breaks something might not be huge, but it might happen pretty much anywhere.

Being completly honest, with RN apps I usually do one of two - enable proguard from the very start of development or just skip it altogether. However, being huge app we want to become, I think it might be viable to go through this at some point.

[staszekscp]

TBH, a couple of months ago we've enabled proguard with Michał in order to check if it gives some performance gains on app startup time. The app booted, but we didn't see any difference in performance (also navigating between screens). Because of the risks Michał has mentioned above, we have decided that it is not the path it is worth to take. 😄 However, the app has changed a lot since, so it may be worth to check again.

[hurali97]

@roryabraham @staszekscp @mczernek

These are some really valid points brought up and I agree with almost all of them. I would like to discuss a few things in detail:

note: By using ProGuard, we mean using R8 which is enabled by default and leverages the ProGuard rules in order to perform the minification of the APK, if it's enabled.

- Enabling ProGuard in React Native:

As mentioned, some libraries aren't supporting ProGuard which makes it hard to enable it. I have had issues with ProGuard previously but in case of Expensify, things were pretty smooth. Mostly because aapt2 tool did the hardwork of generating a ProGuard configuration of all the classes, methods and fields used in our codebase. Which then later is combined with default proguard-android.txt or proguard-android-optimize.txt as configured. Finally we have a configuration.txt in app/build/outputs/mappings which have all of the ProGuard rules combined from above.

• I had to add keep rules for BuildConfig and Expo Modules in order to pick the environment flags and show the assets and that's pretty much it. I navigated between a couple screens and things looked fine.

• Given that there's a chance that in some flow we may face a crash or something, so we will need a regression testing in place.

• In future, if we add a dependency to a library and we face a crash we might need to add the relevant keep rules in order to make things work, so it may be a continuous effort.

- Is Improving Native Bundle worth it:

Yes, the reason is related to the growth of the brand itself. It's proven that the apps with larger size face the most uninstalls than the one's with lesser size. You might know it already but stating just for the sake of it. This goes hand in hand with JavaScript Bundle size but we have different techniques to reduce it. For example, the size of an APK with arm64-v8a architecture for New Expensify is around 40 MB on google play store. The same architecture with ProGuard enabled with optimisations is around 33.7 MB, which is going to positively affect the downloads of the app.

What should we do?

Since we have a dedicated effort going on for performance audit and improvements, I would suggest we should try this and conduct a regression of the app and see if it negatively affects the app. By negatively I mean if it makes the app unusable by throwing ANRs, crashes or reducing the app functionality. If there's no such case or we are able to fix it if there is, then we are good to go with using ProGuard.

[mountiny]

Thanks @roryabraham and everyone else jumping in. I agree with Rory's assessment, I think you know the hustle by now, we dont really like to do thing just for sake of doing them/ unless there is a clear reason/ benefit for us so we should skip doing those where this is not clear or negligeable.

In therms fo Proguard, I am on the same boat, I am not familiar with the complexities of this change, seems like everyone who is familiar with it agrees we should do it, but maybe not now. That feels like we can maybe put it on LOW in terms of priority and come back to it once we get some more significant improvements in. Or would you say this is one of the best improvements we can get now?

[adhorodyski]

Underscore can be removed in favor of lodash

This is already happening with the TypeScript migration, so 👎🏼 to any new coordinated effort to ditch underscore for now. Once all files are migrated to TypeScript we should have no more uses of underscore, at which point it can be removed.

Perfect, thanks for bringing this. Since we're (only) auditing the current state, we had to point out any of the improvements - even the ones that are currently ongoing. Let's skip it then!

[adhorodyski]

For native platforms, polyfills bundled with Hermes can be used

I'm all for removing any polyfills we don't need anymore. Just keep in mind all the JS engines we're exposed to – not only Hermes but also the modern browser landscape.

Absolutely - we can bundle these separately based on the platform ans reduce the size only where it's really possible. Should we treat this as an action item for the next phase? This can potentially shave off ~3% on mobiles.

[adhorodyski]

Static assets that are not being animated can be optimised by serving in other, more optimised formats

Nah, let's just stick with svg.

Noted! Let's skip this part then. Mind sharing the reasoning behind it so we can make it clear for the future and not revisit this later? Is it just the quality or other aspects also play a role here?

[adhorodyski]

Custom code can be replaced with the node-emoji dependency

It's not clear what impact this would have, positive or negative. So unless there's some clear improvement can be made, and it's shown to be significant "worth it", I don't think we should discuss this.

Also, I know in the future we'll want to support custom emojis, that would probably be a better time to discuss overhauls of our emoji implementation.

This is definitely a minor thing so we can ditch it, especially if there's a plan for further iterating on this functionality.

This can potentially bring a very small size improvement (less source code but bringing in a new dependency, the JSON will stay there for each translation) + and a small DX improvement tied to the potentially smaller maintenance costs & offloading some of the work to the external module (implementation, docs).

I understand your reasoning on this, I think it's fair to skip it :)

[adhorodyski]

Apply npm audit fix to fix vulnerabilities

Yeah, sounds good. We should probably think about "chorifying" that – i.e: creating a recurring chore, say, every month or every quarter, that's assigned to someone to run npm audit and update vulnerable packages.

This would be a good one to bring to slack with a problem/solution. We already have robust code for creating and auto-assigning chores, so that part is pretty trivial if we agree that this would be a valuable chore to introduce. Would be a happy to discuss a P/S about this in slack.

But yes, in the meantime we can patch known vulnerabilities, but I want to make sure that proper testing is happening so we should manually upgrade them one-at-a-time and thoroughly test relevant functionality, rather than upgrading them all in one fell swoop with npm audit fix and hoping that everything works.

I love that we already have a process for this 😊 Let me add this as an action item to the next phase on this metric.

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[melvin-bot]

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[neil-marcellini]

Why was I assigned here? To review PRs?

[muttmuure]

This is just a tracking issue @neil-marcellini

[melvin-bot]

This issue has not been updated in over 15 days. @mountiny, @adhorodyski, @muttmuure 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!

[roryabraham]

@adhorodyski @muttmuure @mountiny I know that this is just a tracking issue, should we close it out as we're approaching the conclusion of this performance audit and starting another? Do we need a tracking issue or should we just use the #newdot-performance slack room for tracking?

[mountiny]

I think we can close this one and just use the slack room for faster iteration, we can create some new tracking issue for the tracking v2 or a project board once we have specific issues just to be able to better track them

however, that would be done once we have the specific issues created