Memory Leak

[Snacko]

I've run FindBugs (http://findbugs.sourceforge.net/) on Forge SVN source code and some intresting things came out
example:

Code: Select all

(name != null) &&
                    name.toLowerCase().endsWith(".jpg") ||
                    name.toLowerCase().endsWith(".jpeg") ||
                    name.toLowerCase().endsWith(".gif") ||
                    name.toLowerCase().endsWith(".png")

Easy fix just use backets () && () 2nd one missing

Using new Integer(int) is guaranteed to always result in a new object whereas Integer.valueOf(int) allows caching of values to be done by the compiler, class library, or JVM. Using of cached values avoids object allocation and the code will be faster.

Values between -128 and 127 are guaranteed to have corresponding cached instances and using valueOf is approximately 3.5 times faster than using constructor. For values outside the constant range the performance of both styles is the same.

I suppose you use mostly small numbers for hand, cards in play, etc. And this would bring some speedup.

Concatenation strings using + in a loop. This brings a lot of grabage on stack and later to gc. Use StringBuilder.

Three's also some null targeting around which if executed most likely will give strange results.

And for the last one, that I really liked (there's way more and not everything is really relevant). It's not your code but from javazoom.jl

Code: Select all

   static public Object deserialize(InputStream in, Class cls)
      throws IOException
   {
      if (cls==null)
         throw new NullPointerException("cls");
      
      Object obj = deserialize(in, cls);
      if (!cls.isInstance(obj))
      {
         throw new InvalidObjectException("type of deserialized instance not of required class.");
      }
      
      return obj;
   }

Pure awesome infinite function recursion
One of the reasons to use static code analysis to check for common and easy to fix errors.

I would urge you to at least check FindBugs (integrates fully with Eclipse/Netbeans and other software).

[DennisBergkamp]

Wow, this is cool, thanks a lot Snacko!
Stuff I didn't know... I also never heard of FindBugs.

EDIT: and at the infinite loop, that's a great piece of code

[DennisBergkamp]

Running FindBugs does yield a lot of: "... defines non-transient non-serializable instance field ... " though. I'll ignore those, but I'll go through and fix a bunch of the other warnings (there's quite a lot of String concatenations in loops).

[Snacko]

Yes those are just important if you serialize the class, otherwise you can ignore them. Not all things reported are bugs, sometimes it's the design choice or just plain laziness as with serializables / equals / hash when you know 100% (sometimes this really leads to bugs as you do those things you thought you would never do).

[mtgrares]

One good question would be, "How much memory is used by the Java Virtual Machine to display 10-20 HQ images?" This number would be a good "base" requirement.

I can give a basic answer for that.

680 x 480 @ 32bpp =~ 1.3 MB per picture

Thanks frwololo. I was wondering about my old computer (256 MB) and HQ pictures but the short answer is no. I like looking at the HQ scans but it doesn't matter when I'm playing the game.

[DennisBergkamp]

Yes those are just important if you serialize the class, otherwise you can ignore them. Not all things reported are bugs, sometimes it's the design choice or just plain laziness as with serializables / equals / hash when you know 100% (sometimes this really leads to bugs as you do those things you thought you would never do).

Ok, yes I thought so.
By the way, I've fixed all of the ones in the "PERFORMANCE" category, and also quite a few from "CORRECTNESS". The performance boost is amazing! At least on my computer, I think the game sped up by more than 40%. I'm a big fan of this FindBugs program now, I also like how it explains why certain pieces of code are bad, and how it sometimes offers a better solution.

[mtgrares]

A ton of classes are marked as serializable in Forge but the class is never written to a file. At the very beginning when I started writing Forge, I implemented undo by writing all of the classes to the hard drive. Only a handful of classes are written to the hard drive like Deck, QuestData and maybe CardList.

[mtgrares]

My guess is that the picture are being stored in a cache and the cached is just getting too big. Forge used to use a cache but I don't know if it still does.

This is just a few guesses about what might help the picture memory leak.

1. After you are done with an object, set the object to null. Technically this just "gives a hint" to the JVM to garbage collect it.

2. If the Java heap is the problem, you can specify a larger heap, see this link. This other link describes how to get the JVM heap size, memory used, and total memory.

Code: Select all

   public class TestMemory
   {   
       public static void main(String [] args)
       {   
           int mb = 1024*1024;
   
           //Getting the runtime reference from system
           Runtime runtime = Runtime.getRuntime();
   
           System.out.println("##### Heap utilization statistics [MB] #####");
   
           //Print used memory
           System.out.println("Used Memory:" + (runtime.totalMemory() - runtime.freeMemory()) / mb);
   
           //Print free memory
           System.out.println("Free Memory:" + runtime.freeMemory() / mb);
   
           //Print total available memory
           System.out.println("Total Memory:" + runtime.totalMemory() / mb);
   
           //Print Maximum available memory
           System.out.println("Max Memory:" + runtime.maxMemory() / mb);
       }
   }

3. And if you are using any of the collections of Google's collection you have to make sure that you implement correct versions of equals() and hashCode() for the object that you are adding. hashCode() is used by Hash, Tree, Set, Map objects like HashMap, TreeSet, HashSet and equals() is used for Lists like ArrayList.

Code: Select all

class IntWrapper
{
  private int n;

  public IntWrapper(int number)
  {
    n = number;   
  }
}

...
ArrayList list = new ArrayList();

IntWrapper a = new IntWrapper(10);
IntWrapper b = new IntWrapper(10);

list.add(a);
if(list.contains(b))  //this will always be false since a.equals(b) is false
  ...

If a subclass doesn't override Object.equals() method, then the super class Object.equals() is always used. All classes are subclasses since every class extends Object whether you know it or not. Link to Object Javadoc which describes equals() and hashCode() is detail.

Code: Select all

// hashCode() example
IntWrapper a = new IntWrapper(10);
IntWrapper b = new IntWrapper(10);
HashSet set = new HashSet();

set.add(a);
if(map.contains(b)) //this will always be false since a.hashCode() is different than b.hashCode()
  ...

A short explanation about Java Collections.

Set means that there are not any duplicate objects. HashSet stores objects according to the object's hashCode() method. TreeSet stores objects used the object's equals() method and according to the Javadoc "guarantees log(n) time cost for the basic operations (add, remove and contains)."

Maybe this post will help somebody. I've gotten very frustrated because of equals() and hashCode(). I wish you good luck DennisBergkamp.

[Snacko]

@2 You get integer memory rounding, unless you really want to cut the remainder of KB, which can be substantial, you should cast mb to float or double

@3 You don't need to wrap ints just use boxed type Integer is has a properly implemented equals() and hashCode(). Same goes for most types shipped with jre ex. String, URL(this one has really bad hashCode() which can lead to performance problems), etc.
Also if you don't override hashCode() the default Object hashCode() will be used, which returns the identity hash code (an arbitrary value assigned to the object by the VM). This means that 2 equal object won't have equal hash codes and I don't even have to say how many bugs can pop up due to this. Same as list and equals example.

[mtgrares]

One time I was using my own class and I was putting it into a HashMap but it wasn't working because I didn't override hashCode(). Nothing like pulling your hair out trying to find an error, arg.

[silly freak]

I think I found the memory leak (the mem dumps helped a lot), it seems to be in forge.ImageUtil, which creates the tapped images, and more importantly in ImageCache, for the normal ones.

these two classes are duplicate in my opinion, and should be merged. i would like to do this, but have no clue how the keys are organized. is it simply the card name?

[DennisBergkamp]

Nice!
Yes, I think it's just card.getName(), except for facedown creatures, in which case it's "Morph" (if (card.isFaceDown()) ).

[silly freak]

okay, i guess it's the same for the mana pool, filename minus .jpg?
so there's three types:

• full size images for the preview panel
• small images for the in-play area
• rotated small images for the in-play area

am i right? are the preview images also here?

[DennisBergkamp]

Yeah, "Mana Pool".
What do you mean by preview? I think the big images on the right side are cached in GuiDisplay3.java (using Google Collections).

[silly freak]

okay, thanks for your help, you made my life much easier. i'm still not 100% sure if I have thought of every case, but i've committed a new class that should be able to handle all our imaging. I couldn't test it yet; I still have to look for all the image-using code to finally use the new class, though.