Memory leak with an array

Question

I have a very strange memory problem with my Android app. My app use 3 classes that are the following:

public class RGB 
{
    public int R;
    public int G;
    public int B;
}

public class CMYK 
{
    public int C;
    public int M;
    public int Y;
    public int K;
}

public class COLOR 
{
    public String id;
    public CMYK cmyk = new CMYK();
    public RGB rgb = new RGB();

    public COLOR(String id, int c, int m, int y, int k, int r, int g, int b)
    {
        this.id = id;

        this.cmyk.C = c;
        this.cmyk.M = m;
        this.cmyk.Y = y;
        this.cmyk.K = k;

        this.rgb.R = r;
        this.rgb.G = g;
        this.rgb.B = b;
    }
}

then somewere in the code I have to load 2000 colors from a file (file is about 65K lenght and has exactly 2000 records) and is placed in assets folder

public COLOR[] color_list = new COLOR[2000];
...
...
do
{
    s = reader.readLine();
    if (s != null)
    {
        String[] x = s.split(" ");
        COLOR c = new COLOR(x[0], Integer.parseInt(x[1]), Integer.parseInt(x[2]), Integer.parseInt(x[3]), Integer.parseInt(x[4]), Integer.parseInt(x[5]), Integer.parseInt(x[6]), Integer.parseInt(x[7]));
        color_list[j++] = c;
    }

} while (s != null);

after this the app will crash and stop working. If I remove the do..while all is working, so I think my array will be more and more and more then 65K, what I have done wrong? On Android LogCat I have reached the full of HEAP space (26MB) !!!

Best regards GMG

@JarrodSmith : he is not. the declaration of the array allocates an array, not 2000 actual elements. — njzk2, Oct 26 '12 at 14:24
you can use the allocation tracker to make sure who uses what in terms of memory — njzk2, Oct 26 '12 at 14:25
Problem is you're not assigning memory to your array, you need a "new" for each element in your x[] array. Or declare your array so that some heap is allocated. This is wrong: String[] x = s.split(" "); — CocoNess, Oct 26 '12 at 15:11
@TanjaV that is wrong. The split method allocates the array for you. There is nothing wrong with that line of code. — wolfcastle, Oct 26 '12 at 15:25
Do you really need to store all these colors in memory ? Cant you write them to a DB and make them searchable instead ? I don't understand what value the color_list introduces to your program since the colors cannot be looked up anyway. Can you provide some more context ? — Deepak Bala, Oct 26 '12 at 19:58

score 2 · Answer 1 · answered Oct 26 '12 at 14:21

2

Try using an ArrayList instead of a basic array, it makes memory management much easier.

answered Oct 26 '12 at 14:21

Khantahr

8,156
4
37
60

I don't get the point of using a `List` if he doesn't need `List` functionality. If there's a bug with its code, the bug depends on its faults, not on the data structure used for storage. Why do you state an `ArrayList` makes memory leaks less likely? – Raffaele Oct 26 '12 at 14:35
1

You don't have to allocate the space yourself, you can resize it at will. It's also easier to avoid over running the index, there are all kinds of benefits. You can even get a basic array from it for methods that need one, like the SQLiteDatabase ones. There's little reason not to use a List. – Khantahr Oct 26 '12 at 14:42
A `List` could hide a malformed file. If he's allocating an array of **exactly** 2000 elements, and filling it with a file which is supposed to be **exactly** 2000 lines long, using a `List` may hide a malformed file with 1999 lines, and will his app will crash lately (or misbehave at some point). – Raffaele Oct 26 '12 at 14:46
1

You don't need an array to verify that there are exactly 2000 elements. You read it all in, check the size and then do something sensible instead of just blowing up with a RuntimeException, or having to try/catch the RuntimeException. See Effective Java Item #25 - Prefer lists to arrays for a number of good arguments. – wolfcastle Oct 26 '12 at 14:49
Actually, a List would make it easy to detect and gracefully handle a malformed file rather than just exploding. – Khantahr Oct 26 '12 at 14:50
You make too many wrong assumptions. His app expects a file with 2000 lines. If there are 1999 **the app must crash**! No options to recover here: the file is in his assets folder, it's not user-supplied, so an error in that file is like a programming error, and it should fail ASAP (even at compile time if it were possible). Moreover, this is not Java in the traditional sense, you don't have GB of RAM and GHz of CPU: the advantage of using arrays may be negligible, but why *disoptimize* at every cost? Just because Joshua tell you Lists are better? – Raffaele Oct 26 '12 at 14:56
So have it exit with an error at start if there aren't 2000 lines. Using an array makes it easy to screw up elsewhere, and makes it harder to maintain. What if he decides that he wants 2010 lines in his file six months down the road? With a List he just changes the file, with an array it's quite a bit more involved. Optimization is great, but not at the expense of maintainability. Especially when he DOES have GB of ram and GHz of cpu. The benefits of a List outweigh the only drawback. – Khantahr Oct 26 '12 at 15:08
If you too want it to crash, why encapsulate the exception and explicitly check for some condition? Just let it fail. Harder to maintain? Never heard of constants? Not to tell that for iteration it can simply use foreach or `.length`. So you don't get any *real* point, and just hide behind the common *Lists are better*. I like lists, too, but `List` it's not a matter of love: lists allow you to add, remove, resize and so on - if you need these, that'ok. If you don't, why? Especially in this case, that there's a memory leak and you suggest switching the data structure. There's no link – Raffaele Oct 26 '12 at 15:21
I never said Lists were better, I listed merits of Lists and suggested that there's no reason not to use them. You then jumped to the conclusion that they're better. It's a simple fact that you don't have to worry about size with Lists. With an array, you have to initialize it. Whether you use a constant to do that or not, it's an extra step and an extra place to make an error. That's a fact, not an opinion, no matter how trivial it might seem to you. I suggested a switch of data structure because it would fix his memory leak. – Khantahr Oct 26 '12 at 16:44
Doesn't the name **ArrayList** suggest anything to you? What is your opinion that lists prevent memory leaks based upon? You leak memory when you don't release objects: either if it's a plain old array, or an array accessed from a `List` facade, the same rule applies: the memory can't be released if there's someone holding a reference to that object, so your answer is **simply pointless**. You use lists when you need `add()` (arrays are fix-sized, you can't add to an array). Arrays don't make his code uglier, so he shouldn't change his API (and I remark **that wouldn't solve leaks, anyway**) – Raffaele Oct 27 '12 at 10:19

score 2 · Accepted Answer · edited May 23 '17 at 10:34

I don't think that code is responsible of the OutOfMemoryException. Maybe there are other fields you don't mention, but without running the code one can't tell.

However there may be a small leak when you create the ID. Whenever you create a String from an existing one (either substring()-based or methods in the regex package), the returned string keeps an internal reference to the old one: it's just a thin wrapper around the old sequence of characters, just with different start and different length. This means that you'd better create your ID like this

String id = new String(x[0]);

This way you don't keep the whole line in memory just to store a few characters.

However, this is an optimization, since you state that your file is 65KB, so even if you retain it all in-memory, it wouldn't crash your application. Post the whole code so we can run and analyze it.

BTW, you can save an indentation level this way:

String line;
Pattern pattern = Pattern.compile(" "); // Help the GC ;)

while ((line = in.readLine()) != null) {
    String[] data = pattern.split(line);

    // Ugly, but still better than a 8-args constructor
    RGB rgb = new RGB(data, 1, 3);
    CMYK cmyk = new CMYK(data, 4, 4);

    // the best would be a constructor like Color(String[8])
    colors[j++] = new Color(new String(data[0]), rgb, cmyk);
}

I also changed the API a bit (I found this more comfortable)

score 1 · Answer 3 · answered Oct 26 '12 at 14:34

It's hard to tell without the error, but I assume you're getting an IndexOutofBoundExceptions or something. You initialize the array to 2000 elements, but keep reading your file until you reach the end. What if there are 2001 entries in it? Then you'll blow past the end. Or what if there are only 100? Then you've wasted a ton of space.

Like Ralgha said, use an ArrayList, not an array.

For your file parsing, you might want to consider the Scanner class.

Memory leak with an array

3 Answers3