Why is this code so slow?

Question

jsc.tools.road.correctType = function() {
    for(row = jsc.data.selection.startX - 1; row <= jsc.data.selection.endX + 1; row++) {
        for(col = jsc.data.selection.startY - 1; col <= jsc.data.selection.endY + 1; col++) {
            if(jsc.data.cells[row-1][col].type != "road" && jsc.data.cells[row+1][col].type != "road" && jsc.data.cells[row][col].type == "road") {
                jsc.ui.addClassToCell("horz", row, col);
            }
            else {
                jsc.ui.removeClassFromCell("horz", row, col);
            }
            if(jsc.data.cells[row][col-1].type != "road" && jsc.data.cells[row][col+1].type != "road" && jsc.data.cells[row][col].type == "road") {
                jsc.ui.addClassToCell("vert", row, col);
            }
            else {
                jsc.ui.removeClassFromCell("vert", row, col);
            }
        }
    }
};

// Elsewhere
jsc.ui.addClassToCell = function(class, x, y) {
    $("#" + x + "-" + y).addClass(class);
};
jsc.ui.removeClassFromCell = function(class, x, y) {
    $("#" + x + "-" + y).removeClass(class);
};

The code above runs very slowly. I can't figure out why. It's using jQuery 1.3.2. Any way to optimize it a bit?

EDIT: The code is part of a javscript game I am making as a personal project. It's bascially a Simcity clone. This piece of code checks the neighbouring cells for each part of the road, and changes the class (and in turn the background image) to the correct one to make the road images line up right, e.g. horizontal, vertical and junction(no class) road images.

EDIT 2: A few more details to provide some context.

The jsc.data.cells is an 200 x 200 array. Each array element is an object with properties like so (default shown): {type: null, developed: false, powered: false, watered: false, hasTransport: false, wealth: 0, quality: 0} .

It's counterpart is in the UI, which is basically a giant table. (200 x 200 again). Each cell has a number of CSS classes added to it throughout the program to change the background image (e.g. .road to change it to road, .com.developed to make it a developed commercial zone). The table cells each have an id of the form #x-y which is what jsc.ui.addClassToCell, and jsc.ui.removeClassFromCell edit.

EDIT 3: Fixed the IDs starting with numbers. Trying out some of the optimizations now.

Answer 1

I can only give some tips, but don't know if they help much. Have no possibility to test your code.

1-st: declare variables in local function scope. I mean the row and col variables, which you declared as global (missing var statement). Access to global variables takes longer (AFAIK) than to local scope vars.

var row = jsc.data.selection.startX-1;

var col = jsc.data.selection.startY-1;

2-nd: cache references to common objects. Here, you can store reference for jsc.data and/ord jsc.data.selection and jsc.data.cells. IIRC, the access to an object property is linear.

jsc.tools.road.correctType = function() {
   var data = jsc.data, selection = data.selection, cells = jsc.data.cells, ui.jsc.ui;

   for(var row = selection.startX - 1, endX = selection.endX + 1, endY = selection.endY + 1; row <= endX; ++row) {
      for(var col = selection.startY - 1; col <= endY; ++col) {
         if(cells[row-1][col].type != "road" && cells[row+1][col].type != "road" && cells[row][col].type == "road") {
            ui.addClassToCell("horz", row, col);
         } else {
            ui.removeClassFromCell("horz", row, col);
         }
         if(cells[row][col-1].type != "road" && cells[row][col+1].type != "road" && cells[row][col].type == "road") {
            ui.addClassToCell("vert", row, col);
         } else {
            ui.removeClassFromCell("vert", row, col);
         }
      }
   }
};

I also moved the declaration of endY variable to the outer loop, so it won't be computed with every access to inner loop.

-- edit

hope you know, that ID attribute values cannot start with a number, like you have, eg. #2-3

Answer 2

Depending on the size of your selection, you might be doing a whole lot of condition checks and DOM edits.

By commenting out the content of addClassToCell and removeClassFromCell and comparing run times you can find out whether the condition checking or the dom editing takes the most time and thus which one is the best candidate for optimising.

Answer 3

Normally you can significantly optimize loops like these;

for( var x = 0; x < someMethod(); x++ ) {
  //... do stuff
}

By exchanging them out with something like this

var x = someMethod();
while( x-- ) {
  //...do stuff
}

Though it becomes slightly different semantically, it normally works quite well as long as you're not dependent upon order in your looping (order is opposite)

Even when you cannot change the order, you will also significantly improve your code by merely moving the someMethod call OUT of your actual loop, since in many JS implementations it will be called once for every iteration...

Answer 4

A short estimate using O() notation:

for(row) ... O(N)
for(col) ... O(N)
$().addClass/removeClass ... O(N^2)

the $() is even called twice within the nested for.

so you end up with O(N^4)

You can optimize this by caching the calculated classes in the as property of jsc.data.cells[row][col], e.g.

jsc.data.cells[row][col].horz = 1; // don't set class "horz" if not present
jsc.data.cells[row][col].vert = 1;

and use the cached data when you create the cells inside the HTML table, rather than calling $() for each cell.

Answer 5

Use a memoizer or a local cache to store the jQuery objects you have already created. That will reduce the numer of calls of the $ function.

var cache = {}, selector;
for (/* … */) {
    selector = "#" + x + "-" + y;
    if (!cache[selector]) {
        cache[selector] = $(selector);
    }
    // cache[selector] refers to the same object as $("#" + x + "-" + y)
}