Hacker News new | comments | show | ask | jobs | submit login

Some Sitepoint Article: How not to Critique a Javascript Library

    for (var i = fromIndex; i < arr.length; i++) {
    
    This for loop looks up the .length property of the array (arr)
    each time through the loop. Simply by setting a variable to
    store this number at the start of the loop, you can make the 
    loop run much faster:
From six lines above, in array.js:

    if (arr.indexOf) {
        return arr.indexOf(obj, opt_fromIndex);
      }
    if (Array.indexOf) {
        return Array.indexOf(arr, obj, opt_fromIndex);
      }
It only falls through to there if Array already has no indexOf prototype method on it at all. In that case, performance on "for" loop is the least of your concerns, because you're using a terribly slow browser where it won't matter anyway. They used a cached length property later on in the very same file, in places where it might actually matter.

    Google’s developers seem to have figured this trick out
    later on in the same file. From array.js, line 153:
    var l = arr.length;  // must be fixed during loop... see docs
    ...
    for (var i = l - 1; i >= 0; --i) {
    
    This loop is better in that it avoids a property lookup each
    time through the loop, but this particular for loop is so
    simple that it could be further simplified into a while loop,
    which will run much faster again:
    var i = arr.length;
    ...
    while (i--) {
On V8 and SquirrelFish Extreme, these two loops are exactly the same speed (and indeed, probably generate near-identical native code.) On FireFox and older Chrome/WebKit, the average difference on an array of 10,000 elements is something like the difference between 0.068ms and 0.072 ms to iterate (I don't remember the actual figure on my machine, it's negligible.) In V8, Google's own native iterators are implemented in JavaScript using that incrementing 'for' loop.

    But not all of Closure Library’s performance woes are due
    to poorly optimized loops. From dom.js, line 797:
    switch (node.tagName) {
      case goog.dom.TagName.APPLET:
      case goog.dom.TagName.AREA:
      case goog.dom.TagName.BR:
      case goog.dom.TagName.COL:
      case goog.dom.TagName.FRAME:
      case goog.dom.TagName.HR:
      case goog.dom.TagName.IMG:
      case goog.dom.TagName.INPUT:
      case goog.dom.TagName.IFRAME:
      case goog.dom.TagName.ISINDEX:
      case goog.dom.TagName.LINK:
      case goog.dom.TagName.NOFRAMES:
      case goog.dom.TagName.NOSCRIPT:
      case goog.dom.TagName.META:
      case goog.dom.TagName.OBJECT:
      case goog.dom.TagName.PARAM:
      case goog.dom.TagName.SCRIPT:
      case goog.dom.TagName.STYLE:
        return false;
    }
    return true;
    
    This kind of code is actually pretty common in Java, and
    will perform just fine there. In JavaScript, however,
    this switch statement will perform like a dog each and
    every time a developer checks if a particular HTML element
    is allowed to have children.

    Experienced JavaScript developers know that it’s much
    quicker to create an object to encapsulate this logic:

    var takesChildren = {}
    takesChildren[goog.dom.TagName.APPLET] = 1;
    takesChildren[goog.dom.TagName.AREA] = 1;
    ...
    This code can be further bulletproofed against outside
    interference using hasOwnProperty (see below for a full
    explanation of this).
    
    return !takesChildren.hasOwnProperty(node.tagName);
Really? You tested it? Because a 'switch' does not necessarily generate the same code as a dynamically constructed set of object properties. In Google's original example, if none of the cases match, it immediately returns 'true'. In Sitepoint's example, if the property does not match on the highest level, it must crawl the property chain on the object until it eventually reaches 'undefined'. Not to mention you are making two inner function calls and a fully dynamic property lookup (!(), hasOwnProperty, and node.tagName), none of which I believe can be calculated statically.

Actually, I'm going to end this rant early, because I just read more of the article, and it looks like the author just keeps pulling random shit that he remembers reading from "Javascript: The Good Parts" or whatever and vomiting it back up onto his keyboard.




Agreed, very poorly written article. You can't approach optimisation in isolation like that. There's a whole range of factors like readability, situational awareness, concurrency etc... that need to be considered.

Plus this is the first release of what has previously been an internal-only library. It needs a little time to mature. jQuery wasn't perfect of the gate either.


it must crawl the property chain on the object until it eventually reaches 'undefined'

If it were a normal property lookup, yes, but not with `hasOwnProperty' - that's the same as any old hash table lookup. So efficiency here really depends on the implementation, and Google's is probably faster. Regardless, we're really splitting hairs - these optimizations will get a few milliseconds, on average. As much as I am a perfectionist, it's all completely negligible in terms of UX.


I'm talking about the class tracing V8 does, not prototype chains. "Property chain" meaning in V8, it must climb the series of what it calls "hidden classes" in order to find all of the properties. Other engines do various things when you pass a totally dynamic property lookup in. Some slower engines probably use naïve hash tables, I don't know.

Feel free to correct me if I'm wrong on this one, though. I've never really ripped open V8 or any of the other engines to look inside, I just go by what I read from their design docs.


From my reading of the V8 docs, the hidden class climbing is done when properties are added to the class. Each added property causes a transition to the next hidden class. The final hidden class has all the properties as offsets and a failed lookup might chain through prototypes (not sure there) but does not work though the other hidden classes.


it looks like the author just keeps pulling random shit that he remembers reading from "Javascript: The Good Parts" or whatever

In defense of the author, he is just the messenger:

"""“I’ll make you a deal,” I told him. “Send me some examples of this terrible code and I’ll publish it on SitePoint.”"""


It also seems like the author also forgot that code brevity is very important when maintaining such a large code base. Everyone can understand that switch, but the suggested object replacement is not so obvious.


One thing we can't deny: "case META" should come before "case NOFRAMES". :-)




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | Legal | Apply to YC | Contact

Search: