-
Notifications
You must be signed in to change notification settings - Fork 20.5k
#13355 - Uglifyjs compression options and variable sequence optimization #1151
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
Conversation
Change variables initialization sequence for some declarations
|
I've contemplated doing this for a while, and actually seeing it leaves me awestruck. Bravo, @Orkel, bravo. 👏 |
|
Wow!
Lazy! 😸 I'll land this today before it gets stale. |
|
I don't want to spoil the party and I appreciate the enormous (ENORMOUS) work involved but I think we're going way overboard here:
Like @gibson042, I toyed with the order of var decls in files to see huge change in min/gzip size. However I always thought the process of re-ordering the var decls without assignments should be automatized. Let a tool handle this and we won't have to think about it or wonder if we found the best permutations. Problem is there are 39916800 permutations as @Orkel pointed out, so it would mean uglyfying 39916800 Maybe we can lax some uglify js rules so that it gzips better but re-ordering variables like this is non-sense. It is an incredibly costly 1.05% saving in regard to maintenance and readability costs. |
|
I'm not that worried @jaubourg. It's like cleaning the bathroom. You know it will get dirty again, and that you can't clean it perfectly, but that shouldn't stop you from trying to clean it. It would be great if Uglify had some knowedge of gzip so that it could optimize the variable name order and length, maybe we can talk @mishoo into it. |
Except it does make it messier from a maintenance point of view. It's not about cleaning it's about re-arranging the dirt so that the help (uglify) can clean better. Re-ordering var decls and separating related decls (as is particularly evident in We're going overboard here. That's definitely not something we wanna do manually. There's a reason why it hasn't been done already which makes it more than likely that it won't be done again. It's a hella lot of work that disrupts the natural reading of the unminified code and needs constant re-adjustements. Putting decls without init value on top was already a stretch (but something easy to maintain so the trade-off was ok). All this work and disruption for 1%? And again, what is the size increase minified NOT gzipped? |
|
There is no additional ongoing maintenance burden introduced by this pull request. I want to make it clear, there is no new requirement that future pull requests and patches be processed through all these permutations to minimize their size. Pull requests should not be rearranging unrelated parts of a file in order to keep the size of the particular patch smaller than it would otherwise be. That just clutters the diffs and makes them harder to follow. The reason I like this pull request is precisely because its purpose is clear and singular. And again, it does not create any future obligations, period. |
|
Yes, PRs shouldn't re-arrange unrelated parts of a file in order to keep the size small. Because it's disruptive and borks existing code. Apply this kind of patch JUST before a release and revert it JUST after if you want the byte savings. But changing code "for real" for a temporal gain makes absolutely no sense. This IS a disrupting patch that changes the natural reading of code throughout the entire code base, yet will progressively lose the gain as the code is reworked. So you're changing code across the board, making it harder to follow/maintain (it's about having stuff in a meaningful order, not about having to re-order all the time, btw), for 346 bytes you'll lose at the first patch declaring variables? Is this for real? And we still don't have the impact on non-gzipped minified which is most probably the most commonly used version of jQuery? Are we that desperate to look good on the front page of our site? |
I don't find it to make the code harder to read. In fact I think you would be hard-pressed to know this patch was applied at all. Yes, there is the chance we'll lose some or perhaps even all the gains over time, don't worry about it. Just like cleaning the bathroom: Go in, do your business, and leave. |
|
@dmethvin comment and close did we? |
Change uglify-js options for compressor Change variables initialization sequence for some declarations
|
Okay now it should be closed. |
|
@dmethvin I'd like to put more gzip love into UglifyJS. I'm lagging behind on the issue tracker, but any case, please file a ticket and I'll find some time to look into it. By which criteria were those variables reordered? If it's something as simple as “most referenced first”, then it should be pretty easy to hack into UglifyJS. |
|
@mishoo sadly, it's not as simple as that, though I guess number of occurences would help. It's more about renaming variables so that the overal script exhibits repeating patterns. So you'd have to consider permutations to variable declarations throughout all functions to obtain the kind of optimization we have here. That being said, a number of occurences approach would be nice but the fact the top variable decls we have in out source base are the ones with no initialization and thus we always end up with something like |
|
Sorting uninitialised variables alphabetically by their mangled names would be a fairly straightforward change I'd imagine. That wouldn't be perfect because some names would be skipped in some functions, but would it provide enough benefit? |
|
I dunno, maybe the best approach would be to put the inits after the decls. So something like this: Would become: Provided vars are sorted by occurences. Dunno if that makes sense. |
…gh-1151. Change uglify-js options for compressor Change variables initialization sequence for some declarations
undefinedas initialization value, so all possible permutations for variables without an assigment is 39916800, i checked only 5040 of them.Same optimizations could be applied to 2.0 source and for variables with (re)assigment but i did not finish AST analysis for those possibilities.
Anyway, 1.9 current gzipped size is 33006 bytes, in this branch it's 32642, all and all it's -346 bytes for gzipped size.