search

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.53k stars 2.88k forks source link

[Tracking] Callstack performance audit of the App #33070

Closed mountiny closed 6 months ago

mountiny commented 11 months ago

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:

adhorodyski commented 11 months ago

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

adhorodyski commented 10 months ago

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:

adhorodyski commented 10 months ago

Current status:

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

adhorodyski commented 10 months ago

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

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

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

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 commented 10 months ago

@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 commented 10 months ago

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

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.

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 commented 10 months ago

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

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:

roryabraham commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

πŸ‘πŸΌπŸ‘πŸΌ 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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

mczernek commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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.

- 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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

adhorodyski commented 10 months ago

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.

This one's tricky, indeed the correct babel setup should transform named imports into the default ones - unfortunately looks like none of this happened, we can see all of the translations being loaded together. Coming up with a single 'best practise' would differ based on the tooling and whether this is an internal or external module (might rely on eg. a weird re-exporting pattern). Eg. a VSC plugin for import cost I use failed on this one, providing correct sizes only for the separated imports (both ~7kb).

For the future, I'd say we should stay cautious especially when importing for the first time from newly introduced dependencies & still rely on babel. Short-term, have an action item for importing these separately. Shall I track this as one on the implementation phase?

adhorodyski commented 10 months ago

πŸ‘πŸΌπŸ‘πŸΌ 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.

Thank you so much for pointing this out! Totally my error, I've updated the original comment with this one striked through. I think it makes sense to iteratively try to strip down the unused dependencies then, each one directly saves $ on the CI jobs. Do you think I can put this into the actionable items?

hurali97 commented 10 months ago

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?

This is the best improvements we can have at our hands as of now. Expensify's codebase is growing continuously and if we don't enable ProGuard now, there's a chance that in future, enabling ProGuard might get out of hands as by that time we would have more dependencies in place which may or may not work with ProGuard right away. We could try to make them work by ProGuard rules but that's a different story.

Reducing around 10MBs is considered really significant in terms of APK size as it's relatively complex to reduce the size of APKs. We do have other plans to try and further reduce the APK Size like LTO or native libraries optimisations manually but that's something which is experimental and may or may not benefit us.

I would suggest we enable ProGuard and have a QA regression testing on the generated APK. If there are any crashes reported by QA, gather them in some place and work on them to resolve those on a lower priority OR at the end of the audit. But if we don't face any crash in the regression, we will be good to go to use it right away.

roryabraham commented 10 months ago

Should we treat this as an action item for the next phase?

Yes, we can take steps to remove unnecessary polyfills.

Mind sharing the reasoning behind [sticking with svg]

Yeah, basically I don't want to deal with the development overhead of different image asset types or worry about resolutions. svg works consistently and looks good on all devices and resolutions, and with the recent introduction of expo-image is a lot more performant than it was. We also have an image optimization bot configured in this repo that automatically optimizes svgs to make them smaller.

That's just my opinion though, if it really does make a big difference we could talk about it, but I doubt it does.

Short-term, have an action item for importing [date-fns/locales] separately. Shall I track this as one on the implementation phase?

Yep, sounds good. πŸ‘πŸΌ

I think it makes sense to iteratively try to strip down the unused dependencies then, each one directly saves $ on the CI jobs. Do you think I can put this into the actionable items?

Yep, sounds good. if you find any dependencies that are truly unused and confirm that they're not used we can absolutely remove them πŸ‘πŸΌ

roryabraham commented 10 months ago

I would suggest we enable ProGuard and have a QA regression testing on the generated APK

@hurali97, sounds good πŸ‘πŸΌ

Feel free to open the PR and I can run an AdHoc build and have Applause run a full regression test suite on Android.

roryabraham commented 10 months ago

Actually, @adhorodyski and @hurali97, can we be sure to create separate issues for each of the action items? You can feel free to create them and link them here – they'll automatically be closed but I can reopen them for you.

Also, since we seem to be focusing on bundle size, should we make an issue to do a CI analysis on bundle size for every PR (excluding PRs that only affect GitHub Actions, HelpDot, or tests) that leaves a comment on the PR with the change in bundle size?

hurali97 commented 10 months ago

Also, since we seem to be focusing on bundle size, should we make an issue to do a CI analysis on bundle size for every PR (excluding PRs that only affect GitHub Actions, HelpDot, or tests) that leaves a comment on the PR with the change in bundle size?

Totally agree with this πŸ‘ We would need two separate issues for this, one for JS bundle size and second for App bundle size for android. Later, we can accommodate iOS and webpack bundles too.

mountiny commented 10 months ago

Good idea to break this up to its own issues, I believe they wont autoclose either as you should be whitelisted

adhorodyski commented 10 months ago

Also, since we seem to be focusing on bundle size, should we make an issue to do a CI analysis on bundle size for every PR (excluding PRs that only affect GitHub Actions, HelpDot, or tests) that leaves a comment on the PR with the change in bundle size?

Totally agree with this πŸ‘ We would need two separate issues for this, one for JS bundle size and second for App bundle size for android. Later, we can accommodate iOS and webpack bundles too.

1 message posted on a PR with these listed per platform, correct? Just want to get this right.

roryabraham commented 10 months ago

Yeah, just a single comment on each PR with the JS bundle size and native application size before and after the PR, along with the delta.

If new changes are pushed, we should cancel any analysis still in progress and kick off a new one. If a comment was already left, the previous comment should be hidden as outdated and then a new comment should be left (there are examples for how to do this in our CI already, see the comments left by testBuild.yml)

adhorodyski commented 10 months ago

Team, please see this linked issue for the Search screen load time metric to better separate our discussions.

Update: measurements & analysis have just landed ;)

rinej commented 9 months ago

Hello team! Please see the new issue for the Report screen load time. Measurements phase was just added

hurali97 commented 9 months ago

Hey @mountiny @roryabraham πŸ‘‹

We would like to discuss a flow regarding measuring the App Startup. As of now, following is the flow that we are taking into account for gathering the measurements:

  1. Open the app
  2. Splash will be hidden after a few secs
  3. Login to the app
  4. Then wait for the data to show
  5. Close the app and force stop it from the settings
  6. Open the app again
  7. Splash will be hidden after a long period
  8. Data will be visible when the splash hides

For each subsequent measurement, steps 1-3 are not followed. Which means we loop over from step 5-8 for the measurement cycle.

This way we get the app startup of the usual flow that a user faces after the fresh install, uses the app for a while and closes it. And then each time he'll launch the app he follows the same flow as we are following here.

As of now, we are conducting measurements based on this flow. We are not taking into account the fresh install app startup as it has TTI until the SignIn page shows after the splash. This TTI doesn't reflect what a user will face on a daily basis. Hence, we opted for going with the flow described above.

We would like to get insights from you and whether the flow we are using for conducting the measurements is what you expect or otherwise. πŸ‘

mountiny commented 9 months ago

As of now, we are conducting measurements based on this flow. We are not taking into account the fresh install app startup as it has TTI until the SignIn page shows after the splash. This TTI doesn't reflect what a user will face on a daily basis. Hence, we opted for going with the flow described above.

I think this makes sense, I think fresh install is an edge case we do not have to focus on in here

adhorodyski commented 9 months ago

Team, please see the update under https://github.com/Expensify/App/issues/34494#issuecomment-1900552863 on how we decided to unify 2 metrics and just stick with the Load time in MS for opening up a report.

jbroma commented 9 months ago

Hi team! I've added new issue for tracking Average FPS on LHN -> #34986

kacper-mikolajczak commented 9 months ago

Hi folks! πŸ‘‹

Please take a look at the Send a message metric tracker #35225. The measurements section has been updated and analysis is coming soon πŸ”œ πŸš€

hurali97 commented 9 months ago

Hey guys πŸ‘‹

Here's the metric tracker for App Startup. We have updated the measurements. The analysis phase is in progress and will be updated in the next week. We have some amazing results from analysis phase, so stay tuned πŸ‘

melvin-bot[bot] commented 9 months ago

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

mountiny commented 8 months ago

Audit still underway

adhorodyski commented 8 months ago

Hey everyone!

As the last analysis has just landed under this issue targeting Report screen load time, we need some of your help with wrapping up the last action items for the next phase of Implementation (please see the linked issue).

This next phase is currently in the works and PRs are going to be shared in the next days as we're finishing up defining the backlog items based on the discussions in the respective threads.

melvin-bot[bot] commented 8 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 8 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.45-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-03-07. :confetti_ball:

For reference, here are some details about the assignees on this issue: