Bug Tracker

Opened 12 years ago

Closed 12 years ago

#2510 closed enhancement (fixed)

Optimizations for the merge() method

Reported by: diego Owned by:
Priority: major Milestone: 1.2.4
Component: core Version: 1.2.3
Keywords: Cc:
Blocked by: Blocking:

Description

By using the Firebug profiler I realized that the "merge()" function is one of the most often called function during element selection or matching and is the second most consuming function in a slickspeed run (7880 calls 27.6% time jquery 1.2.3.js, linux FF 2.0.12).

These are the reasons for which I thought this method may be improved:

  • IE is checked for each call, even if we are using other browsers
  • the loop is looking up the same array twice instead of just once
  • there is a written (only) duplication of the for loop

Take the last less seriously...but only one executes and cross browser debugging requires two hand written break-points...ha.ha.ha...

Probably this has been exacerbated by my last works ;-)

However even if it looks ugly what it does is simple, a function for each browser is compiled runtime when the absolute first call hit the function, and this is also the most compact way of writing it, well after removing the unnecessary concatenation fat.

    merge: function( first, second ) {
        // We have to loop this way because IE & Opera overwrite the length
        // expando of getElementsByTagName

        // Also, we need to make sure that the correct elements are being returned
        // (IE returns comment nodes in a '*' query)
        this.merge = new Function('first, second',
                'var a, i = -1;'+
                'while (a = second[++i])'+
                    (jQuery.browser.msie ? 'if ( a.nodeType != 8 )' : '')+
                    'first[first.length] = a;'+
                'return first;'
        );
        return this.merge( first, second );
    },

on the thread we can probably look at less eye intrusive version.

Please tell if I should build/attach a real diff once this has been reviewed.

One catch is that after this, merge() shows up as an anonymous function, don't know if jQuery use some trick on fn names, however it worked and now slickspeed numbers are different and looking at the profiler again have...7880 calls 17.8% time.

Well surely need review...gain start to be suspiciously high... did I miss the obvious ?

Diego Perini

Attachments (1)

core-merge.patch (924 bytes) - added by diego 12 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 12 years ago by diego

After pondering and reviewing all the talks held on the "Performance Tweaks" thread, I rewrote the patch to the "merge()" method by using lazy functions. The real gain is obtained by doing the browser check only once instead of every time.

By not using the "new Function" constructor, as John suggested, the code will remain compatible with AIR. This method is called thousand of times in intensive selector queries so I tuned a bit the loop trying to avoid to lookup in the same array several times.

For IE I left the check on nodeType != 8 instead of nodeType == 1 as was suggested by me and Matt Kruse because this part is not really related to the "Performance Tweaks" we were talking.

Diego Perini

Changed 12 years ago by diego

Attachment: core-merge.patch added

comment:2 Changed 12 years ago by rformato

At the very beginning of the merge function I read this comment:

// We have to loop this way because IE & Opera overwrite the length
// expando of getElementsByTagName

Maybe that's the reason to stay with push to add elements to the array and not to use first.length, even if it is not the best choice about performance

comment:3 Changed 12 years ago by mkruse

I would still like to recommend this change instead:

jQuery.merge = function (first,second) { 
  var d; 
  if (document.createElement && document.createComment) { 
    d = document.createElement('div'); 
    d.appendChild(document.createComment("test")); 
    if (d.getElementsByTagName && d.getElementsByTagName('*').length>0) { 
      return (newMerge3=function(first,second) { 
        var i=0,e; 
        while(e=second[i++]) 
          if (e.nodeType!=8) 
            first[first.length]=e; 
        return first; 
      })(first,second); 
    } 
  } 
  return (newMerge3=function(first,second) { 
    var i=0,e; 
    while(e=second[i++]) 
      first[first.length]=e; 
    return first; 
  })(first,second); 
}

It has the advantage of being a speedup and also avoiding the unnecessary browser sniff.

See http://groups.google.com/group/jquery-dev/browse_frm/thread/5019fa537c8d3595/3fb717b0dc43e4f0

comment:4 in reply to:  3 Changed 12 years ago by mkruse

Sorry, I had left the "newMerge3" reference inside the code...

jQuery.merge = function (first,second) {
  var d;
  if (document.createElement && document.createComment) {
    d = document.createElement('div');
    d.appendChild(document.createComment("test"));
    if (d.getElementsByTagName && d.getElementsByTagName('*').length>0) {
      return (jQuery.merge=function(first,second) {
        var i=0,e;
        while(e=second[i++])
          if (e.nodeType!=8)
            first[first.length]=e;
        return first;
      })(first,second);
    }
  }
  return (jQuery.merge=function(first,second) {
    var i=0,e;
    while(e=second[i++])
      first[first.length]=e;
    return first;
  })(first,second);
}

comment:5 in reply to:  2 Changed 12 years ago by rformato

Replying to rformato:

At the very beginning of the merge function I read this comment:

// We have to loop this way because IE & Opera overwrite the length
// expando of getElementsByTagName

Maybe that's the reason to stay with push to add elements to the array and not to use first.length, even if it is not the best choice about performance

After a search on svn I found [1594], so the comment was referring to the second array, not the first. The patch is good.

Also consider storing first.length out of the loop in a local var and incrementing it on array augmentation:

jQuery.merge = jQuery.browser.msie ?

             function( first, second ) {
                 var a, i = -1,fl = first.length;
                 while (a = second[++i])
                     if ( a.nodeType != 8 )
                         first[fl++] = a;
                 return first;
             } :
             function( first, second ) {
                 var a, i = -1,fl = first.length;
                 while (a = second[++i])
                     first[fl++] = a;
                 return first;
             };
return jQuery.merge( first, second ); 


This will speedup merge of another 5% in IE, much less in other browsers but it won't harm in any case

comment:6 Changed 12 years ago by flesler

Resolution: fixed
Status: newclosed

Added the looping optimizations at [5578], the code bifurcation will need to be approved in a general way for the code.

Note: See TracTickets for help on using tickets.