-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Upgrade uglifyJS webpack plugin, disabling compress and enabling parallel #21809
Upgrade uglifyJS webpack plugin, disabling compress and enabling parallel #21809
Conversation
@@ -205,7 +205,7 @@ | |||
"trunc-html": "1.0.2", | |||
"trunc-text": "1.0.2", | |||
"type-detect": "^4.0.8", | |||
"uglifyjs-webpack-plugin": "0.4.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, so we weren't even using this node module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According the plugin docs we were, but in the very last available version under the alias 😄
from: https://github.com/webpack-contrib/uglifyjs-webpack-plugin#uglifyjs-webpack-plugin
webpack < v4.0.0 currently contains v0.4.6 of this plugin under webpack.optimize.UglifyJsPlugin as an alias. For usage of the latest version (v1.0.0), please follow the instructions below. Aliasing v1.0.0 as webpack.optimize.UglifyJsPlugin is scheduled for webpack v4.0.0
@@ -365,12 +366,13 @@ export default class BaseOptimizer { | |||
'NODE_ENV': '"production"' | |||
} | |||
}), | |||
new webpack.optimize.UglifyJsPlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was using UglifyJS 0.4.6, which ships with Webpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unfortunately 😢
package.json
Outdated
@@ -205,7 +205,7 @@ | |||
"trunc-html": "1.0.2", | |||
"trunc-text": "1.0.2", | |||
"type-detect": "^4.0.8", | |||
"uglifyjs-webpack-plugin": "0.4.6", | |||
"uglifyjs-webpack-plugin": "1.2.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we ^? this missing ^ predates our yarn.lock file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review @jbudz changed 😄
💚 Build Succeeded |
💚 Build Succeeded |
I am seeing this cut the production optimize time in half (~150s to 70s) while drastically reducing the necessary heap. I am currently seeing an OOM in master, which has a default max-old-space-size of ~1.6G. With this PR, I am able to optimize within 900MB. The one downside to this PR is it slightly increases the JS bundles sizes. This is largely mitigated by the fact that Hapi is gzipping the responses. I feel this tradeoff is warranted, as there are a lot of ways we can cut down the size of these assets which I don't believe has ever been a priority.
|
compress: { | ||
warnings: false | ||
}, | ||
new UglifyJsPlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, If we don't care about mangling and compression, why minify in the first place? Removing this plugin would reduce the build time even further.
All it does right now is white space removal, which can be accomplish by babel itself and its already in the webpack config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vigneshshanmugam thanks for the feedback. You should check the code on last master as we have described why we still need it: https://github.com/elastic/kibana/blob/master/src/optimize/base_optimizer.js#L396
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info, I didn't check the recent changes. Makes sense now 😃
This change allow us to fix a blocker issue for
6.4
#21794 related with an OOM problem.It solves the OOM problem and also reduces the optmizer time in some cases by almost 50%. In combination with the WIP DLL work #20749 it would be powerful!
Besides the UglifyJS upgrade to the last version, which solves the OOM by itself, the final changes with compression disabled were already suggested by @jbudz in the past in #16317.
The final user impact of disabling compression is completely negligible as we are serving the bundles gzipped.
You can found the size differences, for the served bundles, in the following screenshots for both cases:
Bundle Sizes With Compress OFF
**Bundle Sizes With Compress ON
**