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

Minify the startup snapshot script with terser #17926

Merged
merged 1 commit into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Aug 25, 2018

Background

I was just looking at a heap snapshot in Atom and I noticed something striking - over a third of the memory allocated on V8's heap is due to a single script object: Atom's concatenated startup script.

In particular, the script retains a reference to its source code as a string, which consumes around 45MB of RAM:

screen shot 2018-08-24 at 5 08 19 pm

In addition, the script maintains an array of line ends, which I assume V8 uses for computing stack traces. This consumes almost 3MB.

screen shot 2018-08-24 at 5 08 37 pm

Proposed Change

In this PR, I minify the startup script that electron-link generates using terser (a fork of uglify that works with es6).

Results

When I compute a heap snapshot after minification, the startup script's source now consumes only 16MB on the heap, and its line ends no longer shows up in the snapshot (there are no line endings any more).

To get a sense of the overall impact, I rebuilt Atom Dev with this change and ran it simultaneously with the most recent Atom Beta. In each case, I opened one file in the atom/atom repo. When all is said and done, it looks like this reduces our memory consumption by 45 MB per window, and the same amount in the main process!

screen shot 2018-08-24 at 5 20 53 pm

Drawbacks

The minification step does take some time. It adds 10 - 20 seconds to the build.

@daviwil

That's pretty amazing! Does it have a similar reduction in the app size?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 25, 2018

Contributor

Looks like the macOS failure was a flake. Restarting...

Contributor

maxbrunsfeld commented Aug 25, 2018

Looks like the macOS failure was a flake. Restarting...

@smashwilson

😍 Neat! Awesome find.

One question: I see that we aren't generated sourcemaps for the minified code. Does this result in mangled stacktraces? An inline sourcemap would defeat the purpose of the minification I'm sure... but maybe one saved in a separate file wouldn't be loaded unless you actually needed it... ?

I've actually wondered if we could get away with not shipping source at all by default - relying on the snapshot for distribution of core and bundled packages. Maybe bundling it separately and downloading it on demand so that people could debug. It would really slim down our app download. It's probably not worth it though 🤔

process.stdout.write('Minifying startup script')
const minification = terser.minify(snapshotScript, {
keep_classnames: true,
compress: {keep_fargs: true, keep_infinity: true}

This comment has been minimized.

@smashwilson

smashwilson Aug 25, 2018

Member

Just curious - did classname mangling break something?

@smashwilson

smashwilson Aug 25, 2018

Member

Just curious - did classname mangling break something?

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Aug 25, 2018

Contributor

Yeah, I think we have some code that uses ‘constructor.name’. I was seeing errors in the GitHub package; something about repository state transitions...

I didn’t look into it too deeply though.

@maxbrunsfeld

maxbrunsfeld Aug 25, 2018

Contributor

Yeah, I think we have some code that uses ‘constructor.name’. I was seeing errors in the GitHub package; something about repository state transitions...

I didn’t look into it too deeply though.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 25, 2018

Contributor

Yeah, great points about stack traces and source maps. We’d need to do that before merging this. Hopefully it won’t negate the memory savings.

I think the only reason we include the source in the bundle is so you can run Atom in dev mode for debugging without having the source checked out. Not sure how many people do that though.

Contributor

maxbrunsfeld commented Aug 25, 2018

Yeah, great points about stack traces and source maps. We’d need to do that before merging this. Hopefully it won’t negate the memory savings.

I think the only reason we include the source in the bundle is so you can run Atom in dev mode for debugging without having the source checked out. Not sure how many people do that though.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 26, 2018

Contributor

Actually, I don't think we need to do anything for stack traces. Here's an example error that I produced by manually breaking my editor from the dev tools:

screen shot 2018-08-25 at 6 19 10 pm

Since property names are not mangled by default, and I've disabled mangling of class names, everything is just as clear as before. Even before this change, the line numbers weren't really useful because they were referring to the concatenated <embedded> script.

Contributor

maxbrunsfeld commented Aug 26, 2018

Actually, I don't think we need to do anything for stack traces. Here's an example error that I produced by manually breaking my editor from the dev tools:

screen shot 2018-08-25 at 6 19 10 pm

Since property names are not mangled by default, and I've disabled mangling of class names, everything is just as clear as before. Even before this change, the line numbers weren't really useful because they were referring to the concatenated <embedded> script.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 26, 2018

Contributor

One other drawback: the dev tools becomes unresponsive for about 10 seconds if you try to open the embedded script in the Sources tab. I guess the dev tools text editor (like several others) has trouble with long lines. This can be annoying, but I don't think it's a big enough problem to outweigh the benefits of minification; you can still use the dev tools for most normal purposes.

Contributor

maxbrunsfeld commented Aug 26, 2018

One other drawback: the dev tools becomes unresponsive for about 10 seconds if you try to open the embedded script in the Sources tab. I guess the dev tools text editor (like several others) has trouble with long lines. This can be annoying, but I don't think it's a big enough problem to outweigh the benefits of minification; you can still use the dev tools for most normal purposes.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld
Contributor

maxbrunsfeld commented Aug 27, 2018

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 27, 2018

Contributor

Does it have a similar reduction in the app size?

It looks like it does reduce the size of app.asar from about 260MB to 220MB.

I've actually wondered if we could get away with not shipping source at all by default - relying on the snapshot for distribution of core and bundled packages.

I agree. The source code is not very useful anyway in the form that we ship it. If people really want to debug Atom's bundled code, they pretty much need to clone and build Atom anyway. I'm inclined to attack this in a separate PR, but I do think it's worth doing.

Contributor

maxbrunsfeld commented Aug 27, 2018

Does it have a similar reduction in the app size?

It looks like it does reduce the size of app.asar from about 260MB to 220MB.

I've actually wondered if we could get away with not shipping source at all by default - relying on the snapshot for distribution of core and bundled packages.

I agree. The source code is not very useful anyway in the form that we ship it. If people really want to debug Atom's bundled code, they pretty much need to clone and build Atom anyway. I'm inclined to attack this in a separate PR, but I do think it's worth doing.

@maxbrunsfeld maxbrunsfeld merged commit eb39de9 into master Aug 27, 2018

2 of 3 checks passed

VSTS: Atom Pull Requests 20180825.1 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-minify branch Aug 27, 2018

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