3

I'm trying to write John Conway's Game of Life in C, but I'm having trouble adding living cells to the board. The function I wrote to handle it is extremely slow.

Thought process: I want to add n living cells to the board randomly, so while cells left to set alive, get a random (x, y) pair, and if it's dead, make it living. That way I can guarantee n cells become alive.

Is my understanding of the problem incorrect, or am I just being inefficient? Why is it so slow, and how can I make it faster?

void add_cells( int board[BOARD_WIDTH][BOARD_HEIGHT], int n )
{
    //  Randomly set n dead cells to live state.
    while ( n )
    {
        int randX = rand() % BOARD_WIDTH;
        int randY = rand() % BOARD_HEIGHT;

        if( board[randX][randY] == 0 )
        {
            board[randX][randY] = 1;
            n--;
        }
    }
}
MaxG
  • 65
  • 3
  • 4
    This is O(n) and should complete in affordable time. How large is n? – Tatsuyuki Ishi Apr 15 '17 at 05:01
  • Also see https://stackoverflow.com/questions/40485/optimizing-conways-game-of-life – Tatsuyuki Ishi Apr 15 '17 at 05:02
  • If let's say 70% of cells are alive, then it means that your code will have to find an other cell 7 times out of 10, which makes unecessary repetitions. What happens if you pop the cell out from a "remaining cells" array when you set it to alive? – V. Brunelle Apr 15 '17 at 05:05
  • It takes several minutes to run once for n = 10000. – MaxG Apr 15 '17 at 05:05
  • I hadn't thought of that, Brunelle, I'll try it. – MaxG Apr 15 '17 at 05:09
  • If you are willing to consume more memory to achieve performance (i.e. accept the trade-off) set up an array containing pairs of indices (e.g. a struct containing two `int` values) of dead cells. Then just pick one of those at random to come alive. This relies on all other code maintaining that array whenever it updates the board (and vice versa). If you have a requirement for another function that kills live cells, set up a similar array containing indices of live cells. – Peter Apr 15 '17 at 06:11
  • 2
    @TatsuyukiIshi - This code is **not** O(n). The complexity of this function can't be defined. – 4386427 Apr 15 '17 at 06:21
  • @4386427 I'm aware this is not exactly O(n), but the collision cost is quite amortized unless the board is so full. – Tatsuyuki Ishi Apr 15 '17 at 06:22
  • Will the board already have alive cells before you start to add n alive cells? Or will the board be completely empty? – Ajay Brahmakshatriya Apr 15 '17 at 06:23
  • Which value do you use for BOARD_WIDTH, BOARD_HEIGHT and n ? Is the board empty when this function is called ? – 4386427 Apr 15 '17 at 06:43
  • That code is broken since if there is no ==0 cells, it then loops forever. You need to rethink the way you tackle this problem. What you wrote has nothing to do with Game Of Life. – Jean-Baptiste Yunès Apr 15 '17 at 07:33
  • **Do not use `rand()`.** Use [this](http://www.pcg-random.org/) or [this](http://xoroshiro.di.unimi.it/xoroshiro128plus.c) instead. – Veedrac Apr 15 '17 at 14:32
  • If you want to randomly select n dead cells, build a collection of dead cells (linear time or this could be kept track of in a related data structure as cells are updated), then shuffle that collection. Take the top n items in the shuffled collection and make those cells alive. – Michael Burr Apr 15 '17 at 21:36
  • 1
    see also http://stackoverflow.com/questions/35065764/select-n-records-at-random-from-a-set-of-n – Kris Vandermotten Apr 15 '17 at 22:29

4 Answers4

2

If let's say 70% of cells are alive, then it means that your program will have to find an other cell 7 times out of 10, which makes unecessary repetitions.

You could pop the selected cell out from a "remaining cells" array when you set it alive, and select your cell randomly in this array. I suggest to use a dynamicaly resizable container so you don't have to manipulate your entire "remaining cells" array each time you pop out a cell. This should help save you more time.

V. Brunelle
  • 845
  • 9
  • 22
0

If your board is contiguous in memory, you don't have to call rand() twice. You can just use rand() % (BOARD_WIDTH * BOARD_HEIGHT).

void add_cells(uint8_t board[BOARD_WIDTH][BOARD_HEIGHT], int n)
{
    std::mt19937 eng;
    uniform_int_distribution<int> dist(0, BOARD_WIDTH * BOARD_HEIGHT - 1);

    while(n)
    {
        int index = dist(eng);

        uint8_t* cell = (uint8_t*)board + index;
        if(*cell == 0)
        {
            *cell = 1;
            --n;
        }
    }
}
CuriousGeorge
  • 6,446
  • 5
  • 35
  • 66
  • 1
    be careful that RAND_MAX might be smaller than `BOARD_WIDTH * BOARD_HEIGHT` – phuclv Apr 15 '17 at 06:17
  • The variable board is contiguous since it is 2D array of integer. – Nguai al Apr 15 '17 at 06:17
  • That is certainly faster. Not equivalent though, the original version finds and sets `n` clear cells. Therefore a constant factor improvement is not much help here, it really needs an improved data structure to track blank cells to randomly set instead of blindly searching for remaining blanks. – doynax Apr 15 '17 at 06:17
  • This code is not doing the same as OPs code – 4386427 Apr 15 '17 at 06:26
  • In order for above code to work, you need to have these lines: int *cell; cell = (int *)board; Inside the while loop, you need to have these lines: if(!cell[index]) cell[index] = 1; – Nguai al Apr 15 '17 at 06:48
  • @doynax, 4386427: The OP didn't ask for equivalent code, he asked for code which sets N random clear cells to 1, which is what my code does. – CuriousGeorge Apr 15 '17 at 15:40
  • @Nguaial My code **does** work. If you compare output with the OP's code, you won't get the same thing because the random cells are being chosen differently. My code, however, does satisfy what the OP is asking for. – CuriousGeorge Apr 15 '17 at 15:43
  • @LưuVĩnhPhúc True. I've swapped out the generator. The code still has limitations, but not as bad. – CuriousGeorge Apr 15 '17 at 16:42
  • @bitwise - int *cell = (int *)board + index; Is index a multiple of four? cell will storing memory address of integer. Since size of integer is 4(most cases), I think index should be a multiple of 4. – Nguai al Apr 15 '17 at 17:32
  • @bitwise: I was referring to your initial implementation which decremented `n` for every iteration and not just where a cell was found. The current one should be equivalent, aside from the much improved random number generator rendering infinite loops due to dependent pairs of coordinates significantly less likely and therefore reducing the running time. In any event, if the remaining cells have any likelihood of being sparse then I still advocate an alternate solution explicitly tracking a randomized list of remaining cells. – doynax Apr 15 '17 at 19:33
  • @doynax Yeah, I realized that was probably what you meant afterward. You're right about sparse empty cells as well - my version performs poorly for that case. At the same time, keeping a list of empties seems to perform better for sparse empty cells, but slows down horribly for large tables. Interesting problem -_- – CuriousGeorge Apr 15 '17 at 20:53
0

There are several issues that might explain some slowness in your problem:

  • Is the board initialized to 0 before calling add_cells()? If the board has random contents, finding dead cells might take an arbitrary long time, or potentially take forever if fewer than n cells are dead.

  • Are you sure the board is correctly defined? The 2D array seems more natural with y being the first dimension and x the second: using int board[BOARD_HEIGHT][BOARD_WIDTH] and swapping the index values for randX and randY.

  • Testing for (n > 0) would protect against an infinite loop if add_cells() is ever called with a negative n.

  • If n is large, finding dead cells can take a long time as shooting at random has a small chance of hitting one.

  • If n is larger than BOARD_WIDTH * BOARD_HEIGHT or if there are fewer than n dead cells, the loop will iterate forever.

If n is large or if the board has only a few dead cells, it would be more efficient to enumerate the dead cells and chose the target cells at random from the dead cells only. The drawback is such a method would be slower if n is small and the board has many dead cells.

The time complexity for n small compared to the number of dead cells is O(n), which is hard to beat and should be very fast on current hardware, but it tends towards O(n * BOARD_WIDTH * BOARD_HEIGHT) if n is large or close to the number of dead cells, which is much less efficient, and the function never finishes if n is greater than the number of dead cells.

  • If the board is known to be empty when add_cells() is called, if n is larger than BOARD_WIDTH * BOARD_HEIGHT / 2, it would be more efficient to set all cells alive and chose n cells to kill.

  • If the board is not necessarily empty, passing this function the number of live cells would help decide which approach is better and if there are at least n dead cells without the need for a lengthy loop to enumerate the dead cells.

chqrlie
  • 98,886
  • 10
  • 89
  • 149
-2

The modulo function is pretty slow, try (float)rand()/RAND_MAX*BOARD_WIDTH + 0.5

You can also use a faster rand, see here

Community
  • 1
  • 1
yar
  • 1,653
  • 9
  • 24
  • no, any decent compiler will [convert division by a constant into multiply by its multiplicative inverse](http://stackoverflow.com/q/41183935/995714) – phuclv Apr 15 '17 at 06:19
  • @Lưu Vĩnh Phúc: I must have a knack for finding the indecent ones then :( – doynax Apr 15 '17 at 06:21
  • gcc, Clang, ICC, CL... all can do that. [See this](https://godbolt.org/g/0F5Agt), there's no div instruction in any compiler – phuclv Apr 15 '17 at 06:27
  • @Lưu Vĩnh Phúc: Alas the IAR 8051 compiler I am presently working with does not I'm afraid. Regrettably many of the compilers I deal with in my day job are of an entirely different level of equality. Typically those targetting embedded architectures for which the basic assumptions of popular portable compiler backends such as GCC/LLVM break down and a custom job was deemed to be required (efficient stack/pointers, von Neumann, etc). – doynax Apr 15 '17 at 11:05
  • @doynax if you're on a microcontroller like 8051 or a processor without FPU then using float like this is even much worse. If the compiler is so stupid then just calculate the inverse multiplicative manually and do it like what normal compilers do – phuclv Apr 17 '17 at 08:59
  • @Lưu Vĩnh Phúc: Sorry, I didn't mean to imply that I was advocating this approach merely to indicate my frustration with the typical assumption that all compilers in common use are "good". If nothing else array index based upon the results of approximate floating-point results always makes me twitch. – doynax Apr 17 '17 at 09:25