I am trying to solve '15 puzzle', but I get 'OutOfMemoryError' [closed]
Asked Answered
R

1

2

Is there a way that I can optimize this code as to not run out of memory?

import java.util.HashMap;
import java.util.Map;
import java.util.PriorityQueue;
import java.util.Random;
import java.util.Stack;

public class TilePuzzle {

    private final static byte ROWS = 4;
    private final static byte COLUMNS = 4;
    private static String SOLUTION = "123456789ABCDEF0";
    private static byte RADIX = 16;

    private char[][] board = new char[ROWS][COLUMNS];
    private byte x; // Row of the space ('0')
    private byte y; // Column of the space ('0') private String representation;
    private boolean change = false; // Has the board changed after the last call to toString?

    private TilePuzzle() {
        this(SOLUTION);
        int times = 1000;
        Random rnd = new Random();
        while(times-- > 0) {
            try {
                move((byte)rnd.nextInt(4));
            }
            catch(RuntimeException e) {
            }
        }
        this.representation = asString();
    }

    public TilePuzzle(String representation) {
        this.representation = representation;
        final byte SIZE = (byte)SOLUTION.length();
        if (representation.length() != SIZE) {
            throw new IllegalArgumentException("The board must have " + SIZE + "numbers.");
        }

        boolean[] used = new boolean[SIZE];
        byte idx = 0;
        for (byte i = 0; i < ROWS; ++i) {
            for (byte j = 0; j < COLUMNS; ++j) {
                char digit = representation.charAt(idx++);
                byte number = (byte)Character.digit(digit, RADIX);
                if (number < 0 || number >= SIZE) {
                    throw new IllegalArgumentException("The character " + digit + " is not valid.");
                } else if(used[number]) {
                    throw new IllegalArgumentException("The character " + digit + " is repeated.");
                }
                used[number] = true;
                board[i][j] = digit;
                if (digit == '0') {
                    x = i;
                    y = j;
                }
            }
        }
    }

    /**
     * Swap position of the space ('0') with the number that's up to it.
     */
    public void moveUp() {
        try {
            move((byte)(x - 1), y);
        } catch(IllegalArgumentException e) {
            throw new RuntimeException("Move prohibited " + e.getMessage());
        }
    }

    /**
     * Swap position of the space ('0') with the number that's down to it.
     */
    public void moveDown() {
        try {
            move((byte)(x + 1), y);
        } catch(IllegalArgumentException e) {
            throw new RuntimeException("Move prohibited " + e.getMessage());
        }
    }

    /**
     * Swap position of the space ('0') with the number that's left to it.
     */
    public void moveLeft() {
        try {
            move(x, (byte)(y - 1));
        } catch(IllegalArgumentException e) {
            throw new RuntimeException("Move prohibited " + e.getMessage());
        }
    }

    /**
     * Swap position of the space ('0') with the number that's right to it.
     */
    public void moveRight() {
        try {
            move(x, (byte)(y + 1));
        } catch(IllegalArgumentException e) {
            throw new RuntimeException("Move prohibited " + e.getMessage());
        }
    }

    private void move(byte movement) {
        switch(movement) {
        case 0: moveUp(); break;
        case 1: moveRight(); break;
        case 2: moveDown(); break;
        case 3: moveLeft(); break;
        }
    }

    private boolean areValidCoordinates(byte x, byte y) {
        return (x >= 0 && x < ROWS && y >= 0 && y < COLUMNS);
    }

    private void move(byte nx, byte ny) {
        if (!areValidCoordinates(nx, ny)) {
            throw new IllegalArgumentException("(" + nx + ", " + ny + ")");
        }
        board[x][y] = board[nx][ny];
        board[nx][ny] = '0';
        x = nx;
        y = ny;
        change = true;
    }

    public String printableString() {
        StringBuilder sb = new StringBuilder();
        for (byte i = 0; i < ROWS; ++i) {
            for (byte j = 0; j < COLUMNS; ++j) {
                sb.append(board[i][j] + " ");
            }
            sb.append("\r\n");
        }
        return sb.toString();
    }

    private String asString() {
        StringBuilder sb = new StringBuilder();
        for (byte i = 0; i < ROWS; ++i) {
            for (byte j = 0; j < COLUMNS; ++j) {
                sb.append(board[i][j]);
            }
        }
        return sb.toString();
    }

    public String toString() {
        if (change) {
            representation = asString();
        }
        return representation;
    }

    private static byte[] whereShouldItBe(char digit) {
        byte idx = (byte)SOLUTION.indexOf(digit);
        return new byte[] { (byte)(idx / ROWS), (byte)(idx % ROWS) };
    }

    private static byte manhattanDistance(byte x, byte y, byte x2, byte y2) {
        byte dx = (byte)Math.abs(x - x2);
        byte dy = (byte)Math.abs(y - y2);
        return (byte)(dx + dy);
    }

    private byte heuristic() {
        byte total = 0;
        for (byte i = 0; i < ROWS; ++i) {
            for (byte j = 0; j < COLUMNS; ++j) {
                char digit = board[i][j];
                byte[] coordenates = whereShouldItBe(digit);
                byte distance = manhattanDistance(i, j, coordenates[0], coordenates[1]);
                total += distance;
            }
        }
        return total;
    }

    private class Node implements Comparable<Node> {
        private String puzzle;
        private byte moves; // Number of moves from original configuration
        private byte value; // The value of the heuristic for this configuration.
        public Node(String puzzle, byte moves, byte value) {
            this.puzzle = puzzle;
            this.moves = moves;
            this.value = value;
        }
        @Override
        public int compareTo(Node o) {
            return (value + moves) - (o.value + o.moves);
        }
    }

    private void print(Map<String, String> antecessor) {
        Stack toPrint = new Stack();
        toPrint.add(SOLUTION);
        String before = antecessor.get(SOLUTION);
        while (!before.equals("")) {
            toPrint.add(before);
            before = antecessor.get(before);
        }
        while (!toPrint.isEmpty()) {
            System.out.println(new TilePuzzle(toPrint.pop()).printableString());
        }
    }

    private byte solve() {
        if(toString().equals(SOLUTION)) {
            return 0;
        }

        PriorityQueue<Node> toProcess = new PriorityQueue();
        Node initial = new Node(toString(), (byte)0, heuristic());
        toProcess.add(initial);

        Map<String, String> antecessor = new HashMap<String, String>();
        antecessor.put(toString(), "");

        while(!toProcess.isEmpty()) {
            Node actual = toProcess.poll();
            for (byte i = 0; i < 4; ++i) {
                TilePuzzle t = new TilePuzzle(actual.puzzle);
                try {
                    t.move(i);
                } catch(RuntimeException e) {
                    continue;
                }
                if (t.toString().equals(SOLUTION)) {
                    antecessor.put(SOLUTION, actual.puzzle);
                    print(antecessor);
                    return (byte)(actual.moves + 1);
                } else if (!antecessor.containsKey(t.toString())) {
                    byte v = t.heuristic();
                    Node neighbor = new Node(t.toString(), (byte)(actual.moves + 1), v);
                    toProcess.add(neighbor);
                    antecessor.put(t.toString(), actual.puzzle);
                }
            }
        }
        return -1;
    }

    public static void main(String... args) {
        TilePuzzle puzzle = new TilePuzzle();
        System.out.println(puzzle.solve());
    }
}
Repine answered 22/6, 2010 at 16:9 Comment(4)
What are you attempting to do? What do you think the issue is? What have you tried? StackOverflow is more for specific questions. Vague questions with a large code block don't get very good feedback.Pilcher
Yet another "please do my homework for me" question. It's getting to be like the spam of SO now.Howlond
You give us all this code and expect us to improve it for you without even any mention of where or why it runs out of memory?Clintclintock
Nooooooohhhhhh.... don't close it yet... the OP provided more details and I have the answer!!!! :'( :'(Lancey
L
45

The problem

The root cause is the tons of String objects you are creating and storing in the toProcess Queue and the antecessor Map. Why are you doing that?

Look at your algorithm. See if you really need to store >2 million nodes and 5 million strings in each.

The investigation

This was hard to spot because the program is complex. Actually, I didn't even try to understand all of the code. Instead, I used VisualVM – a Java profiler, sampler, and CPU/memory usage monitor.

I launched it:

And took a look at the memory usage. The first thing I noticed was the (obvious) fact that you're creating tons of objects.

This is an screenshot of the app:

As you can see, the amount of memory used is tremendous. In as few as 40 seconds, 2 GB were consumed and the entire heap was filled.

A dead end

I initially thought the problem had something to do with the Node class, because even though it implements Comparable, it doesn't implement equals. So I provided the method:

public boolean equals( Object o ) {
    if( o instanceof Node ) {
        Node other = ( Node ) o;
        return this.value == other.value && this.moves == other.moves;
    }
    return false;
}

But that was not the problem.

The actual problem turned out to be the one stated at the top.

The workaround

As previously stated, the real solution is to rethink your algorithm. Whatever else can be done, in the meantime, will only delay the problem.

But workarounds can be useful. One is to reuse the strings you're generating. You're very intensively using the TilePuzzle.toString() method; this ends up creating duplicate strings quite often.

Since you're generating string permutations, you may create many 12345ABCD strings in matter of seconds. If they are the same string, there is no point in creating millions of instances with the same value.

The String.intern() method allows strings to be reused. The doc says:

Returns a canonical representation for the string object.

A pool of strings, initially empty, is maintained privately by the class String.

When the intern method is invoked, if the pool already contains a string equal to this String object as determined by the equals() method, then the string from the pool is returned. Otherwise, this String object is added to the pool and a reference to this String object is returned.

For a regular application, using String.intern() could be a bad idea because it doesn't let instances be reclaimed by the GC. But in this case, since you're holding the references in your Map and Queue anyway, it makes sense.

So making this change:

public String toString() {
    if (change) {
        representation = asString();
    }
    return representation.intern(); // <-- Use intern
}

Pretty much solves the memory problem.

This is a screenshot after the change:

Now, the heap usage doesn't reach 100 MB even after a couple of minutes.

Extra remarks

Remark #1

You're using an exception to validate if the movement is valid or not, which is okay; but when you catch them, you're just ignoring them:

try {
    t.move(i);
} catch(RuntimeException e) {
    continue;
}

If you're not using them anyway, you can save a lot of computation by not creating the exceptions in the first place. Otherwise you're creating millions of unused exceptions.

Make this change:

if (!areValidCoordinates(nx, ny)) {
    // REMOVE THIS LINE:
    // throw new IllegalArgumentException("(" + nx + ", " + ny + ")");

    // ADD THIS LINE:
    return;
}

And use validation instead:

// REMOVE THESE LINES:
// try {
//     t.move(i);
// } catch(RuntimeException e) {
//     continue;
// }

// ADD THESE LINES:
if(t.isValidMovement(i)){
    t.move(i);
} else {
    continue;
}

Remark #2

You're creating a new Random object for every new TilePuzzle instance. It would be better if you used just one for the whole program. After all, you are only using a single thread.

Remark #3

The workaround solved the heap memory problem, but created another one involving PermGen. I simply increased the PermGen size, like this:

java -Xmx1g -Xms1g -XX:MaxPermSize=1g TilePuzzle

Remark #4

The output was sometimes 49 and sometimes 50. The matrices were printed like:

1 2 3 4 
5 6 7 8 
9 A B C 
D E 0 F 

1 2 3 4 
5 6 7 8 
9 A B C 
D E F 0 

... 50 times

Lancey answered 22/6, 2010 at 16:27 Comment(5)
I'm sorry for not being more specific. I'm trying to solve the 15 puzzle using the Manhattan Distance of all misplaced numbers as the heuristic. With a board of dimensions 3 x 3 it runs OK. But with 4 x 4 it goes out of memory as it puts in the queue a lot of Nodes. What I wanted to ask is if there was a hack, trick or good observation on ways to improve the code, as to not enqueue some nodes, or putting some modifier to some attributes. Thanks.Repine
How do you create a 3x3 board. Every time I try it it says: 9 is not allowedLancey
+1 for the effort you put into this answer ;-)Coast
Grats on the Reversal badge. That thing is super-rare!Horowitz
In Java 7, interned strings are stored in the main Java heap instead of the PermGen space.Dream

© 2022 - 2024 — McMap. All rights reserved.