views:

83

answers:

4

Is there a way to make the following code faster? It's becoming too slow, when length of array is more than 1000 records, especially in IE6.

  dbusers = data.split(";");
  $("#users").html("");
  for (i = 0; i < dbusers.length; i++) {
     if ($("#username").val() != "") {
        if (dbusers[i].indexOf($("#username").val()) != -1) {
           $("#users").append(dbusers[i] + "<br>");
        }
     } else {
        $("#users").append(dbusers[i] + "<br>");
     }
  }
+5  A: 

Minimize the amount of work you do in the loop. Don't add stuff to the DOM in the loop, create a string.

var dbusers = data.split(";");
var username = $("#username").val();
var userlist = "";
if (username == "") {
    for (i = 0; i < dbusers.length; i++) {
        userlist += dbusers[i] + "<br>";
    }
} else {
    for (i = 0; i < dbusers.length; i++) {
        if (dbusers[i].indexOf(username) != -1) {
            userlist += dbusers[i] + "<br>";
        }
    }   
}   
$("#users").html(userlist);
ghoppe
+1 Lots of small DOM insertions are much slower than one big DOM insertion.
Skilldrick
wow, i'm slow. Shouldn't be watching Superman Returns on telly, it sucks anyway :-)
Andy E
This is not a very good idea - building up a string like that is terribly slow in IE.
Pointy
+1  A: 

IE6 doesn't support querySelector, so lookups can be particularly slow. Keep HTML manipulation within loops to a minimum by reducing the number of appends you do, each one has a regular expression run on it to extract the HTML and convert it to a DOM object. Also work in some micro optimisations where you can, might improve performance a little especially over thousands of iterations.

var usersEl = $("#users"); // reduce lookups to the #users element
var result  = "";          // create a variable for the HTML string
var unameVal = $("#username").val(); // do the username value lookup only once
dbusers = data.split(";");
usersEl.html(""); 

// Store the length of the array in a var in your loop to prevent multiple lookups
for (var i = 0, max = dbusers.length; i < max; i++) { 
  if (unameVal !== "") { 
    if (dbusers[i].indexOf(unameVal) != -1) { 
      result += dbusers[i] + "<br>"; 
    } 
  } else { 
    result += dbusers[i] + "<br>"; 
  }
} 
usersEl.html(result);  // Set the HTML only once, saves multiple regexes
Andy E
IE6 is absolutely terrible about repeated string concatenations. Always build big strings as arrays that you join() together at the end!
Pointy
@Pointy: I didn't realize string concatenation in IE6 was so bad. I guess I learned something new today :-)
Andy E
@Andy glad to help - I encourage you to try it out, because it's really jaw-dropping how slow that browser can be on something so simple!
Pointy
Thanks for trying to help, but unfortunetly this advices didn't help much. It's still awfully slows.. Actualy i can generate this list on the server side, while making ajax-request and get completed html to put it on the page, but the problem is that i want to add filtering functionality on the page, that's why i have to store that array with names in memory.
stee1rat
I know this thread is way done by now, @AndyE, but I notice you're setting the HTML every iteration of your for loop. That makes it even slower than @stee1rat's original. I'm sure it's just a typo.
ghoppe
@ghoppe: oops :-) it was a mistake, thank you for pointing it out - I like to edit mistakes like this even though the thread is old.
Andy E
+2  A: 

Faster than those by far (especially in IE!) is to build your string as an array (yes, really) and then concatenate it at the end:

var dbusers = data.split(";"), username = $('#username').val();
$("#users").html($.map(dbusers, function(_, dbuser) {
  if (username == '' || dbuser.indexOf(username) > 0)
    return dbuser + '<br>';
  return '';
}).get().join(''));

The $.map() routine will build an array from the return values of the function you pass. Here, my function is returning the user string followed by the <br>. The resulting array is then turned into a string by calling the native join() routine. Especially when you've got like 1000 things to work with, this will be much faster than building a string with repeated calls to +=! Try the two versions and compare!

Pointy
Your code is incorrect. The first parameter passed to the callback is the data, not the index; remove your `_, `. Also, `$.map` returns an array, not a jQuery object, if an array is passed, so remove `.get()` too.
strager
+2  A: 

Use a document fragment.

You can perform more optimizations, too, like removing that nasty if and creating the nodes yourself.

var frag = document.createDocumentFragment(),
    dbUsers = data.split(';'),
    dbUsersLength = dbUsers.length,
    curDbUser,
    usernameVal = $('#username').val();

for(i = 0; i < dbUsersLength; ++i) {
    curDbUser = dbUsers[i];

    if(curDbUser.indexOf(usernameVal) !== -1) {
        frag.appendChild(document.createTextNode(curDbUser));
        frag.appendChild(document.createElement('br'));
    }
}

$('#users').empty().append(frag);

I made a tool to benchmark all the current answers: http://dev.liranuna.com/strager/stee1rat.html

ghoppe's and mine seem to be the fastest.

strager