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

Let's talk about dead code elimination #314

Closed
andys8 opened this issue Oct 29, 2018 · 12 comments
Closed

Let's talk about dead code elimination #314

andys8 opened this issue Oct 29, 2018 · 12 comments

Comments

@andys8
Copy link
Contributor

andys8 commented Oct 29, 2018

Since dead code elimination is one of the core features of elm 0.19.

Create Elm App comes with an opinionated setup for dead code elimination which is disabled by default, because it may break your code.

Does it make sense to disable dead-code elimination by default?

I guess DEAD_CODE_ELIMINATION is about the uglify settings, but elm-webpack-loader would still set the --optimize flag.
Since the elm documentation itself describes how to run uglify, it should be pretty safe to enable the settings by default.

An alternative approach would integrate elm-minify which is implementing a webpack plugin. It's about adding a dependency, but maintaining less directly (in cases this will be improved in the future). In any way the implementation is interesting, since there are differences in the settings.
https://github.com/opvasger/elm-minify/blob/master/src/api.js

@halfzebra
Copy link
Owner

halfzebra commented Oct 30, 2018

Hi @andys8, thanks for creating this issue!

I think the primary source of confusion is that the documentation was not yet updated to describe the changes of create-elm-app v2 #283. Before 0.19 it was possible to use UglifyJS for dead code elimination, but sometimes it broke th JavaScript, produced by Elm Compiler.

In my opinion, introducing elm-minify does not add any value, since we could inherit the config and enable it by default(if it's stable enough).

Here's what I can propose to solve this issue:

  • Remove DEAD_CODE_ELIMINATION env variable
  • Replace UglifyJS config with the one that could be used out of the box without any changes and disable Dead Code Elimination in UglifyJS
  • Keep the --optimize flag enabled for the elm-app build

This would reduce the unnecessary configuration and provide a less confusing build optimization setup.

How does that sound?

@andys8
Copy link
Contributor Author

andys8 commented Oct 30, 2018

Sounds awesome!

@halfzebra
Copy link
Owner

Are you interested in working on a PR?

@andys8
Copy link
Contributor Author

andys8 commented Oct 31, 2018

I could look into getting a PR started, but I'm not familiar with Webpack plugins. Therefore I'm not confident in implementing the details of the second bullet point and making sure nothing broke.

@andys8
Copy link
Contributor Author

andys8 commented Oct 31, 2018

The interesting part is, that the elm docs suggest running uglifyjs twice (which sounds like a workaround in the first place).

uglifyjs elm.js --compress 'pure_funcs="F2,F3,F4,F5,F6,F7,F8,F9,A2,A3,A4,A5,A6,A7,A8,A9",pure_getters,keep_fargs=false,unsafe_comps,unsafe' | uglifyjs --mangle --output=elm.min.js

https://guide.elm-lang.org/optimization/asset_size.html

And that's probably the case why elm-minify is not using the uglifyjs Webpack plugin, but rather implementing a custom Webpack plugin, calling the uglifyjs library twice.

https://github.com/opvasger/elm-minify/blob/71ef1e75d20e199d96bdf8c5bf0e5a7a5f914353/src/api.js#L27

var minify = function (elmJs, compressionPasses) {

    var compressionResult = ujs.minify(elmJs,
        toUglifyJsCompressionConfig(compressionPasses)
    )

    var mangleResult = ujs.minify(compressionResult.code,
        uglifyJsMangleConfig
    )

    return mangleResult.code
}

Again, not too familiar with Webpack configs, but couldn't we have the UglifyJS Plugin definition just twice with different settings? Order is probably deterministic.

@halfzebra
Copy link
Owner

halfzebra commented Oct 31, 2018

I think they are referring to the UglifyJS configuration property called passes, which is responsible for running the optimizations more than one time.

If I'm not mistaken, the statement about increasing the number of passes is coming from the following discussion elm-community/elm-webpack-loader#142 (comment)

UglifyJsPlugin runs UglifyJS on the code and it is configured by passing the uglifyOptions. What we need is to combine the current configuration with the one from elm-minify and remove Dead Code Elimination stuff.

How does that sound?

@andys8
Copy link
Contributor Author

andys8 commented Oct 31, 2018

Yeah, right. Thanks for the hint for the discussion about passes.

So there are two things:

  1. Uglifyjs is called to compress with 1-3 passes (depending on who you refer to)
  2. The compressed result is again put into uglifyjs with mangle options

A total of 2-4 runs. My comment above was targeting, how to run uglifyjs with different settings sequentially.

But Richard also wrote at the end of the comment, that using uglify-es instead of uglify-js works with a single call.
elm-community/elm-webpack-loader#142 (comment)

Also interesting:

webpack-contrib/uglifyjs-webpack-plugin now has uglify-es support

@halfzebra
Copy link
Owner

halfzebra commented Oct 31, 2018

You are right, I did not check the code very carefully.

After looking into this for some time, I think it might be worth checking https://www.npmjs.com/package/terser-webpack-plugin instead of the UglifyJS plugin.
It seems to replace uglify-js and uglify-es with higher scores in compression.

I suspect that it would be okay to use Terser with a single pass with mangle and compress options on.

Do you think it's worth running multiple passes?

@andys8
Copy link
Contributor Author

andys8 commented Oct 31, 2018

The easiest approach would be to use the provided webpack plugin of elm-minify and measure the output size as reference for comparison. Afterwards we could use UglifyWebpackPlugin or TerserWebpackPlugin and compare if it's worth doing multiple passes (by hand or passes param).

Do you think it's worth running multiple passes?
Can't speak for everybody, but to me the bytes saved by elm-community/elm-webpack-loader#142 (comment) aren't worth the passes.

The more important part is the note in the elm docs, which suggests, that there is an issue, where standard ugligyjs (commandline) usage will not work as expected, which lead to the manually sequenced calls.

Note 1: uglifyjs is called twice there. First to --compress and second to --mangle. This is necessary! Otherwise uglifyjs will ignore our pure_funcs flag.

@halfzebra
Copy link
Owner

Sounds fair 👍

@andys8
Copy link
Contributor Author

andys8 commented Nov 1, 2018

@halfzebra Opened PR #315 :)

@halfzebra
Copy link
Owner

Thanks for your work, your PR has been released with the latest patch release.

Check it out! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants