ansaurus

Question

Answer 1

+2 A:

Hey Max. I suggest you install Firebug for Firefox if you haven't already. You can then use the Javascript Profiling functionality to find out where exactly in your code the slowdown is.

Here's a good tutorial on profiling with Firebug.

Best of luck.

hobodave 2009-07-28 08:39:05

While that will help with detecting which functions are slow, it wont help explain why.

Justin Johnson 2009-07-28 09:34:35

that's what brains are for.

hobodave 2009-07-28 09:35:23

Justin, you need to know where it is before you can fix it and sometimes just looking at the code, it is not obvious where it is...

Darko Z 2009-07-28 20:38:09

Answer 2

+1 A:

Well just from glancing over your code the biggest mistake you seem to be making is in your second callback (the bottom one) where you keep reselecting the row children elements. You should really assign that common query result to a variable and just use that. I would imagine youd see an improvement in performance then. This is how i'd change it:

/* beginning of callback code */

if (data.mstype == 'success') {

    /* the pressed button */
    btn = $(".start_btn[name=" + id + "]");
    /* the tr where the button is inside */
    tr = $(btn).parent().parent();
    /* remove red */
    tr.removeClass('rot');

    /***** This is the change *****/
    var children = $(tr).children();

    /* set text of Kunden-Nr. td */
    children.eq(3).text(arr_infos[1]);

    /* set text of Sort td */
    children.eq(4).text(arr_infos[2]);

    /* set text of Code td */
    children.eq(5).text(str_codes);

    /* set text of start td */
    children.eq(8).text('ja');
    /* prepend notice */
    $("#outline").prepend("<div id='notice'>" + data.ms + "</div>");
}

/* end of callback code */

Other than that you'd need to be more specific as to WHERE exactly it is that the bad performance is coming from and for that you can use Firebug as others have pointed out.

Darko Z 2009-07-28 08:44:14

While the repeat use of `$(tr)` is going to slow things down, the biggest trouble spots are the two loops. The first one calls the HTML parser into play and restructures the DOM every iteration, and the second one searches the DOM every iteration.

Justin Johnson 2009-07-28 09:33:38

hes not manipulating the dom apart from the append() hes manipulating strings which, while ugly and not the most elegant way is faster than creating nodes from the dom like you suggest

Darko Z 2009-07-28 20:42:11

Appending multiple items to a dom node is not faster than appending them to a fragment. Also, he was doing multiple calls to $("#someid") which is slower than storing them in array when creating them and simply accessing them in the array later.

Justin Johnson 2009-07-28 22:29:04

And his problem is in older browsers, whose performance I cannot test to confirm or deny the difference between using a string or creating nodes.

Justin Johnson 2009-07-28 22:32:00

Answer 3

+5 A:

My suggestions are in the code as comments indicated by //++. Generally, JavaScript is faster when you search or alter the DOM as little as possible.

The main points of concern are in your loops and your repeat usage of $(tr).

I haven't tested this code.

$(function() {
    //++ jQuery doesn't cache this call, so it has to search the DOM each time.  
    //++ I've moved any node that is requested more than once here
    var dialog_fieldset_node = $("#dialog_fieldset"),
     dialog_node          = $("#dialog"),
     kunden_nr_node       = $("#kunden_nr"),
     outline_node         = $("#outline");
    //++ variables that you're using as globals I've moved here so that they can be shared 
    //++ between the event handlers, but aren't poluting the global scope.  They are accessible
    //++ to each event handler via the closure property of this annonymous function.
    var id = null;
    //++ Since you're creating the code <INPUT> nodes, store a refernce to them at that time
    //++ instead of having to find them in the DOM again later.  Now, anzahlCodes doesn't need
    //++ to be available to both handlers.
    var code_input_nodes = [];

    /* when start button is clicked */
    $('.start_btn').click(function() {
     /* remove all previous input fields */
     dialog_fieldset_node.children().remove();

     /* save id */
     id = $(this).attr('name');

     /* get data from db */
     $.post("inc/get_start_order_infos.inc.php", { id: id },
      /* Callback */
      function(data){
       /* make data globally available */
       daten = data;

       /* append infos_html to the fieldset */
       //++ No need to build a string in a variable that you're only going to use once.  You might want 
       //++ to build this out using DOM methods as I did below.  Since its only done once, there might 
       //++ not be a difference performancy wise
       dialog_fieldset_node.append(
        '<label for="kunden_nr">Kunden-Nr.</label><input type="text" name="kunden_nr" id="kunden_nr" ' +
        'value="' + data.kunden_nr + '" class="text ui-widget-content ui-corner-all" />' +
        '<label for="sort">Sort</label><input type="text" name="sort" id="sort" value="' +
        data.sort_nr + '" class="text ui-widget-content ui-corner-all" />'
       );


       //++ 1) `var e` to keep `e` from begin global.  If you want this side effect, you should be explicit about it.
       //++ 2) Create nodes via DOM methods to avoid running the HTML parser. node variables are defined outside of the
       //++ loop to avoid overhead of instantiation and scope-chain modification (minimal, but effective for large sets
       //++ of iterations.
       //++ 3) Append created nodes to a document fragment and then append the fragment to the `dialog_fieldset_node` to 
       //++ avoid multiple, unnecessary DOM tree reflows (which are slow).
       var fragment     = document.createDocumentFragment(),
        label_node   = null, 
        input_node   = null;
        anzahlCodes  = 0;
       //++ Seems this needs to be reset everytime
       code_input_nodes = [];

       /* For each element within the sub array codes */
       for( var e in data.codes){
        label_node = document.createElement("LABEL");
        label_node.setAttribute("for", anzahlCodes);
        label_node.innerHTML = "Code " + anzahlCodes;

        input_node = document.createElement("INPUT");
        input_node.setAttribute("type",  "text");
        input_node.setAttribute("name",  "code" + anzahlCodes);
        input_node.setAttribute("id",    "code" + anzahlCodes);
        input_node.setAttribute("class", "text ui-widget-content ui-corner-all");
        input_node.setAttribute("value", data.codes[e]);

        //++ store a reference for later use
        code_input_nodes.push(input_node);

        /* append code_html to the fieldset */
        fragment.appendChild(label_node);
        fragment.appendChild(input_node);
        anzahlCodes++;
       }
       dialog_fieldset_node.append(fragment);

       dialog_node.dialog('open');
       kunden_nr_node = $("#kunden_nr");
       kunden_nr_node.select();

      }, 
      "json"
     );

     return false;
    });

    dialog_node.dialog({
     bgiframe: false,
     autoOpen: false,
     height:   350,
     modal:    true,
     buttons:  {
      'Auftrag starten': function() {
       /* close dialog */
       $(this).dialog('close');

       /* create the info array to be submitted */
       var arr_infos = [id, kunden_nr_node.attr('value'), $('#sort').attr('value')];

       /* write inputs into the str_codes */
       var str_codes = "";
       for ( var i in code_input_nodes ) {
        str_codes += (i ? "" : "<--->") + code_input_nodes[i].attr('value');
       }


       /* execute start */
       $.post("inc/start_orders.inc.php", { 'corrected_infos[]':arr_infos, 'corrected_codes': str_codes },
        /* Callback */
        function(data){
         $('#notice').remove();

         /* if start was successful */
         if (data.mstype == 'success'){
          /* the tr where the button is inside */
          //++ 1) Was this intentionally global?  Global variables are the slowest to access because they
          //++ are at the end of the scope-chain (which *sometimes* makes a difference, depending on depth).
          //++ 2) Instead of wrapping `tr` in `$()` every time you use it, just do it once.
          var tr = $(
            $(".start_btn[name=" + id + "]").parent().parent()
           );
          //++ Avoid calling `.children()` multiple times, just do it once.
          var tr_children = tr.children();

          /* remove red */
          tr.removeClass('rot');

          /* set text of Kunden-Nr. td */
          tr_children.eq(3).text(arr_infos[1]);

          /* set text of Sort td */
          tr_children.eq(4).text(arr_infos[2]);

          /* set text of Code td */
          tr_children.eq(5).text(str_codes);

          /* set text of start td */
          tr_children.eq(8).text('ja');

          /* prepend notice */
          outline_node.prepend("<div id='notice'>" + data.ms + "</div>");
         }

         /* if not successful */
         else {
          outline_node.prepend("<div id='notice' class='notice_err'>" + data.ms + "</div>");
         }
        },
        "json"
       );
      },

      'Abbrechen': function() {
       $(this).dialog('close');
      }
     }
    });
});

Hope that helped.

Justin Johnson 2009-07-28 09:31:42

wow... astonishing answer... going to read it closely now. thx!

Max 2009-07-28 14:23:06

Hey Justin, I am getting the error: code_input_nodes[i].attr is not a function (line 107)Wonder whether the if statement in that line is correct as well!?Thx for all the comments and that lovely help!

Max 2009-07-28 16:47:15

so you said what I said about the TR and changed simple string manipulation into dom element manipulation... how is that faster?

Darko Z 2009-07-28 20:41:17

@Darko You missed the main point. 1) In the click handler, I'm using a fragment to build out everything separate from the main document and then doing one append into the dom, and 2) the loop the builds `str_codes` no longer searches the DOM at all, just access references stored in an array.

Justin Johnson 2009-07-28 22:36:09

@max sorry, like I said, didn't test. Try changing `code_input_nodes.push(input_node);` to `code_input_nodes.push($(input_node));` to apply the jQuery methods to the nodes before adding them to the array

Justin Johnson 2009-07-28 22:43:09

@Justin: that helped, thx a lot!

Max 2009-07-29 10:00:24

Glad I can help. Lets mark this as answered so we can move on with things ;) Also, how was the speed increase?

Justin Johnson 2009-07-29 19:04:22

Great answer man

Sneakyness 2009-09-22 19:18:46

@Sneakyness thank you sir

Justin Johnson 2009-09-23 08:27:07

Answer 4

A:

note that this dialog_fieldset_node.children().remove(); would be more aptly done via empty(); dialog_fieldset_node.empty();

pixeline 2009-07-28 14:50:05

ansaurus

tags:

views:

answers:

is my jQuery script performing bad?

related questions