Optimization of mobile startup time #31776

perunt · 2023-11-23T10:23:57Z

Details

Fixed Issues

$ #31787
PROPOSAL:

This PR introduces lazy loading for large JSON files, notably emoji data, and optimizes imports across the application.

The updates were made across all graphs. Some of the examples here:
Before

After

The testing was limited to a few devices, and further testing across a broader range is encouraged to validate these improvements.

Android: Observed 2x faster performance on devices ranging from HUAWEI P smart 2019 (low end) to Pixel 7 PRO.
iOS: iPhone 7 Pro showed a 2x + improvement in speed.

	BEFORE	AFTER
IOS
Android

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

We need to double-check the places where we use EMOJI:

emoji picker
replacing text with emoji inside the input
edit input
EMOJI in the chat
pages that contain Bank Icons
Auth flow of the app

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

…tartup-profiling

Julesssss · 2023-11-23T16:56:41Z

src/libs/EmojiUtils.ts

@@ -322,6 +322,9 @@ function getAddedEmojis(currentEmojis: Emoji[], formerEmojis: Emoji[]): Emoji[]
 * If we're on mobile, we also add a space after the emoji granted there's no text after it.
 */
 function replaceEmojis(text: string, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE, lang: 'en' | 'es' = CONST.LOCALES.DEFAULT): ReplacedEmoji {
+    // emojisTrie importing the emoji JSON file on the app starting and we want to avoid it


So instead of importing the whole file we wait until replaceEmojis is called?

how much does this reduce startup time? Won't this affect user opening emoji picker as soon as app is loaded?

It shaves off 20-30% of the time. However, this doesn't mean the importing continues for the entire duration. It's more complex than that. In our case, it doesn't impact the start-to-use time in the area where we're currently applying it. At least I haven't seen it. Would be great if you could also test it

yeah, we've shifted the timing of importing this file from when the thread is busy to a time when it can run more smoothly.

Julesssss

Great improvement, we just need a C+ review and to link an issue.

melvin-bot · 2023-11-23T17:30:06Z

@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

situchan

Overall looks good! Please complete author checklist

situchan · 2023-11-23T17:32:30Z

src/libs/EmojiUtils.ts

@@ -322,6 +322,9 @@ function getAddedEmojis(currentEmojis: Emoji[], formerEmojis: Emoji[]): Emoji[]
 * If we're on mobile, we also add a space after the emoji granted there's no text after it.
 */
 function replaceEmojis(text: string, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE, lang: 'en' | 'es' = CONST.LOCALES.DEFAULT): ReplacedEmoji {
+    // emojisTrie importing the emoji JSON file on the app starting and we want to avoid it


how much does this reduce startup time? Won't this affect user opening emoji picker as soon as app is loaded?

src/types/onyx/Fund.ts

blazejkustra · 2023-11-24T08:44:29Z

src/components/Icon/BankIcons.ts

-    if (bankName.startsWith('us bank') || bankName.startsWith('usbank')) {
-        return isCard ? USBankCard : USBank;
-    }
+function getAssetIcon(bankName: BankNameAndEmptyString, isCard: boolean): React.FC<SvgProps> {


How did you test this function? Some tests steps would be appreciated!

I used scenario from this PR #27923
As it was about migrating that component

Thanks! 😄

src/components/Icon/BankIcons.ts

blazejkustra · 2023-11-24T08:47:29Z

src/components/Icon/BankIcons.ts

+function getAssetIcon(bankName: BankNameAndEmptyString, isCard: boolean): React.FC<SvgProps> {
+    // Mapping bank names to their respective icon paths
+    const iconMappings = {
+        [BANK_NAMES.EXPENSIFY]: isCard ? require('@assets/images/cardicons/expensify-card-dark.svg') : require('@assets/images/bankicons/expensify.svg'),


Does it mean that each time getAssetIcon is called all these svgs are imported synchronously? Doesn't it make performance worse?

About using absolute paths – they get cached and then referenced in subsequent uses. I'm not totally sure about paths with aliases like @assets/..., since they're relative. But from my testing, I didn't notice any regression. That's probably because this particular icon isn't used too often, and only one icon is called at a time. Plus, I didn't see any increase in memory usage. So, I'm cautiously optimistic that these might get cached too.

src/components/Icon/BankIcons.ts

blazejkustra · 2023-11-24T09:08:30Z

src/libs/EmojiUtils.ts

@@ -322,6 +322,9 @@ function getAddedEmojis(currentEmojis: Emoji[], formerEmojis: Emoji[]): Emoji[]
 * If we're on mobile, we also add a space after the emoji granted there's no text after it.
 */
 function replaceEmojis(text: string, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE, lang: 'en' | 'es' = CONST.LOCALES.DEFAULT): ReplacedEmoji {
+    // emojisTrie importing the emoji JSON file on the app starting and we want to avoid it
+    const emojisTrie = require('./EmojiTrie').default;


Same question here, does it mean that emojisTrie will be imported each time replaceEmojis is called?

If we do not change the required path, it will return the same reference cached in the first load. At least, that's how I understand this after additional research and testing.

That's cool, thanks!

OSBotify · 2023-11-27T16:55:47Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31776/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/31776/index.html

Desktop 💻	Web 🕸️
❌ FAILED ❌	https://31776.pr-testing.expensify.com
The QR code can't be generated, because the Desktop build failed

…tartup-profiling

perunt · 2023-11-30T11:04:54Z

Hey @situchan, how is it going? I'm quite excited to see the result 😃

perunt · 2023-11-30T12:45:02Z

I've tried to run this on Desktop again, and it works okay for me, so it's probably just a glitch.

mountiny · 2023-11-30T13:22:36Z

Started another build https://github.com/Expensify/App/actions/runs/7047044234

OSBotify · 2023-11-30T13:39:31Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31776/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/31776/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/31776/NewExpensify.dmg	https://31776.pr-testing.expensify.com

blazejkustra

Looks good typescript-wise

src/components/Icon/BankIcons.ts

src/libs/Navigation/AppNavigator/AuthScreens.js

situchan · 2023-11-30T16:23:35Z

please pull main & fix lint

…tartup-profiling

perunt · 2023-11-30T16:41:59Z

The conflicts appear every few minutes 😸
It should be good now

situchan · 2023-11-30T16:47:00Z

The conflicts appear every few minutes 😸 It should be good now

yes, there's light theme migration recently and they touch lots of files

situchan · 2023-11-30T17:33:18Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

mchrome.mov

iOS: Native

ios.mov

iOS: mWeb Safari

msafari.mov

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

situchan

melvin-bot · 2023-11-30T17:34:03Z

We did not find an internal engineer to review this PR, trying to assign a random engineer to #31787 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

mountiny

Thank you @perunt, this is very helpful

mountiny · 2023-11-30T18:09:06Z

I forgot I was not assigned to the issue, added bug label and @situchan for payment later

johnmlee101

Whoa, really cool stuff!

OSBotify · 2023-11-30T18:37:01Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-12-01T12:56:58Z

🚀 Deployed to staging by https://github.com/johnmlee101 in version: 1.4.7-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-12-05T02:27:21Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.7-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

perunt added 17 commits October 5, 2023 17:39

Merge branch 'main' of https://github.com/Expensify/App

a19fcb2

Merge branch 'main' of https://github.com/Expensify/App

78ef9ae

Merge branch 'main' of https://github.com/Expensify/App

3b0858a

Merge branch 'main' of https://github.com/Expensify/App

e28a632

Merge branch 'main' of https://github.com/Expensify/App

9cc08a0

Merge branch 'main' of https://github.com/Expensify/App

6f5fcf4

Merge branch 'main' of https://github.com/Expensify/App

cfb5b6a

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

bc1c7e0

…tartup-profiling

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

bffe6f7

…tartup-profiling

add lazy imports for AuthScreens

4e7f09a

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

1c75def

…tartup-profiling

return RightModalNavigator

9a958eb

export SupportedLanguage

b3d0a63

lazy import of emojisTrie

dad435b

lazy BankIcons

fa945da

add BankName type

a83117b

fix warning

e962db8

perunt changed the title ~~Profiling~~ Optimization of mobile startup time Nov 23, 2023

clean

1e9d3af

Julesssss assigned perunt Nov 23, 2023

Julesssss reviewed Nov 23, 2023

View reviewed changes

muttmuure mentioned this pull request Nov 23, 2023

[HOLD for payment 2023-12-12] [CRITICAL] Optimize App Start Up Time with Lazy Loading #31787

Closed

muttmuure requested a review from situchan November 23, 2023 17:09

perunt marked this pull request as ready for review November 23, 2023 17:30

perunt requested a review from a team as a code owner November 23, 2023 17:30

melvin-bot bot removed the request for review from a team November 23, 2023 17:30

situchan reviewed Nov 23, 2023

View reviewed changes

blazejkustra suggested changes Nov 24, 2023

View reviewed changes

perunt added 2 commits November 30, 2023 11:27

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

25f5b00

…tartup-profiling

fix getBankIcon type after merge

c034e71

blazejkustra approved these changes Nov 30, 2023

View reviewed changes

situchan reviewed Nov 30, 2023

View reviewed changes

src/components/Icon/BankIcons.ts Show resolved Hide resolved

situchan reviewed Nov 30, 2023

View reviewed changes

src/libs/Navigation/AppNavigator/AuthScreens.js Outdated Show resolved Hide resolved

fix imports

a0ab476

perunt added 3 commits November 30, 2023 17:31

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

6166f2b

…tartup-profiling

change the order of imports

cdfb6d7

Merge branch 'main' of https://github.com/Expensify/App into perunt/s…

e6d7779

…tartup-profiling

situchan approved these changes Nov 30, 2023

View reviewed changes

melvin-bot bot requested a review from johnmlee101 November 30, 2023 17:34

mountiny approved these changes Nov 30, 2023

View reviewed changes

johnmlee101 approved these changes Nov 30, 2023

View reviewed changes

johnmlee101 merged commit 56a368b into Expensify:main Nov 30, 2023
17 checks passed

github-actions bot mentioned this pull request Dec 1, 2023

Deploy Checklist: New Expensify 2023-12-01 #32342

Closed

53 tasks

NikkiWines mentioned this pull request Dec 2, 2023

[HOLD for payment 2023-12-12] Web/mweb - Account settings - Assigned cards in Wallet do not display card logos #32392

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimization of mobile startup time #31776

Optimization of mobile startup time #31776

perunt commented Nov 23, 2023 •

edited

Loading

Julesssss Nov 23, 2023

situchan Nov 23, 2023

perunt Nov 24, 2023

perunt Nov 24, 2023

Julesssss left a comment

melvin-bot bot commented Nov 23, 2023

situchan left a comment

situchan Nov 23, 2023

blazejkustra Nov 24, 2023

perunt Nov 24, 2023

blazejkustra Nov 24, 2023

blazejkustra Nov 24, 2023 •

edited

Loading

perunt Nov 24, 2023

blazejkustra Nov 24, 2023

perunt Nov 24, 2023

blazejkustra Nov 24, 2023

OSBotify commented Nov 27, 2023

perunt commented Nov 30, 2023

perunt commented Nov 30, 2023

mountiny commented Nov 30, 2023

OSBotify commented Nov 30, 2023

blazejkustra left a comment

situchan commented Nov 30, 2023

perunt commented Nov 30, 2023

situchan commented Nov 30, 2023

situchan commented Nov 30, 2023

situchan left a comment

melvin-bot bot commented Nov 30, 2023

mountiny left a comment

mountiny commented Nov 30, 2023

johnmlee101 left a comment

OSBotify commented Nov 30, 2023

OSBotify commented Dec 1, 2023

OSBotify commented Dec 5, 2023

Optimization of mobile startup time #31776

Optimization of mobile startup time #31776

Conversation

perunt commented Nov 23, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Nov 23, 2023

situchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazejkustra Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Nov 27, 2023

perunt commented Nov 30, 2023

perunt commented Nov 30, 2023

mountiny commented Nov 30, 2023

OSBotify commented Nov 30, 2023

blazejkustra left a comment

Choose a reason for hiding this comment

situchan commented Nov 30, 2023

perunt commented Nov 30, 2023

situchan commented Nov 30, 2023

situchan commented Nov 30, 2023

Reviewer Checklist

Screenshots/Videos

situchan left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Nov 30, 2023

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Nov 30, 2023

johnmlee101 left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 30, 2023

OSBotify commented Dec 1, 2023

OSBotify commented Dec 5, 2023

perunt commented Nov 23, 2023 •

edited

Loading

blazejkustra Nov 24, 2023 •

edited

Loading