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

Bring back ASAR archives #14682

Merged
merged 8 commits into from Aug 15, 2017

Conversation

Projects
None yet
6 participants
@paulcbetts
Contributor

paulcbetts commented May 30, 2017

As part of #13916, ASAR gets removed in favor of using snapshots. While snapshots solves the startup speed problem better than ASAR, using ASAR archives also helps a lot with installation speed as well as the time taken to generate an installer.

Refs desktop/dugite#103
Refs desktop/dugite#106

TODO:

  • Make sure this actually generates a usable installer
  • Check to make sure this doesn't regress startup time
  • Look for shell scripts in the github package that need to be moved to .unpacked
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson May 30, 2017

Member

This has some implications for the GitHub package: specifically, there are a few shell scripts that we rely on being able to exec through git. Some of them are already being copied to a temporary folder before execution (as a way to deal with being bundled in the .asar to begin with) but not all of them are handled consistently.

Not a reason not to do this, just a heads-up for us to make sure we handle it and test against it 😄

/cc @BinaryMuse @kuychaco

Member

smashwilson commented May 30, 2017

This has some implications for the GitHub package: specifically, there are a few shell scripts that we rely on being able to exec through git. Some of them are already being copied to a temporary folder before execution (as a way to deal with being bundled in the .asar to begin with) but not all of them are handled consistently.

Not a reason not to do this, just a heads-up for us to make sure we handle it and test against it 😄

/cc @BinaryMuse @kuychaco

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu May 30, 2017

Member

Just realized that this maybe also solves the spurious "missing file" issues we've been seeing.

Member

50Wliu commented May 30, 2017

Just realized that this maybe also solves the spurious "missing file" issues we've been seeing.

@paulcbetts paulcbetts requested a review from as-cii May 30, 2017

@paulcbetts paulcbetts self-assigned this May 30, 2017

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst May 30, 2017

Check to make sure this doesn't regress startup time

Please triple-check this. I'd rather have a slower installer than a slower startup. I install Atom once every 3 months (after a release), while I open Atom every single day, several times per day. Having a faster startup speed and taking it away from the install speed seems like a good trade of.

alexandernst commented May 30, 2017

Check to make sure this doesn't regress startup time

Please triple-check this. I'd rather have a slower installer than a slower startup. I install Atom once every 3 months (after a release), while I open Atom every single day, several times per day. Having a faster startup speed and taking it away from the install speed seems like a good trade of.

@Stanzilla

This comment has been minimized.

Show comment
Hide comment
@Stanzilla

Stanzilla May 31, 2017

@50Wliu it is, hitting the PATH length limit. Guess no one tests on Windows :D

Stanzilla commented May 31, 2017

@50Wliu it is, hitting the PATH length limit. Guess no one tests on Windows :D

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts
Contributor

paulcbetts commented May 31, 2017

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst May 31, 2017

@paulcbetts I'm just saying that if there are any performance penalties in the start up speed, I'd rather have a slower installer. If there are no start up performance penalty, then sure, I'm fine with bringing back ASAR.

alexandernst commented May 31, 2017

@paulcbetts I'm just saying that if there are any performance penalties in the start up speed, I'd rather have a slower installer. If there are no start up performance penalty, then sure, I'm fine with bringing back ASAR.

@as-cii

as-cii approved these changes May 31, 2017 edited

If I recall correctly, introducing .asar archives has a performance penalty of about 50ms: it's not extremely bad but everything adds up in that code path, so I decided to remove them.

Since we use snapshots, another solution to this problem could have been to not bundle files that are already embedded in the snapshot blob. The only reason why we are keeping those files around is because there is a bug in Chromium that prevents snapshotted code from being debugged, so I thought it would have been nice for users to open Atom in dev mode even if they don't have a local checkout of the atom/atom repository and step through source code.

Overall I think I am cool with adding .asar back, but I feel like we should keep in mind that we may be able to deprecate them again in favor of snapshots as soon as that debugging issue is solved upstream (in either Chromium or Electron) and we can avoid shipping those files entirely.

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst May 31, 2017

Just out of curiosity. What is the performance difference between an installer with and without ASAR?
Are we talking about seconds? Minutes?

My point is, if the difference is ~10 seconds, it doesn't pay off adding 50ms (if @as-cii 's memory is correct) to the startup. Installing Atom is something that does not necessarily has to be fast, but opening the editor does has to be.

alexandernst commented May 31, 2017

Just out of curiosity. What is the performance difference between an installer with and without ASAR?
Are we talking about seconds? Minutes?

My point is, if the difference is ~10 seconds, it doesn't pay off adding 50ms (if @as-cii 's memory is correct) to the startup. Installing Atom is something that does not necessarily has to be fast, but opening the editor does has to be.

@Stanzilla

This comment has been minimized.

Show comment
Hide comment
@Stanzilla

Stanzilla May 31, 2017

@as-cii how would that solve the PATH issue on Windows though?

Stanzilla commented May 31, 2017

@as-cii how would that solve the PATH issue on Windows though?

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts May 31, 2017

Contributor

Just out of curiosity. What is the performance difference between an installer with and without ASAR?
Are we talking about seconds? Minutes?

Probably on slow machines, on the order of 10+ minutes, especially if they're applying delta updates. One ASAR file is much faster to delta than 36,000 small files. The bigger problem though, is that the file count is affecting the build process for Atom itself, making it 1hr+ to build packages and making CI time out. We're working on other mitigations to this problem, but moving everything to an ASAR archive will also help.

Also, while ASAR might cause a slight startup penalty, in general it's going to make operations faster, especially on Windows where accessing lots of small files is pretty slow

Contributor

paulcbetts commented May 31, 2017

Just out of curiosity. What is the performance difference between an installer with and without ASAR?
Are we talking about seconds? Minutes?

Probably on slow machines, on the order of 10+ minutes, especially if they're applying delta updates. One ASAR file is much faster to delta than 36,000 small files. The bigger problem though, is that the file count is affecting the build process for Atom itself, making it 1hr+ to build packages and making CI time out. We're working on other mitigations to this problem, but moving everything to an ASAR archive will also help.

Also, while ASAR might cause a slight startup penalty, in general it's going to make operations faster, especially on Windows where accessing lots of small files is pretty slow

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Jun 3, 2017

Member

Also, while ASAR might cause a slight startup penalty, in general it's going to make operations faster, especially on Windows where accessing lots of small files is pretty slow

Unless I am missing something, most of the core files are baked into the snapshot so I am not sure using .asar would buy us much in that regard. Still, it might be nice to profile startup time with and without .asar on Windows and get a sense of how performance looks like.

@as-cii how would that solve the PATH issue on Windows though?

If the paths that exceed the length limit are snapshotted it means that (as soon as that Chromium bug is fixed) we can avoid shipping them as part of the bundle and include them all in the blob that's generated when using mksnapshot.

Member

as-cii commented Jun 3, 2017

Also, while ASAR might cause a slight startup penalty, in general it's going to make operations faster, especially on Windows where accessing lots of small files is pretty slow

Unless I am missing something, most of the core files are baked into the snapshot so I am not sure using .asar would buy us much in that regard. Still, it might be nice to profile startup time with and without .asar on Windows and get a sense of how performance looks like.

@as-cii how would that solve the PATH issue on Windows though?

If the paths that exceed the length limit are snapshotted it means that (as soon as that Chromium bug is fixed) we can avoid shipping them as part of the bundle and include them all in the blob that's generated when using mksnapshot.

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Jun 13, 2017

Contributor

Alright, after the typo fix in desktop/dugite#106, it looks like basic Git operations work, though I still need to check pushing via SSH

Contributor

paulcbetts commented Jun 13, 2017

Alright, after the typo fix in desktop/dugite#106, it looks like basic Git operations work, though I still need to check pushing via SSH

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Aug 15, 2017

Member

A quick benchmark of startup performance confirms the extra cost introduced by setting up .asar in Electron is ~50ms:

screen shot 2017-08-15 at 10 22 50

For now, I think this is a good tradeoff to minimize installation time. Once electron/electron#10234 lands, however, I think we should revisit this for the reasons outlined in #14682.

I'll go ahead and merge this to master as soon as we have a green build. Thanks for your work on this, @paulcbetts. ⚡️

Member

as-cii commented Aug 15, 2017

A quick benchmark of startup performance confirms the extra cost introduced by setting up .asar in Electron is ~50ms:

screen shot 2017-08-15 at 10 22 50

For now, I think this is a good tradeoff to minimize installation time. Once electron/electron#10234 lands, however, I think we should revisit this for the reasons outlined in #14682.

I'll go ahead and merge this to master as soon as we have a green build. Thanks for your work on this, @paulcbetts. ⚡️

@as-cii as-cii merged commit 2cd107a into master Aug 15, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@as-cii as-cii deleted the bring-back-asar branch Aug 15, 2017

@rsese rsese referenced this pull request Aug 29, 2017

Closed

Slow install from zip/rar #15484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment