tags:

views:

139

answers:

3

The following function needs at least 3 seconds to run (on 500 table rows). Is it possible to make this function faster?

function prepareTable() {
    var groupIndex = 0;
    $("#row tbody tr").each(function(index) {
    // each row gets a unique id
    // remove default css styles for table rows
    // read out hidden value, that stores if row is a group
    var group = $(this).attr('id', 'node-'+index).removeClass("odd event").find('td :hidden').attr('value');
    // if it is a group, add special styles to row and remember row index
    if (group == 'true') {
        groupIndex = index;
        $(this).addClass('odd').find("td:first")
            .mouseenter(function() {
                $(this).parent().addClass("swGroupLink");
            })
            .mouseleave(function() {
                $(this).parent().removeClass("swGroupLink");
        });
    } else {
        // make all following rows to children of the previous group found
        $(this).addClass('even child-of-node-' + groupIndex);
    }   
    });
}
A: 

i guess the find is the one where all the times goes lost.

can you not make a selector of it in stead of a find. what's the HTML?

Stefanvds
find needs 80ms for all 500 rows
Steven
+6  A: 

I suggest two improvements:

  • Cache DOM References
  • Work at your table offline

Example

function prepareTable() {
   var groupIndex = 0;
   var $mytable = $('#row'),
       $parent  = $mytable.parent();

   $mytable = $mytable.detach();

   $mytable.find('tr').each(function(index) {
      var $this = $(this);
      var group = $this.attr('id', 'node-'+index).removeClass("odd event").find('td :hidden').attr('value');
// if it is a group, add special styles to row and remember row index
   if (group == 'true') {
       groupIndex = index;
       $this.addClass('odd').find("td:first")
           .mouseenter(function() {
               $this.parent().addClass("swGroupLink");
           })
           .mouseleave(function() {
               $this.parent().removeClass("swGroupLink");
       });
   } else {
       // make all following rows to children of the previous group found
       $this.addClass('even child-of-node-' + groupIndex);
   }   
   });

   $parent.append($mytable);
}

I added a variable $this which caches $(this) in your .each() loop. I also added $mytable and $parent. $mytable stores the #row element and $parent stores the parent-node from #row. That is because I remove the whole element from the DOM, do the work and re-attach it to the parent.

Test environment: http://www.jsfiddle.net/2C6fB/4/

If that is still too slow, you have other options here. First, look if you can split the loop into smaller pieces. You can optimize that like a lot by using asychronous callbacks, for instance, setTimeout. That can be a tricky business and I would need to know your code in more detail, but in general you might just want to wrap your whole loop into single setTimeout() functions. Example -> http://www.jsfiddle.net/2C6fB/5/

This ensures that the browser won't "hang" while operating. But of course this took a little bit longer to complete the whole task.

jAndy
Your optimization sound very good for me. Unfortunately the function is now about 20 percent slower :-( (Tested with Firebug and Google Chrome Developer Tools)
Steven
@Steven: That sounds almost impossible. I'm setting up a quick test environment.
jAndy
@jAndy: Thank you very much. I'm also confused...
Steven
@Steven: As I expected, my version is about 50% faster than the old one. See http://www.jsfiddle.net/2C6fB/2/You need to open firebug/chrome console to see the results. Just comment the bottom lines out/in to test the different functions.
jAndy
http://www.jsfiddle.net/2C6fB/4/
jAndy
Weird! Here a screenshot of my results on the real page: http://yfrog.com/mrtimerp
Steven
@Steven: I added another solution in my answer, you'll maybe find that more usefull. But I wouldn't use that as "universal" solution. You still should think over your loop there, where you can split it into sub-problems and optimize it.
jAndy
@JAndy: Yes, I think so too. Thanks for helping...
Steven
+2  A: 

you can take mouseenter and mouseleave outside with live event, so that it will not execute with prepareTable function and you can put it in document ready function.

 $("#row tbody tr td.trueInPrepareTable")
  .live("mouseenter", function(event){      
               $(this).parent().addClass("swGroupLink");    
    }).live("mouseleave", function(event){      
               $(this).parent().removeClass("swGroupLink");    
    });

instead of fetching group value from hidden field, put this value in the rel,rev or title attribute.

    function prepareTableEdit() {
                var groupIndex = 0;
                $("#row tbody tr").each(function(index) {
                     groupIndex = index;
                     $(this).attr('id', 'node-'+ groupIndex ).removeClass("odd even");
                    if($(this).attr('rel') == 'true')
                    {                           
                        $(this).addClass('odd').find("td:first").addClass('trueInPrepareTable');                      }
                    else
                    {
                         $(this).addClass('even child-of-node-' + groupIndex).find("td:first").removeClass('trueInPrepareTable');  
                    }
                });

 }

check out http://www.jsfiddle.net/raBGq/

Ayaz Alavi
nice idea, but almost the same execution time (-100 to -200 ms)
Steven
check out http://www.jsfiddle.net/raBGq/. it took 160 ms to run, much faster as compared to yours. You need to open firebug console for viewing results.
Ayaz Alavi
your new version ignores "child-of-node-<id>"
Steven
see this one http://www.jsfiddle.net/uRyrC/4/
Ayaz Alavi