Skip to content

Conversation

@markelog
Copy link
Member

  1. Some Uglifyjs options might work great for minification, but worsens gzip size, disabling these options will increase minified size but decrease gzipped one,
  2. Sometimes changing sequence of variables initialization might save couple of bytes in gzipped size, in this PR, i also propose find optimal sequences of those variables that don't have an assigment. I did it with limitation of 7 items in sequence, i.e. here is 11 variables with undefined as 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.

@gibson042
Copy link
Member

I've contemplated doing this for a while, and actually seeing it leaves me awestruck. Bravo, @Orkel, bravo. 👏

@dmethvin
Copy link
Member

Wow!

all possible permutations for variables without an assigment is 39916800, i checked only 5040 of them.

Lazy! 😸

I'll land this today before it gets stale.

@jaubourg
Copy link
Member

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:

  • managing the order of variables is absolutely unmaintainable so the first patch we add, boom, +70bytes min/gzipped for no apparent reason,
  • the order of variable declarations sometimes have a meaning, for instance, some vars are grouped to make logical sense in parts of the code. Do we really want to make the code more difficult to read/maintain?
  • How aggressively is the minified version size augmented by this ? Everyone is not using gzip. I'd love to have stats that actually establishes the majority does. Also, the augmentation of the minified size would be nice to have.

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 jquery.js to find the best. One could argue euristics could be found, but even ordering by number of occurences of the variables in the code is not good: it's all about repeating patterns throughout the file so it's about naming variables so that the same code appears throughout various functions/methods. So I seriously doubt the approach is feasible anyway.

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.

@dmethvin
Copy link
Member

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.

@jaubourg
Copy link
Member

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.

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 $.ajax) makes it much more difficult to maintain and fix stuff.

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?

@dmethvin
Copy link
Member

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.

@jaubourg
Copy link
Member

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?

@dmethvin
Copy link
Member

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.

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 dmethvin closed this Jan 31, 2013
@jaubourg
Copy link
Member

@dmethvin comment and close did we?

@jaubourg jaubourg reopened this Jan 31, 2013
markelog added a commit that referenced this pull request Jan 31, 2013
Change uglify-js options for compressor
Change variables initialization sequence for some declarations
@dmethvin
Copy link
Member

Okay now it should be closed.

@dmethvin dmethvin closed this Jan 31, 2013
@mishoo
Copy link

mishoo commented Jan 31, 2013

@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.

@jaubourg
Copy link
Member

@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 )var a,b,c,d,e... at the beginning of most functions probably does more to help gzip than having )var d,f,e,a,b... and )var a,f,e,d,b... because of variables with more occurences but declared with an init, if you see what I mean.

@curiousdannii
Copy link

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?

@jaubourg
Copy link
Member

I dunno, maybe the best approach would be to put the inits after the decls. So something like this:

var string = "hello",
     i, length,
     bob = "woops";

Would become:

var a,b,c,d;
c = "hello";
a = "woops";

Provided vars are sorted by occurences.

Dunno if that makes sense.

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
…gh-1151.

Change uglify-js options for compressor
Change variables initialization sequence for some declarations
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants