help Optimizing code

ok, so i've been working on this project for a long time now, a falling sand game, on and off. it works almost the way i want it too... 2 problems, one i don't think this community will be able to help me with, the other, the optimization, i have higher hopes for.

the first one, less likely to have a solution is that heat doesn't spread right, at the moment i am averaging two heat values, the current and a neighboring pixel's, to get the new current heat, i believe this is done in the doHeat() method, but my code is sloppy and it has been a while since i did anything too it.

the second, is that it runs slowly, especially with a lot of stuff on the screen. there are a lot of loops, and i cannot think of a way to reduce the loop count, or could accessing arrayLists also be limiting the speed?

i apologize for the large size of this code, and the lack of useful comments. the entirety of the source is attached in a zip (including a main class, an class that adds to the array lists in this class, and a class that contains an editor for the physics of the sand), to find out more about the project my site has a page on it...

If I remember correctly, using switch/case statements yields much faster selection than if-else statements.

I don't think that's the main issue though - it may be the operations done in the for loops or possibly the amount of threads you're using for calculations. I've only briefly glanced at your code while testing it (enjoyably!) =)

Edit: This may sound redundant, but considering a post made by Ezzaral in the past your problem may have something to do with your Thread performing operations vs the Event Dispatching Thread performing operations.

It may be a good idea to (somehow) add the event you need to run to the EDT to be dispatched sometime in the future. It may or may not be in your best interests, so I won't post the way to do it until a professional can support this claim (I'm currently still learning the mechanics of the EDT so I do not want to make this a concrete claim).

If I remember correctly, using switch/case statements yields much faster selection than if-else statements.

I don't think that's the main issue though - it may be the operations done in the for loops or possibly the amount of threads you're using for calculations. I've only briefly glanced at your code while testing it (enjoyably!) =)

Edit: This may sound redundant, but considering a post made by Ezzaral in the past your problem may have something to do with your Thread performing operations vs the Event Dispatching Thread performing operations.

It may be a good idea to (somehow) add the event you need to run to the EDT to be dispatched sometime in the future. It may or may not be in your best interests, so I won't post the way to do it until a professional can support this claim (I'm currently still learning the mechanics of the EDT so I do not want to make this a concrete claim).

first, thanks, and thanks for the reply.
i may overhaul the code into using switch's, but it will likely take a while.

i don't know much about the EDT either, i just know that this whole thing should be running in a completely different thread than the rest of the program.

i really think the answer is in the removal of loops, but as far as i can tell all are essential.

any professionals have any opinion? any amateurs? advice still welcome

Neat program. I don't have a great deal of time to go through it at the moment, but here are some things to consider from the start.

First, you'll want to set up a repeatable test state of the program. In order to get any reliable information on the impact of any changes, you need to get your simulation running the same for each iteration. The easiest way to do that is probably to capture some of the commands you use to add elements to System.out (with their current parameters) and paste these into a test() method that you can call to rebuild the running conditions each time you want to test any changes.

Secondly, you need to capture the execution time for each invocation of computeAndDrawSand() so you have concreate numbers to work from. That is of course the roughest measure you can get. If you have a profiler to use, all the better, so you can drill down through methods calls (Netbeans has one build in and I think Eclipse may as well).

Here is a profiler snapshot from Netbeans after letting your code for several minutes with a lot of elements on the screen:

The bulk of your computation time is occurring in the moveSand() method, with doNeighbor() as the heaviest offender. The pc() method is pulling a lot of time due to a very large number of invocations (it also creates a new Random for each invocation). Are you sure you need the random chance check quite that often?

I think you can cut a good amount of execution time out by minimizing redundant array access. Many of your calcs re-access by index values that you have already captured to a variable. You may also be able to eliminate some neighbor checks that have already been performed or not needed. It's hard to say without going through it all in detail. Also, as already mentioned, you have a lot of if() braches within the tightest loops and anything you can do to eliminate or combine those could definitely help. Many of the operations they perform are similar and differ only by array position, which is often a good clue that you could rearrange them or reduce them to fewer statements with a couple of new variable calculations for the relative positions.

That's all I have time to offer at the moment, but perhaps it might help get you started.

thanks, i will try to eliminate making a new random, and yes, i do need to call that method a lot, because it determines what pixel will change in an interaction. but i can use an instance random variable, again i can make an overhaul to switch statements

ok, i replaced the random creation with an instance variable, i haven't re-profiled it yet, although it didn't make a noticeable frame rate difference, i didn't expect it too... but overall it will probably all adds up though.

changing to switch statements just won't work in an easy way, i use too many range comparisons (rand<3 for example) and not enough equalities (==) to make using switch statements work at all.

i notice that because of the (possibly unnecessary) loops, but any time i try to add the same code to a pre-existing loop, doesn't to the same thing that it does on it's own, and i cannot figure out why, and it is frustrating me, i tried putting it in the draw sand method right after the for statements, but it just doesn't do it right anymore.

ok, i replaced the random creation with an instance variable, i haven't re-profiled it yet, although it didn't make a noticeable frame rate difference, i didn't expect it too... but overall it will probably all adds up though.

Correct. It's not a huge overhead, but given that it is unnecessary and the number of invocations of pc(), it is a worthwhile change to make.

changing to switch statements just won't work in an easy way, i use too many range comparisons (rand<3 for example) and not enough equalities (==) to make using switch statements work at all.

Changing to switch statement is unlikely to reap large improvements anyway, so I wouldn't sweat that. The number of array accesses and calculations per pixel is overwhelmingly the bulk of the workload.

(~line 739)
The compiler may be able to consolidate some of those if it recognizes the invariance, but short of looking at the bytecode I can't say for certain. Those are just a few places where there is redundancy in an inner loop.

Other unnecessary access may be occurring from checks or calculations that have already been performed by a neighboring pixel or not needed at all because they are not near any active elements of the display. Not knowing nor having time to intimately know the code, I can't really say if that is the case or if it can't be dealt with easily. You know the logic of those loops and calculations and your intent far better and I am just pointing out some possibilities.

previous checks by neighbors do get counted and are not repeated (in theory :P ) whether it really helps i don't know.

the first line in every loop set is typically if(!drawData[x][y]){ which surrounds the rest of the loop body, it is set to true whenever something is done to a pixel or a neighboring pixel

then after all of the variable declarations another if statement that surrounds the rest of the body checks to make sure that the element at the current point is not nothing.

i deleted a few of those redundant lines.

i don't know about some stuff, though but without someone getting payed to read through every line of my code i won't get as specific help as may be necessary, but still thanks for your advice, and more is always appreciated