- Joined
- Jun 5, 2010
- Messages
- 567
- Reaction score
- 598
A few memory leaks and why they are memory leaks and a few other memory optimizations.
Introduction:
Why Use This:
What This Will Cover
Map Saving Leak
Timers Issues (ScheduledFuture) and References:
Scripting Engine:
SpawnPoint Leak:
Storing Too Much Information:
Other things that can improve performance:
Introduction:
I learned most of these from Simon from LocalMS so also be sure to thank him if you like this guide. Secondly, these are mostly fixed in LocalMS source. A common question people ask me when they ask me about memory leaks is how much memory I use. Well we use like < 400 MB after 24 hours now since we have about 30-50 online. When we had 100+ online a couple of months ago, it would go up to around 500 MB after a day. After a week, it does go up to about a 1000 MB used for 10 channels. Remember when people said that no one would ever last a week? Well it's not that hard to and I haven't coded with performance in mind for a while.
Why Use This:
If you are tired of servers crashing every 12 hours and don't know what to do, it is possible that you have memory problems. Fixing these cannot hurt you and can only increase up-time. This is a better solution than raising Xmx for allocated heap size.
What This Will Cover
This will only cover a few big memory leaks and obviously not everything will be stated straight out. However, it will say why it is a memory leak, what it was supposed to do, and how to fix the memory leak. With what is covered in this guide, you are supposed to apply what you learned to fix other leaks.
Map Saving Leak
Memory Leak: Introduced in OdinTeh, this is a pretty big memory leak and can even waste some CPU in grabbing mob skill data, pet skill data every single time. The current code saves the Object into a Map of the form Map<Pair, V> or something of the sort with Pair as the key and something else as the value.
What it is supposed to do: All factory storage in OdinMS coded by Leifde work like this: It is supposed to return an object from the Map when it is needed when a get method is called. If it is null, the program will proceed and read from the XML files. This is a good method and is suggested to do with most caches.
What it actually does: The methods containsKey and get both use some sort of hashing and the problem is just like using equals on objects without an equals method, it will never return true. I will give an example.
This will be false. It is because without an overridden equals method, it would go to the super class and use the equals method defined in Object. The equals method in Object compares the location where it is stored in memory. The two As being compared here are stored in different memory locations and therefore will return false.
What will happen is it will always store into the Map and after a long period of time, the storage will become abnormally large causing a lot of memory to be wasted.
How to fix: There are three ways to do this.
The easiest way would be to implement an equals method in Pair. This would fix it for all the other problems as well. Of course this is extra code. I personally do not use this method, but it would be the best.
Add somewhere in Pair (this is not tested):
The second way to fix it would be to convert the whole thing into a String. Instead of saving two Integers in the Pair, you can turn it into a String. So instead of doing new Pair<Integer, Integer>(arg1, arg2), change the storage from Map<Pair, V> to Map<String, V> (where V is the second object). Change containsKey to check for String.valueOf(int1 + " " + int2) and then save it into Map<String, V>. Here is an example:
The third way would be to change Pair into an object with two values and then adding an equal method in the new class. So instead of MobSkill's two integers, you would make something like
Bottom line: don't do it this way but it might be more clear than the second way.
[/b]
What it is supposed to do: All factory storage in OdinMS coded by Leifde work like this: It is supposed to return an object from the Map when it is needed when a get method is called. If it is null, the program will proceed and read from the XML files. This is a good method and is suggested to do with most caches.
What it actually does: The methods containsKey and get both use some sort of hashing and the problem is just like using equals on objects without an equals method, it will never return true. I will give an example.
Code:
class A {
public static void main(String[] a) {
System.out.println((new A()).equals(new A());
}
}This will be false. It is because without an overridden equals method, it would go to the super class and use the equals method defined in Object. The equals method in Object compares the location where it is stored in memory. The two As being compared here are stored in different memory locations and therefore will return false.
What will happen is it will always store into the Map and after a long period of time, the storage will become abnormally large causing a lot of memory to be wasted.
How to fix: There are three ways to do this.
The easiest way would be to implement an equals method in Pair. This would fix it for all the other problems as well. Of course this is extra code. I personally do not use this method, but it would be the best.
Add somewhere in Pair (this is not tested):
Code:
public boolean equals(Object o) {
if (this.getClass() != o.getClass()) return false;
if (this == (Pair) o) return true;
Pair compare = (Pair) o;
return compare.getLeft() == this.getLeft() && compare.getRight() == this.getRight();
}The second way to fix it would be to convert the whole thing into a String. Instead of saving two Integers in the Pair, you can turn it into a String. So instead of doing new Pair<Integer, Integer>(arg1, arg2), change the storage from Map<Pair, V> to Map<String, V> (where V is the second object). Change containsKey to check for String.valueOf(int1 + " " + int2) and then save it into Map<String, V>. Here is an example:
You must be registered to see links
The third way would be to change Pair into an object with two values and then adding an equal method in the new class. So instead of MobSkill's two integers, you would make something like
Code:
class MobPair {
private int x, y;
public MobPair(int x, int y) {this.x = x; this.y = y;}
public boolean equals(Object o) {/*this is left as an exercise to the reader*/ return false;}
}Bottom line: don't do it this way but it might be more clear than the second way.
[/b]
Timers Issues (ScheduledFuture) and References:
Memory Leak: This has been an issue since beholders and other timers were coded such as berserk and other skills. The issue is that sometimes the object never really goes away. I noticed this when I was analyzing a heap dump (though I heard of this before I did from Simon) and found ScheduledFuture objects that should have been long gone still around. Using this saved me a ton of memory.
What is supposed to happen: When the character is disconnected, after a while garbage collection is supposed to dispose of the object. The memory would be reused for other things.
What actually happens: Because of hard references to the MapleCharacter object (these are basically normal references to another object), these objects are never really removed as garbage collection really only removes when there are no longer hard references. The issue here is that there are hard references both ways so garbage collecting won't work well. From this, this will slowly but surely take up a ton of memory. When programming, the programmer should be very careful with ScheduledFutures.
How to fix: Add a clear method for when the client disconnects. Cancel every schedule and then set them to null.
Example:
MapleCharacter:
MapleClient:
Apply this to every schedule. Do not forget to cancel the mount schedule. Removing the references to client and player will also fix another memory leak while you're at it.
What is supposed to happen: When the character is disconnected, after a while garbage collection is supposed to dispose of the object. The memory would be reused for other things.
What actually happens: Because of hard references to the MapleCharacter object (these are basically normal references to another object), these objects are never really removed as garbage collection really only removes when there are no longer hard references. The issue here is that there are hard references both ways so garbage collecting won't work well. From this, this will slowly but surely take up a ton of memory. When programming, the programmer should be very careful with ScheduledFutures.
How to fix: Add a clear method for when the client disconnects. Cancel every schedule and then set them to null.
Example:
MapleCharacter:
Code:
public void clear() {
client = null; // removes this hard reference
if (beholderSchedule != null) { beholderSchedule.cancel(true); }
beholderSchedule = null;
}MapleClient:
Code:
public void clear() {
if (player != null)
player.clear(); // clears schedules and stuff
player = null; // removes hard reference here
}Apply this to every schedule. Do not forget to cancel the mount schedule. Removing the references to client and player will also fix another memory leak while you're at it.
Scripting Engine:
Memory Leak: This is a leak I found myself while analyzing heap dumps. The scripting engine goes up really high after a while for every MapleClient and eats up memory. This leak is less significant than the previous two, but it should still be fixed as it did decrease usage by around 100 MB with 400 online after a long period of time.
What is supposed to happen: Like the timers, these should go away after the client is gone. However, these don't. I never really bothered to find out why this happens (never looked at the variables inside of the classes), but I'm pretty certain it is the same cause as the timers. There is another possible problem which I will not propose a solution here and it is that some scripts fail to be removed.
What actually happens: Things don't go away, memory leak happens after a while. The MapleClient object starts to get pretty big with the biggest portion being the area saved for scripts. Clearing them and then setting it to null when the user disconnects will keep it low. Setting it to null marks it for able to be garbage collected right away.
How to fix:
In MapleClient clear, add the following:
Be sure to make sure that the scripts are removed when they are done. Also make sure all of your scripts properly dispose and the scripts are removed from the list when you are done.
What is supposed to happen: Like the timers, these should go away after the client is gone. However, these don't. I never really bothered to find out why this happens (never looked at the variables inside of the classes), but I'm pretty certain it is the same cause as the timers. There is another possible problem which I will not propose a solution here and it is that some scripts fail to be removed.
What actually happens: Things don't go away, memory leak happens after a while. The MapleClient object starts to get pretty big with the biggest portion being the area saved for scripts. Clearing them and then setting it to null when the user disconnects will keep it low. Setting it to null marks it for able to be garbage collected right away.
How to fix:
In MapleClient clear, add the following:
Code:
this.engines.clear();
this.engines = null;Be sure to make sure that the scripts are removed when they are done. Also make sure all of your scripts properly dispose and the scripts are removed from the list when you are done.
SpawnPoint Leak:
Memory Leak: This is not actually fully fixed in LocalMS source but it is partially done. So in OdinMS in every SpawnPoint object there is a MapleMonster reference. Most of the methods in MapleMonster aren't even used. In fact the only one that is used is the id. So why save extra information? This can be found in every source (except LocalMS) since it was added in OdinMS days.
What is supposed to happen: Well this is just an optimization. The code itself works completely fine.
What actually happens: By saving an int instead of saving MapleMonster, you free up some 300 bytes per object in every SpawnPoint object. This can add up to a lot since in many maps there are many spawn points. If you save 300 bytes per spawn point, you will end up saving a ton.
How to fix:
Change private MapleMonster monster; to private final int monster;
This is not all. Your spawn points are now broken and monsters will no longer have footholds. This is why monsters fall of platforms in LocalMS. You have to set footholds yourself. So add an additional parameter into the constructor for MapleMonster asking for foothold. Then add a this.fh = foothold. Add a private final int fh; to the top. When you spawn the monster, remember to spawn it with the foothold. This will be left for you to do. There is also a place where it uses monster as MapleMonster before. You have to change that to MapleLifeFactory.getMonster(monster). This will not be a big CPU problem because MapleLifeFactory caches.
Don't forget to apply the same thing to other classes. If you see a variable that is unused or is a huge object, try to make it smaller. Do not over do this though.
What is supposed to happen: Well this is just an optimization. The code itself works completely fine.
What actually happens: By saving an int instead of saving MapleMonster, you free up some 300 bytes per object in every SpawnPoint object. This can add up to a lot since in many maps there are many spawn points. If you save 300 bytes per spawn point, you will end up saving a ton.
How to fix:
Change private MapleMonster monster; to private final int monster;
This is not all. Your spawn points are now broken and monsters will no longer have footholds. This is why monsters fall of platforms in LocalMS. You have to set footholds yourself. So add an additional parameter into the constructor for MapleMonster asking for foothold. Then add a this.fh = foothold. Add a private final int fh; to the top. When you spawn the monster, remember to spawn it with the foothold. This will be left for you to do. There is also a place where it uses monster as MapleMonster before. You have to change that to MapleLifeFactory.getMonster(monster). This will not be a big CPU problem because MapleLifeFactory caches.
Don't forget to apply the same thing to other classes. If you see a variable that is unused or is a huge object, try to make it smaller. Do not over do this though.
Storing Too Much Information:
Memory leak: So when you cache String data in the source sometimes it stores too much data. For example item names should only store item names. It stores the description and other information as well. This is a waste of memory and space. This leak is in almost every source starting from OdinMS days.
What is supposed to happen: The cache should only store the item name associated with the id and not anything else. When you call the cache, it returns the name of the item id if it exists. If it does not exist, it adds all the item ids from the XML loading from String.wz.
What actually happens: When you load all item ids, it also loads everything else in the file since it searches all the children of children. This leads to a bunch of extra information stored. Example:
The description of the item is saved as well and since each char takes up some space, a lot of chars would use up a lot. Also since the code is written to load all items at once, String data takes up a lot of room.
How to fix: There are a couple easy ways to fix this. The first way is to instead of loading from XML, make a parser that reads the IDs and make SQL data. The benefits are that it would be light, faster to read, and take no memory. Also, you can make modifications yourself. This is probably the easiest way to do it.
The second way is to just make sure when reading from XML it reads the name and not anything else. This is just a few more lines of code but it works and it takes less time to make than the SQL. The downside for SQL is that it will have to be updated per version and this will not need updating ever.
Apply this trick to many other data caches. MCDB is actually a great resource that should be used. If you can't get MCDB of the version you are coding, then code your own version. A significant amount of memory is saved.
What is supposed to happen: The cache should only store the item name associated with the id and not anything else. When you call the cache, it returns the name of the item id if it exists. If it does not exist, it adds all the item ids from the XML loading from String.wz.
What actually happens: When you load all item ids, it also loads everything else in the file since it searches all the children of children. This leads to a bunch of extra information stored. Example:
Code:
<imgdir name="4032190">
<string name="name" value="Orange Mushroom Doll"/>
<string name="desc" value="An adorable doll modeled after the Orange Mushroom, but something doesn't seem right…"/>
</imgdir>The description of the item is saved as well and since each char takes up some space, a lot of chars would use up a lot. Also since the code is written to load all items at once, String data takes up a lot of room.
How to fix: There are a couple easy ways to fix this. The first way is to instead of loading from XML, make a parser that reads the IDs and make SQL data. The benefits are that it would be light, faster to read, and take no memory. Also, you can make modifications yourself. This is probably the easiest way to do it.
The second way is to just make sure when reading from XML it reads the name and not anything else. This is just a few more lines of code but it works and it takes less time to make than the SQL. The downside for SQL is that it will have to be updated per version and this will not need updating ever.
Apply this trick to many other data caches. MCDB is actually a great resource that should be used. If you can't get MCDB of the version you are coding, then code your own version. A significant amount of memory is saved.
Other things that can improve performance:
1. Check your exceptions. Start logging all of them. There are a ton of ConcurrentModificationExceptions and NullPointerExceptions everywhere. In MapleServerHandler
Then fix each exception by reading the stack trace. Most of the null pointers can just be fixed by adding a null check. Usually it is better to fix the source of the null pointer, but in this case it really isn't that needed since lag plays a role here.
2. Diseases and buffs are coded pretty poorly (also did you know diseases and buffs should have the same buff stats). When you cancel a disease, all diseases are canceled (what??). The same thing for buffs. When you cancel a buff, it tries to cancel all of them and selects the one you want to cancel and cancels it. This needs recoding for sure. Monster statuses also fall under this category. When you cancel a monster status, all monster statuses are all canceled, not to mention poison already being some of the worst code ever. Fixing bad code can give a performance boost. Slow algorithms are not very welcome.
3. Use EnumMaps when able. This has better performance than its counterpart. I will not help with implementation since this requires modification of many classes and each class will be a little different. Do not attempt this unless you know what you are doing.
4. ConcurrentModificationExceptions are bad. Fix them since they lead to threading problems just like...
5. Use ReentrantReadWriteLocks instead of ReentrantLock. They are faster but have less control. Also they are supposed to prevent even more deadlocks. Anyhow, ReentrantLocks are more beneficial than synchronizing due to having control over the locking.
6. Use final modifier when the variables are constant (when it is a field). Doing this in classes and methods do not help that much and using final is used more as a marker to denote that the method or class cannot be changed. Performance gain there is negligible.
7. Analyze heap dumps yourself. There are a few memory leaks that I haven't listed here because they are hard to describe and fix. There are a few problems with quests that are leaky and buffs and monster statuses actually have leaks of their own (not what was described earlier).
Code:
public void exceptionCaught(IoSession session, Throwable cause) throws Exception {
if (cause instanceof IOException || cause instanceof ClassCastException) {
return;
}
MapleClient mc = (MapleClient) session.getAttribute(MapleClient.CLIENT_KEY);
if (mc != null && mc.getPlayer() != null) {
System.out.println("Exception caused by " + mc.getPlayer().getName());
}
cause.printStackTrace();
}2. Diseases and buffs are coded pretty poorly (also did you know diseases and buffs should have the same buff stats). When you cancel a disease, all diseases are canceled (what??). The same thing for buffs. When you cancel a buff, it tries to cancel all of them and selects the one you want to cancel and cancels it. This needs recoding for sure. Monster statuses also fall under this category. When you cancel a monster status, all monster statuses are all canceled, not to mention poison already being some of the worst code ever. Fixing bad code can give a performance boost. Slow algorithms are not very welcome.
3. Use EnumMaps when able. This has better performance than its counterpart. I will not help with implementation since this requires modification of many classes and each class will be a little different. Do not attempt this unless you know what you are doing.
4. ConcurrentModificationExceptions are bad. Fix them since they lead to threading problems just like...
5. Use ReentrantReadWriteLocks instead of ReentrantLock. They are faster but have less control. Also they are supposed to prevent even more deadlocks. Anyhow, ReentrantLocks are more beneficial than synchronizing due to having control over the locking.
6. Use final modifier when the variables are constant (when it is a field). Doing this in classes and methods do not help that much and using final is used more as a marker to denote that the method or class cannot be changed. Performance gain there is negligible.
7. Analyze heap dumps yourself. There are a few memory leaks that I haven't listed here because they are hard to describe and fix. There are a few problems with quests that are leaky and buffs and monster statuses actually have leaks of their own (not what was described earlier).
