Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have coded up an implementation of Conway's Game of Life and I have a performance bottleneck in it which I wish to be optimized. The main logic is in the Universe class. I have omitted all code which is not applicable here for brewity:

public class Universe {

    private static final int FLIP_INDEX = 0;
    private static final int FLOP_INDEX = 1;
    private final boolean[][][] universeDoubleBuffer;
    private final int height;
    private final int width;
    private int flipFlopIndex = FLIP_INDEX;

    public Universe(boolean[][] universeState) {
        height = universeState.length;
        width = universeState[0].length;
        universeDoubleBuffer = new boolean[2][height][width];
        for (int y = 0; y < height; y++) {
            for (int x = 0; x < width; x++) {
                universeDoubleBuffer[FLIP_INDEX][y][x] = universeState[y][x];
            }
        }
    }   

    public boolean[][] recalculateUniverseState() {
    int newFlipFlopIndex = (flipFlopIndex == FLIP_INDEX ? FLOP_INDEX : FLIP_INDEX);
    for (int y = 0; y < height; y++) {
        for (int x = 0; x < width; x++) {
            int liveNeighbors = countLiveNeighbors(x, y);
            boolean isLiving = universeDoubleBuffer[flipFlopIndex][y][x];
            if (!isLiving && liveNeighbors == 3) {
                universeDoubleBuffer[newFlipFlopIndex][y][x] = true;
            } else if (isLiving && (liveNeighbors == 2 || liveNeighbors == 3)) {
                universeDoubleBuffer[newFlipFlopIndex][y][x] = true;
            } else {
                universeDoubleBuffer[newFlipFlopIndex][y][x] = false;
            }
        }
    }
    stampPatterns();
    logger.info("Old:" + Arrays.deepToString(universeDoubleBuffer[flipFlopIndex]));
    logger.info("New:" + Arrays.deepToString(universeDoubleBuffer[newFlipFlopIndex]));
    flipFlopIndex = newFlipFlopIndex;
    return universeDoubleBuffer[flipFlopIndex];
}

    private int countLiveNeighbors(int x, int y) {
        int result = 0;
        for (CellNeighbor neighbor : CellNeighbor.values()) {
            try {
                boolean isLiving = universeDoubleBuffer[flipFlopIndex][y + neighbor.getYOffset()][x + neighbor.getXOffset()];
                if (isLiving) {
                    result++;
                }
            } catch (IndexOutOfBoundsException e) {
                logger.info("Cell's neighbor was off the grid.", e);
            }
        }
        return result;
    }    
}

Here is the code for CellNeighbor:

public enum CellNeighbor {

    TOP_LEFT(-1, 1), TOP(0, 1), TOP_RIGHT(1, 1), RIGHT(1, 0), BOTTOM_RIGHT(1, -1), BOTTOM(0, -1), BOTTOM_LEFT(-1, -1), LEFT(-1, 0);

    private int xOffset;
    private int yOffset;

    private CellNeighbor(int xOffset, int yOffset) {
        this.xOffset = xOffset;
        this.yOffset = yOffset;
    }

    public int getXOffset() {
        return xOffset;
    }

    public int getYOffset() {
        return yOffset;
    }
}

My problem is that if I create a world big enough (I tested with 5000 * 5000 for example) life really slows down and the time spent in recalculateUniverseState() reaches almost a second (with an 5000 * 5000 universe the average was 766ms).

I have tried without a double buffer (swapping new boolean[][] arrays) and without the try/catch block but it yielded no significant performance improvements.

My question is that how can I optimize the above code to be faster?

share|improve this question
    
What was the problem with the old title? –  Adam Arold Feb 25 at 1:12
    
A little trick to dodge the array range exceptions and help with performance is to make the array have a border around the "world". You can then safely access cell values "off the edge of the world". You don't update the edge cells, but access them in the countLiveNeightbours call. Alternatively, for Conway's life, you can use a torus; i.e. wrap the index values to stay in range. –  Keith Feb 25 at 3:33
    
Also, that cascaded conditional on mapping number of live neighbours and current state to new state could be done as a table look up. –  Keith Feb 25 at 3:43
3  
A final thought - provided you have a grid that size, a simple algorithm will require 25M updates per frame. So 766ms in Java is actually not that bad. You could get a fair bit with some threading - Java is great for that. But a major speed up could be achieved by not updating large "blank" areas, i.e. dividing into regions via a quad tree or such. The usefulness of this would depend on the current state/ initial conditions. –  Keith Feb 25 at 4:30
4  
About the title edit: the title should summarize the context of your code (what the code is about), and not what you think is wrong with it. This makes it easier to search for questions, it makes the related-questions lists more accurate, etc. The original title was also a very narrow request, but code review is about all aspects of the code, with special attention paid to your requested areas of focus. –  rolfl Feb 25 at 4:43

6 Answers 6

up vote 5 down vote accepted

First of all, this should be obvious, but when you have performance issues in code that involves a tight inner loop, you want to simplify that loop as much as you can. If you can save one cycle in the loop by spending a dozen or a hundred cycles somewhere else, do it, because every cycle in the loop gets multiplied by 5000².

Also, what you really want to minimize in the inner loop are memory accesses. Accessing local variables is fast, since they're typically cached and the JVM knows that their values cannot change unless the code in the method itself changes them. In comparison, pulling a random element from a large array is relatively slow, since it requires a full RAM access, as is, typically, accessing a member variable or calling an uncached method.

I've done something like this before,* so, rather than listing every improvement I might make to your code, let me start by sketching how I might rewrite your core update loop:

// assume these arrays are (height + 2) by (width + 2)
boolean[][] oldBuffer = universeDoubleBuffer[flipFlopIndex],
            newBuffer = universeDoubleBuffer[newFlipFlopIndex];

for (int y = 1; y <= height; y++) {
    int environment
        = (oldBuffer[y-1][0] ? 32 : 0) + (oldBuffer[y-1][1] ?  4 : 0)
        + (oldBuffer[y  ][0] ? 16 : 0) + (oldBuffer[y  ][1] ?  2 : 0)
        + (oldBuffer[y+1][0] ?  8 : 0) + (oldBuffer[y+1][1] ?  1 : 0);

    for (int x = 1; x <= width; x++) {
        environment = ((environment % 64) * 8)
            + (oldBuffer[y-1][x+1] ? 4 : 0)
            + (oldBuffer[y  ][x+1] ? 2 : 0)
            + (oldBuffer[y+1][x+1] ? 1 : 0);

        newBuffer[y][x] = lookupTable[ environment ];
    }
}

A few things to note about this code:

  • There is no countLiveNeightbors() method; in fact, the code does not explicitly count live neighbors at all. Instead, the pattern of the nine cells including and surrounding the cell at (x, y) is maintained in the variable environment, which is used as an index to a 512-element lookup table.

    For example, if the surroundings of the current cell look like this (# = live cell, _ = dead cell):

    # # _
    _ # #
    # _ _
    

    then the value of the environment variable will be:

    (1 * 256) + (1 * 32) + (0 * 4) +
    (0 * 128) + (1 * 16) + (1 * 2) +
    (1 *  64) + (0 *  8) + (0 * 1) = 370
    

    Of course, you'll have to set up this lookup table before using it, but that is something you can do outside the core loop (e.g. in your constructor), so it's not performance-critical.

    (In fact, while I haven't benchmarked this, I'd at least consider making lookupTable a local variable and rebuilding it every time the update method is called, as the extra cost of rebuilding the table might be balanced out by extra optimization opportunities available to the compiler / JVM if it knows that no other code can modify the table. You may want to test it both ways and see which one is faster.)

    One additional advantage of such a lookup table based implementation is that, by changing the lookup table, it can simulate any two-state cellular automaton using the Moore neighborhood, not just Conway's Game of Life specifically.

  • By reusing the parts of the environment pattern that are shared between adjacent cells, the code only needs to do three reads from the oldBuffer array per cell, as opposed to nine in your version. Uncached array access is expensive, so this is likely to provide a significant speedup. (Also, like your code, mine also makes sure to access the buffers as close to sequentially as possible, iterating first by rows and then by columns. This is also important for CPU cache locality.)

  • The code above doesn't update the cells at the edges of the array, which means it doesn't have to worry about (literal) edge cases such as array indices being out of bounds. (Hopefully, the compiler / JVM may notice this too, and may omit some of its internal array bounds checks.)

    If you want your grid to be surrounded by dead cells, you can just initially mark these border cells as dead and leave them untouched in your update method. Alternatively, if, say, you want the grid to wrap around, you can update the edge cells in a separate loop (which can be less efficient, since it only runs for a tiny fraction of all the cells).

Actually, there are a few ways in which the code I suggested above could be optimized further. For example, an obvious optimization is to get rid of the two-dimensional array accesses in the inner loop, since they require two array lookups each.

There are (at least) two ways you could do this:

  • a) In the outer loop, save the previous, current, and next rows of oldBuffer in local variables, like this:

    boolean[] prevRow = oldBuffer[y-1],
              currRow = oldBuffer[y],
              nextRow = oldBuffer[y+1];
    
  • b) Make the buffers themselves one-dimensional arrays, and adjust the indexing so that, instead of buffer[y][x], you use buffer[ y * (width+2) + x ]. You can precalculate the offset y * (width+2), and possibly also (y ± 1) * (width+2), in the outer loop to save a bit of arithmetic, or you could rely on the compiler / JVM to do this for you. Again, you could try it both ways and see if there's any difference.

Also, for cellular automata like the Game of Life, where only a small fraction of the cells typically change in each time step, there are much faster algorithms available than even the generic table lookup method described above.

  • As a first step, ChrisW's suggestion of caching the live neighbor count is likely to be faster whenever the environment of each cell changes relatively infrequently.

  • An even greater speedup may be obtained by storing a list of active cells, i.e. cells that changed or had at least one of their neighbors change state during the previous update step, and only iterating through those cells on the next update. (Since the total number of active cells is bounded above by the size of the grid, you can use a simple array as a circular buffer to efficiently store this list.)

    Effectively combining active cell lists with double buffering can be somewhat tricky. An alternative solution is to use a single buffer, but divide your update method into two phases:

    1. In the first phase, you iterate through the list, calculate the new state for each active cell, and store it in the list.
    2. In the second phase, you iterate through the list again and update the grid buffer to match the new states calculated in the first phase.

    (That said, using both activity lists and double buffering does have one advantage: it allows you to treat cells that oscillate with a period of two, which are quite common in the Game of Life, as inactive. This does require you to maintain separate active cell lists for each buffer.)

  • Finally, if you want a really fast algorithm for simulating Conway's Game of Life, look up Hashlife. It's literally orders of magnitude faster than any "naïve" simulation algorithm, especially for sparse and highly repetitive patterns like many constructed ones.

*) Please don't take that code as an example of good coding style. It is pretty fast, though.


Addendum: Here's a basic implementation of the activity list method, using a single buffer, and with the Conway's Game of Life rules hardcoded.

It uses a byte-packed cell state buffer, where the lowest bit of the byte indicates whether the cell is currently in the activity list, the second bit stores the actual state of the cell, and the following (four) bits store the number of surrounding live cells (to avoid having to recalculate it whenever the cell is examined):

// assume that buffer is a (height) by (width) byte array, and that
// xQueue and yQueue are (height * width) int arrays, of which the
// first (activeCells) elements contain the current active cell coords

// loop over active cells, and find those whose state will change
int changedCells = 0;
for (int i = 0; i < activeCells; i++) {
    int x = xQueue[i], y = yQueue[i];
    byte packedState = buffer[y][x];  // = 4*neighbors + 2*state + active
    boolean wasAlive = (oldState & 2 == 2);

    // quickly check if cell will live under the Game of Life rules
    // (10-11 = alive, 2 neighbors; 12-13 = dead, 3 neighbors; 14-15 = alive, 3 neighbors)
    boolean isAlive = (10 <= packedState && packedState <= 15);

    // add cell to queue if it died or was born
    // we know that changedCells <= i, so it's safe to reuse the same queue space
    if (wasAlive != isAlive) {
        xQueue[changedCells] = x;
        yQueue[changedCells] = y;
        changedCells++;
        // assume active bit is already 1, so no need to change
    }
    else {
        buffer[y][x] = packedState - (byte)1;  // unset active bit
    }
}

// carry out deferred state updates
activeCells = changedCells;
for (int i = 0; i < changedCells; i++) {
    int x = xQueue[i], y = yQueue[i];
    boolean wasAlive = (buffer[y][x] & 2 == 2);
    byte delta = (byte)(wasAlive ? -4 : +4);  // change in neighbor count * 4

    // update neighbor counts of adjacent cells, and mark them as active
    int yMin = (y > 0 ? y-1 : 0), yMax = (y < height-1 ? y+1 : height-1);
    int xMin = (x > 0 ? x-1 : 0), xMax = (x < width-1 ? x+1 : width-1);
    for (int ny = yMin; ny <= yMax; ny++) {
        for (int nx = xMin; nx <= xMax; nx++) {
            byte newState = buffer[ny][nx] + delta;
            // if this cell is not yet active, add it to the queue for next time step
            if (newState & 1 == 0) {
                xQueue[activeCells] = nx;
                yQueue[activeCells] = ny;
                activeCells++;
                newState += (byte)1;
            }
            buffer[ny][nx] = newState;
        }
    }
    // the middle cell only needs to adjusted by +/-2
    buffer[y][x] -= (byte)(delta / 2);
}

Before the first pass, all cells should be initialized to have the correct neighbor counts, have the active bit set and be added to the active cell queue. I've omitted this part of the code, since it's not performance-critical.

Note that this method does not use any padding cells along the edges of the state array. Alternatively, we could add the padding and permanently set the active bit for those cells, but not include them in the queue, allowing the min/max coords calculations to be simplified. I suspect this would not make much difference, but there's no way to be sure without trying it.

share|improve this answer
    
Thanks for the thorough explanation! For some reason using a bitvector(like) structure did not came into mind. –  Adam Arold Feb 26 at 19:48
    
Can you please explain the environment variable's calculation? I don't understand how you calculate its initial value without knowing about x. The ((environment % 64) * 8) part is not clear either. –  Adam Arold Feb 26 at 20:20
    
((environment % 64) * 8) strips away all but the lowest six bits of the environment pattern, and then shifts the remaining bits left by three. You could equally well write it with bit operators as ((environment & 0b111111) << 3). The initial value is, in effect, the environment for x = 0, consisting of the states of the six cells in rows y-1, y and y+1 and columns 0 and 1. It doesn't include the (non-existent) column -1, since the bits that would encode those cell states will just get shifted out of the environment in the inner loop anyway. –  Ilmari Karonen Feb 26 at 20:25
1  
And I can create the lookup by simply converting the numbers from 0 to 511 to a binary arrays and applying the game of life rules to them. –  Adam Arold Feb 26 at 21:00
1  
You definitely need more upvotes. The preliminary implementation resulted in a 10x speedup! –  Adam Arold Feb 26 at 21:03

Not about performance, just some quick generic notes:

  1. try {
        boolean isLiving = universeDoubleBuffer[flipFlopIndex][y + neighbor.getYOffset()][x
                + neighbor.getXOffset()];
        if (isLiving) {
            result++;
        }
    } catch (final IndexOutOfBoundsException e) {
        logger.info("Cell's neighbor was off the grid.", e);
    }
    

    You shouldn't use exceptions for normal cases. (See Effective Java, Second Edition, Item 57: Use exceptions only for exceptional conditions)

  2. The isLiving explanatory variable is great here.

  3. I think using two separate buffers (inputBuffer, outputBuffer) would be readable than the flip-flop.

  4. Instead of 2, 3 use constants, they're magic numbers.

  5. I don't see why is the following comment here, since there's not any sign of that this class is used by multiple threads:

    flipFlopIndex = newFlipFlopIndex; // assignment is atomic, no
                                      // synchronization needed
    

    Anyway, assigments might be atomic but you might need proper synchronization.

    [...] synchronization has no effect unless both read and write operations are synchronized.

    From Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data.

    Locking is not just about mutual exclusion; it is also about memory visibility. To ensure that all threads see the most up-to-date values of shared mutable variables, the reading and writing threads must synchronize on a common lock.

    From Java Concurrency in Practice, 3.1.3. Locking and Visibility.

share|improve this answer
1  
The fact is that I can't name them inputBuffer, outputBuffer because their roles get swapped with each new generation. Others mentioned the try block's logic fault so I'll remove it. The comment there is a leftover, but thanks for pointing it out. This game of life will be used from multiple threads (in a web application) but the recalculateUniverseState() will only be called from one thread. There is an option in the code which is not shown for stamping game of life patterns onto the universe but I solved that problem with a concurrent queue. (code omitted). –  Adam Arold Feb 25 at 10:22

That countLiveNeighbors function looks expensive; are you sure for (CellNeighbor neighbor : CellNeighbor.values()) isn't doing a whole bunch of new CellNeighbor calls (and garbage-collection) behind the scenes?

Even more importantly: All that exception-handling is expensive!

Use exception-handling for exceptional control flow. Don't use it for the usual case.

I suggest rewriting just that portion of the code as follows, and then profiling to see if the bottleneck is gone.

However, writing that just reminded me: Have you tried profiling your code? (Random Google link. Someone in the comments may have better advice about how to profile Java code.)

private int countLiveNeighbors(int x, int y) {
    int result = 0;
    for (int dx = -1; dx <= 1; ++dx) {
        int x2 = x + dx;
        if (x2 < 0 || width <= x) continue;
        for (int dy = -1; dy <= 1; ++dy) {
            int y2 = y + dy;
            if (y2 < 0 || height <= y) continue;
            boolean isLiving = universeDoubleBuffer[flipFlopIndex][y2][x2];
            if (isLiving) {
                result++;
            }
        }
    }
    return result;
}
share|improve this answer
2  
Hmm, you've recently been active. You should come join us in the chat room sometime :) –  syb0rg Feb 25 at 1:59
    
I tested the running times without the try block but it did not yield significant improvement. I will check CellNeighbor, thanks for the idea! –  Adam Arold Feb 25 at 10:17
    
As CellNeighbor is an enum, I'm quite sure that it doesn't do any new CellNeighbor. Iterating on ints only will probably help a bit, but I doubt that it will be significant. –  Simon André Forsberg Feb 25 at 11:36
2  
Calling values on an enum will create a new array each time. So better to save it locally somewhere once. –  Radiodef Feb 25 at 18:02

One way to improve performance is to use a different data structure. For example, you can encode the state of a cell's neighbours using 8 bits (one bit for each neighbour).

You can have a look-up table with 256 elements, which decides what to do for any combination of neighbour-bits: that's a single table look-up.

If a cell-state changes (which happens very rarely) update the corresponding bits in its neighbours' neighbour-bits.

Your universe is something like:

boolean state;
byte neighbours;

Your recalculateUniverseState method is something like:

foreach (cell in universe)
{
    boolean newState = (state) ? liveState[neighbours] : deadState[neighbours];
    if (newState != state)
        changedCells.add(cell);
}
foreach (cell in changedCells)
{
    // change the state of this cell
    // update the bits in this cell's neighbours
}

This is just an example from memory; there's a more efficient way to do it: Abrash's Zen of Code Optimization describes an structure which encode the cell's state and its neighbour's states in 8 bits (relying on the fact that at least one bit is redundent because it's encoded in a next-door neighbour).

There are algorithmic speedups suggested on Wikipedia and probably on this site too: for example, remember which cells or areas of the board aren't changing and don't recalculate those.

share|improve this answer
1  
I made this 'community wiki' because it's such a generic answer applicable to any question about improving game-of-life questions that it's not really a code review. –  ChrisW Feb 25 at 9:03
  1. instead of universeDoubleBuffer[someIndex] use 2 separate fields and 2 separate local variables, swapping values of local variables each step of the loop. Thus one expensive array access operation is excluded.

  2. Conditional and unconditional branches should be avoided for fast execution. In countLiveNeighbors, explicitly read 4 neighbour cells instead of loop.

  3. To represent liveness, use byte 0 or 1 instead of boolean. Instead of if (isLiving && liveNeighbors ... use predefined table for values of next states depending on previous state, and just read new state from table instead of using conditional statements.

share|improve this answer
    
Do you think that a byte[][] will be faster than a boolean[][]? –  Adam Arold Feb 26 at 21:14
    
I mean that in countLiveNeighbors, if (isLiving){result++;} can be replaced with result+=isLiving, which is faster. –  Alexei Kaigorodov Feb 27 at 2:22
    
I've already implemented @Ilmari Karonen`s suggestions which does away with this problem. –  Adam Arold Feb 27 at 10:25

[A bit of topic, as it's not really helping correct "the code above"... but to get at the source of the problem: fast Game of Life generations]

I'm surprised no-one mentionned Bill Gosper yet (if you google "Bill Gosper life" you'll see some conferences he made on the subject). Here is a link that you will probably find interresting :

http://en.wikipedia.org/wiki/Hashlife ( Bill Gosper's Hashlife algorithm )

Optimizing a loop is ok, but first of all should be optimizing the way to look at the problem, and the ways to solve it.

Hashlife is probably quite a good starting point for Conway's Game of Life:

An example on the wikipedia page for hashlife talks about "The 6,366,548,773,467,669,985,195,496,000 (6 octillionth) generation of a very complicated Game of Life pattern computed in less than 30 seconds on an Intel Core Duo 2GHz CPU using hashlife in Golly. Computed by detecting a repeating cycle in the pattern, and skipping ahead to any requested generation."

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.