[HOLD for payment 2024-06-24] [$250] Desktop app startup is slow

m-natarajan commented 5 months 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!

Version Number: 1.4.75-0 Reproducible in staging?: n Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @neil-marcellini Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716480870229429

Action Performed:

Install the desktop app on Mac
Open it
Quit/close it completely
Open it again

Expected Result:

The app opens quickly, within a few seconds

Actual Result:

The app takes over 30 seconds to open, then a bunch of time to load a chat. Let's focus on the slow opening part specifically, since that's unique to the desktop app.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/7347b5a4-0e8d-480f-acf5-cfc14a0dead7

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0192a06bcd75528339
Upwork Job ID: 1793745864691560448
Last Price Increase: 2024-06-06
Automatic offers:

eh2077 | Reviewer | 102692308
dominictb | Contributor | 102692310

Issue Owner

Current Issue Owner: @anmurali

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

MelvinBot commented 5 months ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0192a06bcd75528339

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

goldenbear101 commented 4 months ago

I would like to take on this job

melvin-bot[bot] commented 4 months ago

📣 @goldenbear101! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

goldenbear101 commented 4 months ago

Contributor details Your Expensify account email: irvinwoluchem@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~017e81b8541352d771?mp_source=share

melvin-bot[bot] commented 4 months ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

goldenbear101 commented 4 months ago

Contributor details Your Expensify account email: irvinwoluchem@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~017e81b8541352d771?mp_source=share

melvin-bot[bot] commented 4 months ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

melvin-bot[bot] commented 4 months ago

@anmurali, @neil-marcellini, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

goldenbear101 commented 4 months ago

I have solved the issue and it is now ready for submition

eh2077 commented 4 months ago

@goldenbear101 Can you post a proposal for this issue? You can refer our https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eh2077 commented 4 months ago

Waiting on proposals

goldenbear101 commented 4 months ago

Sorry to say but, I do not have the means (connects) to write a proposal on upwork. I request you allow me to operate through Telegram, you would be able to access the work, and only pay after confirmation.

eh2077 commented 4 months ago

@goldenbear101 You can just post proposals here and we'll review it, see propose-a-solution-for-the-job

dominictb commented 4 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The app takes over 30 seconds to open, then a bunch of time to load a chat. Let's focus on the slow opening part specifically, since that's unique to the desktop app.

What is the root cause of that problem?

In https://github.com/Expensify/App/blob/e6c89d9d06f77eca57faa8ef20b236bbfdbc1153/config/electronBuilder.config.js#L38, we don't specify the arch of the desktop build, so electron builder will select based on the machine arch (x64 or arm64), so if I tried to build locally, it will produce an arm64 binaries as I'm using M1 machine, but we are using macos-13 (Intel-based) in the CI, thus using the app built by the CI will be x64 arch.

For x64 app, M1 machine will need to run the app using Rosetta 2 virtualization => slow startup time is expected.

What changes do you think we should make in order to solve the problem?

I recommend we build universal target by supplying in this https://github.com/Expensify/App/blob/e6c89d9d06f77eca57faa8ef20b236bbfdbc1153/config/electronBuilder.config.js#L47

mac: {
  ....,
 target: [
            {
                target: "dmg",
                arch: [
                    "universal"
                ]
            }
        ]
}

This will double the desktop app size as it contains both x64 and arm64 binaries.

Another option is to release separate binaries for different archs, this can be done by specifying the x64, arm64 in the arch array above.

Obviously, to build the arm64/universal arch on CI, we need to update this https://github.com/Expensify/App/blob/e6c89d9d06f77eca57faa8ef20b236bbfdbc1153/.github/workflows/platformDeploy.yml#L137

to macos-14 (macos-14-large).

What alternative solutions did you explore? (Optional)

NA

goldenbear101 commented 4 months ago

Proposal

The desktop app for Mac takes over 30 seconds to open, significantly longer than expected. This issue is unique to the desktop app and affects user experience.

Root Cause The slow startup likely results from:

Slow initialization of application components Inefficient resource loading Network requests made during startup Proposed Changes To solve the problem, the following changes are recommended:

Optimize Initialization Streamline the initialization process to load only essential components at startup.

async function initializeApp() {
    await loadEssentialComponents();
    // Load non-essential components after startup
    setTimeout(loadNonEssentialComponents, 0);
}

Asynchronous Resource Loading Load resources asynchronously to avoid blocking the main thread.

async function loadResources() {
    const [image, data] = await Promise.all([
        loadImageAsync('path/to/image.png'),
        fetchDataAsync('path/to/data.json'),
    ]);
}

Network Request Optimization Delay non-critical network requests until after the app has loaded.

async function initializeApp() {
    await loadEssentialComponents();
    setTimeout(() => {
        fetchNonCriticalData();
    }, 0);
}

Expected Result The app should open quickly, within a few seconds.

By implementing these changes, the startup time of the desktop app should be significantly reduced.

neil-marcellini commented 4 months ago

@dominictb your proposal sounds pretty promising. Would you please do some benchmarking / testing for us to gather evidence about how much faster it starts up when built for arm? If you don't have an arm mac, please provide steps for me to test.

dominictb commented 4 months ago

@neil-marcellini I'm not sure about benchmarking, will need to think about this. However, you can test this locally since the difference is so evident.

You can try to build the app in both x64 arch and arm64 arch by filling in the electronBuilder.config.js file

mac: {
  ....,
 target: [
            {
                target: "dmg",
                arch: [
                    "x64",
                    "arm64"
                ]
            }
        ]
}

Then npm run desktop-build. After that, just check in the desktop-build folder and compare the startup time of the mac and mac-arm64 app

eh2077 commented 4 months ago

I'm testing @dominictb 's proposal

mvtglobally commented 4 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eh2077 commented 4 months ago

I'll update testing results tomorrow

melvin-bot[bot] commented 4 months ago

@anmurali @neil-marcellini @eh2077 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

flodnv commented 4 months ago

@dominictb this sounds related with https://github.com/Expensify/App/issues/39631#issuecomment-2148278445, right?

dominictb commented 4 months ago

@flodnv yup, as explained in https://github.com/Expensify/App/issues/39631#issuecomment-2148278445, these 2 issues are tightly coupled.

eh2077 commented 4 months ago

I can see significant difference of starting time between universal and x64 app running on M1 MacBook. I think it's common to create distributables for different hardwares, eg x64, arm64 and universal type bundles for macOS. So, @dominictb 's proposal looks good to me.

The other issue seems share some root cause with this issue. #39631 was created earlier though this issue seems has clearer scope. I'm also fine if we decide to put this issue on hold for the older one. Let's defer it to internal engineering team.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 4 months ago

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

yuwenmemon commented 4 months ago

Unassigning myself as @neil-marcellini is the true Engineer on this issue.

melvin-bot[bot] commented 4 months ago

@anmurali, @neil-marcellini, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

eh2077 commented 4 months ago

Not overdue, waiting for @neil-marcellini 's review on https://github.com/Expensify/App/issues/42558#issuecomment-2155182911

neil-marcellini commented 4 months ago

I can see significant difference of starting time between universal and x64 app running on M1 MacBook. I think it's common to create distributables for different hardwares, eg x64, arm64 and universal type bundles for macOS. So, @dominictb 's proposal looks good to me.

Ok great, I agree! Let's build the desktop app as universal.

melvin-bot[bot] commented 4 months ago

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 4 months ago

📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

neil-marcellini commented 4 months ago

I merged that PR, but I want to ask @dominictb @eh2077 how does this work for updating the app? If I click the update menu item in app will it change to being a universal app? If not, how can we make it do that?

dominictb commented 4 months ago

@neil-marcellini I haven't check that, will post my findings here soon.

dominictb commented 4 months ago

@neil-marcellini according to the documentation, it should download the new binaries and run properly. We could create a test-build branch to test that if you want.

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

https://github.com/Expensify/App/pull/43505

If no regressions arise, payment will be issued on 2024-06-24. :confetti_ball:

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

@eh2077 requires payment automatic offer (Reviewer)
@dominictb requires payment automatic offer (Contributor)

melvin-bot[bot] commented 4 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@eh2077] Determine if we should create a regression test for this bug.
[ ] [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot[bot] commented 4 months ago

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

https://github.com/Expensify/App/pull/43505

If no regressions arise, payment will be issued on 2024-06-28. :confetti_ball:

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

@eh2077 requires payment automatic offer (Reviewer)
@dominictb requires payment automatic offer (Contributor)

melvin-bot[bot] commented 4 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@eh2077] Determine if we should create a regression test for this bug.
[ ] [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

eh2077 commented 3 months ago

Checklist

[x] [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I don't think there's a specific PR responsible for this
[x] [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
[x] [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
[x] [@eh2077] Determine if we should create a regression test for this bug. No

eh2077 commented 3 months ago

We fixed two issues together here

@mallenexpensify I saw you have handled the payment for the other issue. Can you also take care of this one please? As for the payment arrangement, whatever you all think is best works for me!

melvin-bot[bot] commented 3 months ago

@anmurali, @neil-marcellini, @eh2077, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 3 months ago

Contributor: @dominictb paid $250 via Upwork Contributor+: @eh2077 paid $250 via Upwork

@eh2077 , the PR to fix this that you reviewed also fixed another bug. If you invested additional time on the review, because their was an increase of scope, you might be due additional compensation. Comment if so.

Expensify / App