Support: Reduce size #1518

Closed
wants to merge 9 commits into
from

Projects

None yet

5 participants

@gibson042
Member

Pros:

   raw     gz Sizes
282255  84119 dist/jquery.js
 95647  33205 dist/jquery.min.js

   raw     gz Compared to 1.x-master @ a96ff1c3a864cee27ac3014b63b7054bb938c30d
 -1824   -222 dist/jquery.js
  -883   -247 dist/jquery.min.js

Cons:

  • Consolidated CSS support tests may incur a few ms extra delay on first run
@markelog

What these comments should accomplish?

Owner

Breadcrumbs for future investigation. Such comments would be useful if either a code rewrite or a new minification strategy changes these variables (e.g., rewriting var div, body, container; body = … to var b,c,d=… instead of var b,c,d;c=…), but honestly I am on the fence about including them.

I would remove them, i'm pretty sure no one is crazy enough to look that deeply in this stuff as you are :-)

@markelog markelog and 1 other commented on an outdated diff Feb 19, 2014
src/attributes/support.js
// Setup
+ div = document.createElement( "div" );
@markelog
markelog Feb 19, 2014 Member

Is moving div variable from var declarations helps with the size?

@gibson042
gibson042 Feb 19, 2014 Member

Yes, because it lets div get minified to b (like it is in the other similar support tests) instead of e.

@markelog markelog and 1 other commented on an outdated diff Feb 19, 2014
src/css/support.js
div.style.backgroundClip = "content-box";
div.cloneNode( true ).style.backgroundClip = "";
support.clearCloneStyle = div.style.backgroundClip === "content-box";
// Null elements to avoid leaks in IE.
- a = div = null;
+ a = style = div = null;
@gibson042
gibson042 Feb 19, 2014 Member

For minification to d=c=b=null, which shares a common suffix with analogous expressions in the other similar support tests.

@markelog
markelog Feb 19, 2014 Member

Do i understand this correctly, you null the style var only because it's compresses better?

@gibson042
gibson042 Feb 19, 2014 Member

Well, I introduced style for better compression, but having done so I think it needs nulling anyway.

@markelog
markelog Feb 20, 2014 Member

There is know memory leak problem associated with native properties, if you would introduce an expando that would be another story, but nulling the style prop is unnecessary

@markelog
Member

cc @mzgol

@markelog markelog and 2 others commented on an outdated diff Feb 19, 2014
src/effects/support.js
- // Null elements to avoid leaks in IE.
- body = container = div = null;
- }
+ // Null elements to avoid leaks in IE.
+ container = body = div = null;
@markelog
markelog Feb 19, 2014 Member

I wonder if we could remove these, since we almost dropped support for IE6, whereas these kind of hacks should be relevant only for IE6 before service pack patch, but even if this code is leaking, this is not in a cycle or anything, so damage should be close to nothing.

@dmethvin ?

@mgol
mgol Feb 23, 2014 Member

@markelog Yeah, this is executed only once and if the scope of the problem is to unpatched versions of IE6... I'd remove that as well.

@gibson042
gibson042 Feb 23, 2014 Member

I'm convinced. Unless someone objects, I will remove remove reference nulling from the support setter functions.

@rwaldron rwaldron commented on the diff Feb 19, 2014
src/css/support.js
-
- body.appendChild( container ).appendChild( div );
-
- // Check if div with explicit width and no margin-right incorrectly
- // gets computed margin-right based on width of container. (#3333)
- // Fails in WebKit before Feb 2011 nightlies
- // WebKit Bug 13343 - getComputedStyle returns wrong value for margin-right
- marginDiv = div.appendChild( document.createElement( "div" ) );
- marginDiv.style.cssText = div.style.cssText = divReset;
- marginDiv.style.marginRight = marginDiv.style.width = "0";
- div.style.width = "1px";
-
- reliableMarginRightVal =
- !parseFloat( ( window.getComputedStyle( marginDiv, null ) || {} ).marginRight );
-
- body.removeChild( container );
@rwaldron rwaldron commented on the diff Feb 19, 2014
src/css/support.js
- tds = div.getElementsByTagName( "td" );
- tds[ 0 ].style.cssText = "padding:0;margin:0;border:0;display:none";
- isSupported = ( tds[ 0 ].offsetHeight === 0 );
-
- tds[ 0 ].style.display = "";
- tds[ 1 ].style.display = "none";
-
- // Support: IE8
- // Check if empty table cells still have offsetWidth/Height
- reliableHiddenOffsetsVal = isSupported && ( tds[ 0 ].offsetHeight === 0 );
-
- body.removeChild( container );
-
- // Null elements to avoid leaks in IE.
- div = body = null;
-
@mgol mgol and 1 other commented on an outdated diff Feb 23, 2014
src/css/support.js
@@ -4,89 +4,43 @@ define([
], function( jQuery, support ) {
(function() {
- var a, reliableHiddenOffsetsVal, boxSizingVal, boxSizingReliableVal,
- pixelPositionVal,
- div = document.createElement( "div" ),
- containerStyles = "border:0;width:0;height:0;position:absolute;top:0;left:-9999px",
- // Support: Firefox<29, Android 2.3 (Prefixed box-sizing versions).
- divReset =
- "-webkit-box-sizing:content-box;-moz-box-sizing:content-box;box-sizing:content-box;" +
- "display:block;padding:0;margin:0;border:0";
+ // Minified: b,c,d,e,f,g, h,i
@mgol
mgol Feb 23, 2014 Member

I'm pretty sure almost no one will understand what's going on in this comment. :) And if someone does, they'd be puzzled even more.

@gibson042
gibson042 Feb 23, 2014 Member

The thing is, related code exists in multiple source files (and they're not even siblings). I want to leave something to make it clear why a seemingly small future change might inordinately increase the size of jquery.min.js.gz. Would Minify: var b,c,d,e,f,g, h,i be a better syntax?

@mgol
mgol Feb 23, 2014 Member

Maybe leave Minified: as it is but add a var at the beginning?

@mgol mgol commented on an outdated diff Feb 23, 2014
src/manipulation/support.js
@@ -3,13 +3,13 @@ define([
], function( support ) {
(function() {
- var fragment = document.createDocumentFragment(),
+ // Minified: a,b,c
+ var input = document.createElement("input"),
@mgol
mgol Feb 23, 2014 Member

If you're moving this line around, you might as well fix the code style. ;)

@gibson042
Member

The latest and greatest:

   raw     gz Sizes
281830  84023 dist/jquery.js
 95576  33182 dist/jquery.min.js

   raw     gz Compared to 1.x-master @ a96ff1c3a864cee27ac3014b63b7054bb938c30d
 -2249   -318 dist/jquery.js
  -954   -270 dist/jquery.min.js

And with that, I think it's ready.

@dmethvin dmethvin commented on the diff Mar 1, 2014
src/manipulation/support.js
@@ -3,13 +3,13 @@ define([
], function( support ) {
(function() {
- var fragment = document.createDocumentFragment(),
- div = document.createElement("div"),
- input = document.createElement("input");
+ // Minified: var a,b,c
@dmethvin
dmethvin Mar 1, 2014 Member

Okay...this comment has me puzzled. More accurately, "these comments".

@gibson042
gibson042 Mar 2, 2014 Member

See #1518 (comment) and #1518 (comment). UglifyJS2 renames these variables to a, b, and c, and that's important to know when trying to maintain patterns across multiple files for high (gzip) compressibility.

@markelog
markelog Mar 3, 2014 Member

Still think this has very limited use, if developer understand how uglify works he could work this out himself

@gibson042
gibson042 Mar 3, 2014 Member

if developer understand how uglify works he could work this out himself

I'm actually counting on it. These comments are meant to document expectations about what should happen, so that such developers can use them for comparison with actual minification results.

@markelog markelog commented on the diff Mar 3, 2014
src/attributes/support.js
@@ -54,9 +55,6 @@ define([
input.value = "t";
input.setAttribute( "type", "radio" );
support.radioValue = input.value === "t";
-
- // Null elements to avoid leaks in IE.
- a = input = select = opt = div = null;
@gibson042
Member

Landed at b96522a

@gibson042 gibson042 closed this Mar 4, 2014
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@gibson042 gibson042 Support: Reduce size via code consolidation and minification awareness
Ref badcd1b
Closes gh-1518
ff26f26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment