-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Build: Seasonal update of uglify and its options #3994
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
9c151cd to
447acd4
Compare
Gruntfile.js
Outdated
| banner: "/*! jQuery v<%= pkg.version %> | " + | ||
| "(c) JS Foundation and other contributors | jquery.org/license */", | ||
| compress: { | ||
| "pure_getters": true, |
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.
Hmm, what does it exactly do? The Uglify README mentions:
UglifyJS will assume that object property access (e.g.
foo.barorfoo["bar"]) doesn't have any side effects
but technically we can't guarantee that as jQuery.event.addProp creates a getter utilizing a provided hook.
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.
My understanding is going from something like this:
foo.bar++;
if (foo.bar) {
console.log('test');
}to something like this:
++foo.bar && console.log("test");In the first case, If you have a getter on "bar" it will be executed twice, in the second case – once i.e. we should be fine
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 see, that meaning of the flag makes sense. On the other hand, I imagine something like the following:
var bar,
flag = foo.a;
if (flag) {
bar = flag + 1;
}might be minified to (I'm skipping other optimizations here):
var bar;
if (foo.a) {
bar = foo.a + 1;
}which would invoke a getter twice which - in turn - may lead to a performance penalty as the getter is invoked twice.
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.
Well, we don't have getters anyway
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.
We do, our event object is implemented via getters now.
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.
Those event getters are super fast and should be pure functions. It is also normal and expected that they will be called many times.
The potential of calling those getters multiple times for the benefit of size reduction sounds good to me.
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.
OK, I've checked what's the difference between the output with pure_getters on and off and it was huge, 57 bytes gzipped. I prettyfied the code then and compared the two outputs and I saw that one of the differences is pure_getters cuts out this:
https://github.com/jquery/jquery/blob/3.3.1/external/sizzle/dist/sizzle.js#L193-L195
// Support: Android<4.0
// Detect silently failing push.apply
arr[ preferredDoc.childNodes.length ].nodeType;and this: https://github.com/jquery/jquery/blob/3.3.1/external/sizzle/dist/sizzle.js#L1521-L1523
// Accessing this property makes selected-by-default
// options in Safari work properly
if ( elem.parentNode ) {
elem.parentNode.selectedIndex;
}and this: https://github.com/jquery/jquery/blob/3.3.1/src/attributes/prop.js#L106-L109
var parent = elem.parentNode;
if ( parent && parent.parentNode ) {
parent.parentNode.selectedIndex;
}and this: https://github.com/jquery/jquery/blob/3.3.1/src/attributes/prop.js#L113-L123 (the whole setter body)
etc.
In other words, in our code getters are sometimes invoked for side effects only so we cannot enable this flag, unfortunately.
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 find @mgol, pure_getters option is removed
raw gz Compared to last run = = dist/jquery.js -294 -88 dist/jquery.min.js
mgol
left a comment
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.
Too bad it's no longer 88 bytes, that looked sweet. Nonetheless, 31 bytes is still a significant improvement! :) LGTM
|
We probably can play with the options a little bit more, but we can do that in another season :). |
Summary
Checklist