🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@markelog
Copy link
Member

@markelog markelog commented Mar 2, 2018

raw     gz Compared to last run
 =     +1 dist/jquery.js
-31    -31 dist/jquery.min.js

Summary

Checklist

@markelog markelog force-pushed the uglify branch 2 times, most recently from 9c151cd to 447acd4 Compare March 2, 2018 17:26
Gruntfile.js Outdated
banner: "/*! jQuery v<%= pkg.version %> | " +
"(c) JS Foundation and other contributors | jquery.org/license */",
compress: {
"pure_getters": true,
Copy link
Member

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.bar or foo["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.

Copy link
Member Author

@markelog markelog Mar 3, 2018

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

@mgol mgol Mar 5, 2018

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.

Copy link
Member Author

@markelog markelog Mar 5, 2018

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
Copy link
Member

@mgol mgol left a 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

@markelog
Copy link
Member Author

markelog commented Mar 5, 2018

We probably can play with the options a little bit more, but we can do that in another season :).
Yet again - nice work @mgol :)

@markelog markelog closed this in 09684ba Mar 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 1, 2018
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.

3 participants