Skip to content
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] Reduce Desktop size #6927

Closed
kidroca opened this issue Dec 29, 2021 · 30 comments
Closed

[Performance] Reduce Desktop size #6927

kidroca opened this issue Dec 29, 2021 · 30 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Not a priority Planning Changes still in the thought process

Comments

@kidroca
Copy link
Contributor

kidroca commented Dec 29, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


What performance issue do we need to solve?

  • Desktop app: large installer file size (326 MB)
  • Desktop app: taking a lot of space after a fresh install (812 MB)

What is the impact of this on end-users?

  • Faster app download and install
  • Users might be reluctant to install a large desktop app

List any benchmarks that show the severity of the issue

  • App installer size: 326MB
  • Slack installer size: 90MB (Also uses Electron / Chromium)
  • Chrome installer size: 1MB

Proposed solution (if any)

Research ways to reduce the installer size
For example Chrome's installer is so small because it downloads more data as part of the installation process

Research Electron documentation on ways to reduce size

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

Note: These should be the same as the benchmarks collected before any changes.

Platform:

Where is this issue occurring?

  • ⚠️ Desktop App

Version Number: 1.1.24-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
#expensify-open-source: https://expensify.slack.com/archives/C01GTK53T8Q/p1640642055143300

View all open jobs on Upwork

@MelvinBot
Copy link

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@kidroca
Copy link
Contributor Author

kidroca commented Dec 29, 2021

@parasharrajat
Copy link
Member

This article seems helpful https://medium.com/gowombat/how-to-reduce-the-size-of-an-electron-app-installer-a2bc88a37732

@iwiznia
Copy link
Contributor

iwiznia commented Dec 29, 2021

I don't think we should go the chrome route. Either we make the app smaller or we don't, making the installer seem small is not worth it IMO.

@marcaaron
Copy link
Contributor

Thanks for the investigation! Just my opinion, but doesn't seem super high priority considering contributors and Expensify staff are the main users of the desktop app at the moment and large installer sizes are not likely to inhibit anyone motivated to install and use the app. I would recommend we wait until there are more usage statistics to address the problem of a large installer. Just my 2 cents though - will leave this open and let others share their thoughts.

@marcaaron marcaaron removed their assignment Dec 29, 2021
@marcaaron marcaaron added Monthly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Dec 29, 2021
@MelvinBot
Copy link

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Monthly KSv2 labels Dec 29, 2021
@mallenexpensify
Copy link
Contributor

@kidroca are you fine with me assigning this to you as a monthly? I can add the planning label too. There isn't an immediate need to decrease the file size but it's something we should be aware of and consider addressing. We might be able to make incremental improvements over time too, which is why I'd like to leave this issue open.

@kidroca
Copy link
Contributor Author

kidroca commented Dec 30, 2021

Yes, I'd like that

@kidroca
Copy link
Contributor Author

kidroca commented Dec 30, 2021

I don't think we should go the chrome route. Either we make the app smaller or we don't, making the installer seem small is not worth it IMO.

Chrome's 1MB is not exactly a trick to make the app small, but actually to download only what's necessary

We have 2 architectures on mac now - x86 (Intel) and arm64 (M1) supporting both with 1 installer means 2x the size

One installer for all archs (current):

  • ✅ installs on both architectures
  • ❌ 2x package size due to architectures

Installer per arch:

  • ❌ users can download the wrong installer
  • ✅ small package size

Installer downloads on the fly:

  • ✅ installs on both architectures
  • ✅ small package size

Let's investigate the possibilities

  1. Is it OK to make separate installers for so we have 1 installer per arch.
  2. Installer that downloads on the fly may be not so hard to do

(1) Would probably be the simplest. We'd need to auto detect the user architecture and offer them the correct package to download. And/or let them pick manually

Investigate what would it take to implement (2)

@iwiznia
Copy link
Contributor

iwiznia commented Dec 30, 2021

Oh, good points. I assume that 2 needs 1, no? ie: we need the per arch installers that the "main" installer downloads, right?
If so, let's start with 1 and then we can do 2 if we want.

@mallenexpensify mallenexpensify added Monthly KSv2 Planning Changes still in the thought process and removed Daily KSv2 labels Dec 30, 2021
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Dec 30, 2021

Thanks @kidroca, issue labels/assignment updated!

@kidroca
Copy link
Contributor Author

kidroca commented Jan 22, 2022

Found something interesting

electron-builder would include all node_modules that are not devDependencies in the package.
App being a RN project there are a lot of native platform code inside node_modules that is getting packed (and of course it's not needed)

  • why does it include node_modules - because we have a custom files config: https://www.electron.build/configuration/contents.html

    Development dependencies are never copied in any case. You don’t need to ignore it explicitly.
    ...
    package.json and **/node_modules/**/* (only production dependencies will be copied) is added to your custom in any case

Before

  • zip: 318.9 MB
  • App: 848.1 MB

image

After moving dependencies to devDependencies

  • zip: 161 MB
  • App: 406 MB

image

Video Sample

Expensify.mp4

@kidroca
Copy link
Contributor Author

kidroca commented Jan 22, 2022

Let's investigate the possibilities

1. Is it OK to make separate installers for so we have 1 installer per arch.
2. Installer that downloads on the fly may be not so hard to do

It turned out we only build for a single arch already - the build command will use the current architecture
The doubled size is due to node_modules and not multiple architectures being built and bundled in one package

An easy change to reduce app size by half would be to move our dependencies to devDependencies
Some believe this is the way to go, because front-end apps only use external packages during the build/bundling stage

https://jsramblings.com/do-dependencies-devdependencies-matter-when-using-webpack/

This approach considers that since your production app (aka the bundle you built with Webpack) can just run by itself, it means you have no production dependencies. Thus, all dependencies are devDependencies.

App being an end product (e.g. not a library) has not much use for declaring dependencies - it's purely used to separate tooling like eslint from external libraries like React

Another approach might be to come up with an ignore pattern(s), like !**/node_modules/react-* and supply it to the config
One caveat is that this pattern might need to be kept up to date, while simply adding packages to devDependencies would make sure electron never copies them to the end build


The electron app actually starts here, and these packages do need to be included in the dependencies list (because they are used post build)

App/desktop/main.js

Lines 9 to 13 in 0bce091

const _ = require('underscore');
const serve = require('electron-serve');
const contextMenu = require('electron-context-menu');
const {autoUpdater} = require('electron-updater');
const log = require('electron-log');

  • Their total size is about a 1 MB

@mallenexpensify
Copy link
Contributor

@marcaaron @iwiznia can you review the above? (not time sensitive). Cutting the file size in half would be a big improvement.

@iwiznia
Copy link
Contributor

iwiznia commented Jan 27, 2022

I am a bit confused... how do we know which dependencies are used during build and which are used during run? Seems to me that there's a ton of dependencies that are used during run besides the ones being required in main.js, no?

@kidroca
Copy link
Contributor Author

kidroca commented Jan 27, 2022

Seems to me that there's a ton of dependencies that are used during run besides the ones being required in main.js, no?

Non of the src code is imported/required in main.
The way the desktop project is setup the main is evaluated during run time and all the require calls count as runtime dependencies

how do we know which dependencies are used during build and which are used during run?

Excluding the stuff in main, everything else is used only during build time

  1. At build we tell Webpack (or Metro) to bhndle the app given an entry point (src/index)
  2. The bundle analyzes the entry follows all imports and creates a single (or multiple) js file that has everything - our code + external libraries
  3. Evening needed to run the app is included in this bundle.
  4. Depending on the platform the bundle is hosted or packed inside the app

For example on web we host an index.html that includes a script tag with the bundled js. We're not copying the node modules along with that

Pretty much everything in a front end project is a build time dependency - whatever is needed during runtime is bundled in

@parasharrajat
Copy link
Member

Great, idea.

  1. Build the app like web app first.
  2. Move the dist./ files to the main Desktop Package only.
  3. No node_modules required.

@kidroca
Copy link
Contributor Author

kidroca commented Jan 28, 2022

Found another improvement - we're double adding the compiled app which is another 2x size bump

This config rule adds the dist folder to the desktop package

files: [
'./dist/**/*',

But by default dist is also what electron uses to build the desktop package, and while building it adds temporary stuff to it
This temporary stuff gets added to the package because we're telling Electron to put everything from dist in the package

Let's see what's actually packed in App:
image
- note: this is App after the node_modules fix, Content used to be 800+ MB before

Frameworks (180MB) - can't really do anything about this it's mostly electron

image

Resources (213 MB) on the other hand is where our bundle is

image

Let's see what's inside this `app.asar` archive

image

This is evidence of the dist/mac folder getting packed in - we don't need that

Electron using the output folder (dist) for a temporary build which we unintentionally add to the package and this is worth another 180MB

End result of fixing this is the installer becomes 88MB

image

I've provided a link - you can see for yourself that the app is working same as before but it's 4x less size


It's easy to fix this by specifying an output directory for Electron

diff --git a/config/electron.config.js b/config/electron.config.js
--- a/config/electron.config.js	(revision d59aa4f750ba847843aee64d4c5a2bbf4ded3528)
+++ b/config/electron.config.js	(date 1643385656801)
@@ -27,4 +28,7 @@
         './desktop/*.js',
         './src/libs/checkForUpdates.js',
     ],
+    directories: {
+        output: 'desktop-build',
+    },
 };

@kidroca
Copy link
Contributor Author

kidroca commented Jan 28, 2022

Note on architectures

I don't know how the app is built and distributed. From package.json and electron.config.js I see an arch is not specified - when not specified it would be the arch of the current computer (x64 Intel mac?)

Here's the result from building the `x64` (same as the one shared in the previous post)

image
This works perfectly fine on my M1 mac, though it's emulated through Rosetta

A `arm64` app is about the same size and works without emulation

image

And finally there's the option to build an Universal app that works on both architectures, but takes 2x the size

image

Architectures and sizes

image

  • folders are unpacked apps
  • .dmgs are installer sizes

@iwiznia
Copy link
Contributor

iwiznia commented Jan 28, 2022

Nice! Thanks for the explanations!!
Let's get those changes in.
As for the platform, I would think it makes the most sense to build for the 2 different architectures and then somehow detect them when downloading them, so you always download the correct one.

@kidroca
Copy link
Contributor Author

kidroca commented Jan 31, 2022

@iwiznia
I think others might be confused as well when all of the sudden dependencies become devDependencies, shouldn't we prepare for that with a slack comment or something?

It might be a good idea to update documentation so that we don't make the same mistake when new packages are installed
They should be installed in devDependencies otherwise they'll again get packed in vain

@kidroca
Copy link
Contributor Author

kidroca commented Jan 31, 2022

Posted a thread on slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1643645168448979


We decided to go in another direction and create a 2nd package.json for the ./desktop app
This way we can isolate dependencies there
It's know as the Two package.json Structure

@kidroca kidroca mentioned this issue Feb 14, 2022
14 tasks
@kidroca
Copy link
Contributor Author

kidroca commented Feb 15, 2022

Pull Request is ready for review: #7744

@kidroca
Copy link
Contributor Author

kidroca commented Mar 16, 2022

There's another way to make the Desktop app much smaller

By making web comply with Progressive Web App (PWA) standards, Chrome would allow you to install the website as a Desktop App: https://caniuse.com/web-app-manifest

By default the app is kept up to date the same way a website is - when you launch/refresh you see the updated version

Expensify.PWA.mp4
Installing and removing the app
Expensify.PWA.installing.mp4

About 200 line changes were necessary to achieve what's presented in the video

  • draft: Expensify PWA app #8182
  • 30 for web pack config
  • 100 to have the "install" option in the download links section
  • 75 for a custom status bar

@melvin-bot
Copy link

melvin-bot bot commented May 30, 2022

@kidroca, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@mallenexpensify
Copy link
Contributor

@kidroca you were able to reduce the desktop size some, right? Can you provide a quick update for auditing purposes (or... let me know if you think it should be reopened)

@kidroca
Copy link
Contributor Author

kidroca commented Jun 1, 2022

@mallenexpensify
It's ok to close this one, the work was done quite some time ago
I've posted some other options here and on slack and was waiting in case they get picked up, but they didn't

Summary

Before After
Installer size (dmg) 326 MB 86 MB
Installed app size 812 MB 187 MB

@mallenexpensify
Copy link
Contributor

@kidroca were we able to reduce the size at all? Are the After #s where we're currently at or where we could be?

@kidroca
Copy link
Contributor Author

kidroca commented Jun 1, 2022

Sorry for the confusion. The size is considerably reduced - "after" is the result of this ticket and the current size of the desktop app

@kidroca
Copy link
Contributor Author

kidroca commented Jun 1, 2022

What I'm demoing here is removing the desktop app entirely and make the web app inatallable as PWA (Progressive web app): #6927 (comment)

The experience is very similar to what we have with the electron desktop app, but without the need to download the app (and maintain desktop specific code)

But it has some caveats
Like App (PWAs) can only be installed if you open web on Chrome (so far)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Not a priority Planning Changes still in the thought process
Projects
None yet
Development

No branches or pull requests

6 participants