My JavaScript book is out! Don't miss the opportunity to upgrade your beginner or average dev skills.

Monday, November 09, 2009

Google Closure ? I'm Not Impressed

We all know that Google is synonym of performances, simplicity, and again performances. A search engine that uses a truncated body for a not valid W3 markup should be the most bytes and performances maniac in the current web era, isn't it?
Well, Google Closure Tools has been a negative surprise, at least this is what I can tell about it after a first quick review.

Closure Compiler


It's since ages I am wondering what kind of tool Big G is using to produce their scripts and finally we got the answer: Closure Compiler ... ooooh yeah!
Packer, YUIC, it does not matter, when Google needs something, it creates something. This is almost intrinsic, as developers, in our DNA: we spot some interesting concept? We rewrite it from the scratch pretending we are doing it better!
This is not the case, or better, something could go terribly wrong!

// ==ClosureCompiler==
// @compilation_level ADVANCED_OPTIMIZATIONS
// @output_file_name default.js
// ==/ClosureCompiler==

(function(){
"use strict";
this.myLib = this.myLib || {};
}).call(this);

myLib.test = 123;

The produced output:

(function(){this.a=this.a||{}}).call(this);myLib.test=123;

And 2 warnings:

JSC_USELESS_CODE: Suspicious code. Is there a missing '+' on the previous line? at line 2 character 4
"use strict";
^
JSC_USED_GLOBAL_THIS: dangerous use of the global this object at line 3 character 4
this.myLib = this.myLib || {};
^

Excuse Me?
The sophisticated compiler is able to understand the "use strict" ES5 activation statement and the fact we are passing the global object as this reference in the closure. It does not matter, as showed in the produced code the result will be a broken library, thanks to its missed name, magically transformed into "a".
Advanced Optimization Could Fail to both give us right suggestions and fix or optimize the code, since in that case, as example, this.a won't perform anyhow faster than original this.myLib.
Advanced Optimization parameter could also be dangerous for lazy loaded libraries.
We need to be extremely careful with this option and, as result, rather than a Compiler, we will deal with a "code messer" where hard debug will become automatically the hardest ever.
Read Carefully This Page if you are planning to use this option because under the flag "best ratio" and "removed dead code" we could have massive surprises in the middle of the application.

As summary, SIMPLE_OPTIMIZATIONS as compilation_level directive is so far the recommended one, but at the same time it won't offer that different ratio compared against YUI Compressor or Dean's Packer (NO base62) produced outputs while ADVANCED_OPTIMIZATIONS could be tested for single stand alone files hoping these won't break the global namespace via renamed variables.
In this case a JavaScript closure, the real one, is an absolute must!

Closure Library


This is another part, not strictly related with the Compiler, but apparently able to work with it. The Closure Library is a wide namespace loads of core features and a cross browser User Interface. I have to admit this library is a massive piece of work, but techniques used to make it happen are often hilarious.
First of all, this is the first time I read protected variables called with an underscore at the end, rather than as first char:

// normal code
function Constructor(){
this._protected = [];
};

// Closure Library Creativity
function Constructor(){
this.protected_ = [];
};

Why On Earth? Python style a part, where the underscore has a concrete meaning, the technique to use underscore as first character has a reason to exists.

function copyOnlyPublic(o){
var $o = {}, k;
for(k in o){
if(k.charAt() !== "_")
$o[k] = o[k]
;
};
return $o;
};


var myC = new Constructor;
var $myC = copyOnlyPublic(myC);

No Way! To make the style "creative" the charAt method with optional 0 as argument needs to become:

if(k.charAt(k.length - 1) !== "_")

Is this what we would expect from the performances king? I don't think so.
Is this faster to read at least for human eyes? Neither!
Gotchas are everywhere in the library ... the most redundant stuff I've ever seen is the array namespace!

goog.array.indexOf = function(arr, obj, opt_fromIndex) {
if (arr.indexOf) {
return arr.indexOf(obj, opt_fromIndex);
}
if (Array.indexOf) {
return Array.indexOf(arr, obj, opt_fromIndex);
}

var fromIndex = opt_fromIndex == null ?
0 : (opt_fromIndex < 0 ?
Math.max(0, arr.length + opt_fromIndex) : opt_fromIndex);
for (var i = fromIndex; i < arr.length; i++) {
if (i in arr && arr[i] === obj)
return i;
}
return -1;
};

OMG, I can't believe performances matter only for a missed body tag in the layout ... it cannot be real, can it?
What we have there? 1 to 2 possibly missed checks for each call (IE) and everything just to emulate the native Array.indexOf which is present in basically every browser except truly old or Internet Explorer?

goog.array.indexOf = Array.indexOf || function(arr, obj, opt_fromIndex) {
var fromIndex = opt_fromIndex == null ?
0 : (opt_fromIndex < 0 ?
Math.max(0, arr.length + opt_fromIndex) : opt_fromIndex);
for (var i = fromIndex; i < arr.length; i++) {
if (i in arr && arr[i] === obj)
return i;
}
return -1;
};

Array.indexOf is used as fallback if for some unknown reason (and browser...) an Array has not indexOf but Array.indexOf is present ... well, if we can trust that case is there any valid reason to create a performance gap like that for almost every method?
forEach, lastIndexOf, every JavaScript 1.6 or greater emulated method contains redundant checks performed for each call ... where is the performance maniac here?
The feeling is that this library has been created by some Java guy, probably extremely skilled with Java, but definitively not that clever with JavaScript programming style. Google if you need skilled JS developers there are hundreds of us out there. What I mean is that it does not matter if a minifier is able to remove dead code because dead code should not be there at all, isn't it?

// WebReflection Suggestion
goog.array.clone = (function(slice){
try{slice.call(document.childNodes)}catch(e){
slice = function(){
var rv = [];
if(this instanceof Object)
// suitable for arguments
// and every other ArrayLike instance
return rv.slice.call(this)
;
for (var i = 0, len = this.length; i < len; ++i)
rv[i] = this[i]
;
return rv;
};
};
return function(arr){
return slice.call(arr);
};
})(Array.prototype.slice);

Above suggested snippet is based on Features Detection, a technique apparently completely discarded in those files I've read in this library.

Features Detection Cons

  • performed runtime, few milliseconds before the library or function is ready to use


Features Detection Pros

  • performed once, and never again for the entire session
  • best performances, browser independent and browser focused at the same time
  • being usually based over most recent standards, features detections could cost a bit more only for deprecated, obsolete, or truly old browsers


Moreover!

If the problem is the wasted millisecond to perform a feature detection, we can always fallback into lazy feature detection.
I agree that web performances are more about download time and round trip, but if Google has V8 as engine monster, do we agree that better JavaScript practices could make even V8 faster?

Lack Of Creativity

Even if this library uses some weird practice, most logical and common techniques to speed up execution and reduce code are often not considered. As example, this is just one method we can find in the crypt file:

goog.crypt.byteArrayToString = function(array) {
var output = [];
for (var i = 0; i < array.length; i++) {
output[i] = String.fromCharCode(array[i]);
}
return output.join('');
};

Oh Really? So now a JavaScript developer should create a function which accept an array in order to create another array to populate over a loop and a global method call to perform a join at the end? That's weird, I thought that method could have been written in this way:

// WebReflection Suggestion
goog.crypt.byteArrayToString = function(array) {
return String.fromCharCode.apply(null, array);
};

... but maybe it's just me that noob that I cannot spot the difference except better performances over less code ... please enlighten me!

Closure Library, The Good Part

Is massive, for basically each file there is a test case so it is robust. For sure as we have seen before, this is not the fastest general purpose library we could find in the net, something I was expecting from Big G, but hey ... we cannot blame the excellent work done so far, can we?

Closure Templates

Well, here I am almost without a word ... I mean, the number 1 search engine able to create monster pages via JavaScript templates? I do hope those files will be performed everywhere but a web page and runtime, 'cause with incoming HTML5 I feel horrified thinking about a new web loads of document.write and nothing else. Where is the semantic? Where is the logic applied before and not during execution? Where are best practices? Looking at examples even V8CGI project is not able to perform that code ... what is that, exactly? And WHY???
The only thing I do like there is the definition file, something simple, clever, easy to parse, exactly the opposite of its implementation via JavaScript: please avoid it!

Closure Conclusion

No this is not another piece of the puzzle, just the end of this post. I've been probably too aggressive and it's only thanks to Google decision that I can write a post like this: Open Source is the key, or your libraries, operating systems, whatever, will be always behind 'cause developers able to help you are not only in your team.
I was expecting excellent ideas, new killer techniques unknown for everybody else, what I have found is "yet another toolkit". I am always up to contribute so if interested put my name in the committers list, I have already lot of stuff to optimize because a Compiler, whatever it does, cannot create a better code, it can simply try to make a bit better and shorter the existent one: never delegate skills to a machine until these will be able to take decisions for us!

20 comments:

Anonymous said...

Can’t agree more. In fact I found lots more of ridiculous stuff inside closure. I am pretty sure you can do the same with ease. Just another proof that it was written by Java guys:
Math.round(factor * rgb1[0] + (1.0 - factor) * rgb2[0])
Who else will write “1.0” to ensure it is float?
BTW, coming to your first paragraph, I did a tiny research of the home page of Google. It is not as minimised as we use to think: The Emperor’s New Clothes

kangax said...

Another unsafe thing I noticed about Closure Compiler is the way it turns function expressions into function declarations.

var f = function(){};

becomes:

function f(){};

These are far from being equivalent, of course, so we end up with harmful behavior where:

alert(typeof f);
var f = function(){};

becomes:

alert(typeof f);
function f(){}

completely changing program output.

Andrea Giammarchi said...

ah ah, @dmitry that's another prove. We all know in JavaScript 1.0 === 1 don't we?

@kangax did you try with advanced? That is quite obtrusive change maybe var f is considered dead code ...

I am sure a tool able to parse JavaScript and tell us where and when some piece of code is unreferenced is more than welcome, but a "compiler" able to make debug harder 'caues it's too greedy, well I would not call Compiler Enterprise Ready if used with advanced option, but I'll test more ASAP

Anonymous said...

I think this code dates 2003-2004 and then it was probably best organized and most innovative JS library, now clearly it isn't.
It is nice to see code that build such legendary products but I'm also not tempted to use it.

Andrea Giammarchi said...

@medikoo.com are you saying this is abandoned code? This would make things even worse!

I'd love to use the library only if they'll allow me to commit changes so I can reduce size and improve performances. The way the lib is organized is truly good (so not everything is crap)

Kamil Trebunia said...

I believe that changing function expressions (as kangax named it) into function declaration is there, because 'var f = function(){};' creates an anonymous (unnamed) function which aren't that nice to debug with Firebug (not sure about the other tools), because stack trace only shows stack of anonymous functions. On the other hand even with stack trace of anonymous functions we do know where they were declared so with that Firebug extension that Google also created I guess it shouldn't matter. Difficult to say - it's often easier to talk than to implement.

Sam-I-Am said...

Well arr[i] == "thing" is the slowest way to populate an array that I know of..

@Andrea and others, just send along your patches. Just because its google doesn't mean it cant be improved. Closure is apparently *good enough* for production use, but constructive criticism and proactive attitude are always appreciated, no matter the size or reputation of the company.

kangax said...

@Andrea

I tried with simple and andvanced, and they are both harmful like that. Try compiling:

alert(f());
var f = function(x){
return 'redefined';
};
alert(f());
function f(x){
return 'original';
}

@Kamil

Even if func. expressions are translated into func. declarations for debugging purposes (I don't think that's the case), it should be done in a safe manner.

Besides, nothing (well, almost nothing) is stopping you from naming function expressions as well.

fearphage said...

Someone pointed out that `if(this instanceof Object)` fails across frames. This code is all designed to be used across frames. Aside from that, I think you nailed it. It appears to me that Java developers were writing javascript or this is just a really old library. All the browser sniffing (there's lots) also put me off.

Andrea Giammarchi said...

@fearphage I do believe 80% of cases that function will be used in the same window context but in any case that is a single reasonable check specific for IE and nothing else rather than 2 checks plus fallback without that "instanceof" possibility and even for standard/updated browsers.

Andrea Giammarchi said...

All, the reason I've been nit'n'pick with this Closure stuff is simple: Google claims best JavaScript hackers and if this is the result I feel like somebody is kidding me.

JavaScript is not properly studied at University, where the OO model is the classic one, and there's no degree/Phd about JS logic and techniques.

D. Crockford is just one of the examples that JavaScript is more about experience and that we have to deal with a flexible but extremely difficult language due to numerous VM and missed standards (with few gotchas in ECMA 262 itself)

Google is probably hiring Java developers with whatever degree pretending they are best JavaScript hackers? AFAIK Google should know that JavaScript is not Java, an error lot of people do out here.

Anyway, hopefully they know it, but reading the library code I am starting to think they are seriously missing real JavaScript hackers and since Google uses JavaScript for everything ... well, think about how many other "things" it could have done with real these developers.

Some error is not even about the language (crypt method as example) but a logical operation ... Google has extremely interesting projects and best coders for everything but for the main used programming language, JavaScript, there's no interest in devs like me or others?

Missed expectations and a bit deluded about what I've found here, that's it about this post.

P.S. I have asked in the ML if I can commit, I'll keep the post updated as soon as I'll know the answer.

Andrea Giammarchi said...

Again not surprised, they are not ready for external committers ... well, Google, what can I say, you have my CV there since ages, use it!

(... but I am sure after this post and related comments they would call me just for a fight and nothing else, lol ...)

Anonymous said...

@Andrea

I don't think it's abandoned.. but probably since then they're just maintaining code. They don't feel about making revolution as "code is already good enough". Probably :)

Anyway in Google closure I smell strong background of some other language that's not that close to JavaScript. When they wrote it, they didn't think in ways we (JS hackers) do.. and probably that's why we're a bit disappointed :)

Anonymous said...

Just want to link to SitePoint “interview” of mine concerns about Closure: http://tr.im/badclosure

Andrea Giammarchi said...

@kangax

var foo = (function(){
var foo;
if(whatever) {
foo = function foo(){/* 1 */};
} else {
foo = function foo(){/* 2 */};
};
return foo;
})();

Above one is safe without ADVANCED_OPTIMIZER.
var foo=function(){var a;return a=whatever?function b(){}:function b(){}}();
correctly referenced via var a inside the function body, if necessary.
with ADVANCED_OPTIMIZER things change:
(function(){return whatever?function a(){}:function a(){}})();
IE should have the fallback as last possible one ;-)
But I agree this is more about ADVANCED option gotcha

Unknown said...

Re: Closure Compiler
The docs for advanced compilation comes with a big red flag. Your code must be tailored for advanced to work. The author of this blog should know that, because he linked to the docs page. Yet he *didn't* tailor his code and then whined when it didn't work.

Sorry, but that's your own damn fault.

Maik K said...

Your conclusion on the Closure Compiler is right. And totally wrong.

First of all, you can't really compare packer or YUI with the CC for a simple reason: the CC >>compiles<< as it name says, other engines just compress the scripts. So the CC will produce not just less bytes of code, it also will optimize the code.

If you read the docs you will see that there are plenty of tips how to use it and that you just can't use some coding styles. I myself used the compiler for some tests and I have to say if you follow the tips in the documentation you can get short, working code thats cleaned up.

Saying that it wouldnt have performance improvements is just dumb. I compile my files to an avarage of 20 to 25% of the original size, what is way better than packer or YUI ever did and saving bandwith IS an improvement. Also it kinda seems that unlike packer the scripts don't need to be "unpacked" on the client because it's just the native code compiled, without the need of eval or base62 shrinks(which indeed needs some ms to parse/decode), so the CC runs also faster.

Also the CC needs either unobstrusive JS(inline JS is one of the things that bugs me most if I look at the scripts of other developers.. what's so hard about using getElementById or using $ and $$ functions like in most JS libraries?!) or a simple JS file you ommit to the compiler as externs( also described in the docs)

So before bashing again something you don't understand first get the technical background... Google did a great job with the compiler and if you just can't understand how to use it( don't get me wrong but most ppl if shown it were just confused) - that might be because the have some of the best programmers ever, that think way more abstract and complex most people/programmers do..

Andrea Giammarchi said...

Maik, you did the same, commenting without understanding what you were commenting ... but it does not matter, old post, the compiler has been improved/changed in the meanwhileso if you are happy, we are ;)

Maik K said...

Oh snap, I just saw the release date of the post xD Sry for that, but even back then it was pretty nifty though, you just had to think like the guys from google did - even it was sometimes coding 'around' the problems ;P

Andrea Giammarchi said...

they thought in "Java therms" and this was the mistake #1 - you cannot hire JavaScript programmers demanding Java as CV requirement, simply two different worlds and by that time, before this post (out basically the day after) and many others, it was more than evident ;)