Storing player in hashmap causes memory leaks

Discussion in 'Plugin Development' started by HeyAwesomePeople, May 31, 2014.

Thread Status:
Not open for further replies.
  1. Hello. I have heard that storing a player in a HashMap can cause a memory leak. This is how I am storing the player:

    HashMap<Player, PlayerData>

    If I put a player into this hashmap when they join, and take them out when they leave, is there still a memory leak?

    Thanks,
    HeyAwesomePeople
     
  2. Offline

    xTrollxDudex

  3. Offline

    Stealth2800

    It's commonly warned against because people typically don't handle the stored data correctly. As long as you're careful about it, you should be fine.
     
  4. Offline

    AronTheGamer

    Use their names (p.getName())
     
  5. Offline

    aaomidi

    If he removes it, its fine.
     
  6. Offline

    AronTheGamer

    Well, that's true. Btw, I like your ava :)
     
  7. I like using uuids. Just cuz it only takes a few extra words.
    player.getUniqueId().toString()
    Store it as a string.
    No worries of depreciation or possible memory leaks or playerdata loss.. No worries at all.
     
  8. Offline

    JuicyDev

    Also it would be better to handle UUIDs, in case a player doesn't get removed for some reason, if in the future they logged back in with a different name then they would lose their "PlayerData" info.
     
  9. Offline

    L33m4n123

    If you keep storing the string after the player left, it is still a memory leak. Ok. It is not as big as the player object. But anyways. If you make sure to remove the object from your list when you don't need it anymore or when the player logs out, you are more than good to go to store the player object
     
  10. Offline

    RawCode

    I have heard that storing a player in a HashMap can cause a memory leak.

    post source of that information. HeyAwesomePeople
     
  11. Offline

    Zupsub

    Well, since java only stores references, there is no memory difference between a HashMap/List/Queue<Player> or a <UUID> or a <String> or <World>



    But the only problem is, that if you store the player reference in your List/Map/... then the "real player object" won't get removed!

    The Java Garbage Collector (GC) knows that you stored the player's reference and even if bukkit doesn't need the player anymore, the player will be left in memory for your plugin.

    And thats the possible memory leak RawCode .
     
    WinX64 and L33m4n123 like this.
  12. Offline

    RawCode

    read thread name carefully, it state that storing player cause memory leak by itself and this statement is invalid Zupsub
     
  13. Offline

    Zupsub

    Read my post carefully, I've never said storing player cause memory leaks itself. :) RawCode
     
  14. Offline

    L33m4n123

    He does not talk about you ;) He is talking about the title of the thread^^
     
  15. This a temporary data, only stores data for the player while they are online, so no need to worry about UUID here.

    And I didn't expect this thread to get so many posts. xD. I have figure it out now, thanks!
     
  16. Offline

    thecrystalflame

    you could do it fairly safely with WeakHashMaps rather then its kin.

    in fact if your not using the player name or uuid and insist on using the Player object WeakHashMaps is the only way it should ever be done.
     
  17. Offline

    1Rogue


    You should just store the UUID directly:

    Code:java
    1. Map<UUID, ?> yourMap = new /*Some*/Map<>();



    Bad advice < No advice at all

    WeakHashMaps store map values using java.ref.WeakReference, and allows the garbage collection of those values. This will both result in unexpected results while the player is still online, and will not resolve the keys of the Player object still being in the map.
     
  18. Offline

    xTrollxDudex

    thecrystalflame
    False. The only implementation that should be used is the direct removal of the player from the map itself on leave.

    1Rogue
    False. You have it backwards.
    First line of WeakHashMap javadoc:
     
  19. Offline

    Sleaker

    the question is rather, how can the Player object get GCed while still online? I think that would let people understand why not to use WeakHashMap, rather than simply stating 'unexpected results will occur'
     
  20. Offline

    RawCode

    I will help you all to understand how JVM GC over player references works:

    0) Low level "unsafe" classes can be found here
    https://github.com/RawCode/UBT/tree/master/src/rc/ubt

    1) Tracing to get memory offset performed by something like this:

    Code:
    @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
    	public void _EntityDamageEvent(PlayerJoinEvent e)
    	{
    		CraftPlayer ep = (CraftPlayer) e.getPlayer();
    		
    		System.out.println(UnsafeImpl.Object2Trace(ep));
    		try
    		{
    			ep = (CraftPlayer) UnsafeImpl.unsafe.allocateInstance(CraftPlayerImpl.class);
    		} catch (InstantiationException e1)
    		{
    			e1.printStackTrace();
    		}
    		System.out.println(UnsafeImpl.Object2Trace(ep));
    		
    	}
    
    After tracing performed you can set class of object by something like this:

    Code:
    package rc.ubt.handlers;
    
    import org.bukkit.Bukkit;
    import org.bukkit.craftbukkit.v1_7_R3.entity.CraftPlayer;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.EventPriority;
    import org.bukkit.event.Listener;
    import org.bukkit.event.entity.EntityDamageEvent;
    import org.bukkit.event.player.PlayerJoinEvent;
    
    import rc.ubt.Loader;
    import rc.ubt.implementations.UnsafeImpl;
    
    public class Respawn implements Listener {
    	
    	public Respawn(){Bukkit.getPluginManager().registerEvents(this, Loader.INSTANCE);}
    	
    	@EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
    	public void _EntityDamageEvent(PlayerJoinEvent e)
    	{
    		CraftPlayer ep = (CraftPlayer) e.getPlayer();
    		
    		System.out.println(UnsafeImpl.Object2Trace(ep));
    		try
    		{
    			ep = (CraftPlayer) UnsafeImpl.unsafe.allocateInstance(CraftPlayerImpl.class);
    		} catch (InstantiationException e1)
    		{
    			e1.printStackTrace();
    		}
    		System.out.println(UnsafeImpl.Object2Trace(ep));
    		
    		int newclassid = UnsafeImpl.unsafe.getInt(ep, 8L);
    		
    		UnsafeImpl.unsafe.putInt(e.getPlayer(), 8L,newclassid);
    		
    		ep.sendMessage("TESTING");
    	}
    }
    
    PlayerImplementation shoud looks like this:

    Code:
    package rc.ubt.handlers;
    
    import net.minecraft.server.v1_7_R3.EntityPlayer;
    
    import org.bukkit.craftbukkit.v1_7_R3.CraftServer;
    import org.bukkit.craftbukkit.v1_7_R3.entity.CraftPlayer;
    
    public class CraftPlayerImpl extends CraftPlayer
    {
    
    	public CraftPlayerImpl(CraftServer server, EntityPlayer entity) {
    		super(server, entity);
    		// TODO Auto-generated constructor stub
    	}
    
    	@Override
    	public void finalize()
    	{
    		System.out.println(this.toString() + " is garbaged!");
    	}
    	
    	@Override
    	public void sendMessage(String s)
    	{
    		System.out.println("NOPE");
    	}
    }
    
    You will see "nope" in console on your login as sign of success.
    If you try to sent any message to player you will see same nope in console and player wont see any message.
    This can be perfomed over classes or over individual objects, in our case we change class of specific object.

    After you disconnect from game nothing will happen.
    You wont see message about "x is garbaged".

    WTF?
    Lets create other object, lets call it hashmapanchor:

    Code:
    package rc.ubt.handlers;
    
    public class HashMapAnchor
    {
    
    	public HashMapAnchor()
    	{
    		payload = new long[999];
    	}
    	
    	long[] payload = null;
    	
    	@Override
    	public void finalize()
    	{
    		System.out.println("HashMapAnchor is garbaged!");
    	}
    }
    
    When player join we create such object and put into hashmap.
    When player leaves - remove entry from map.

    Something like this:

    Code:
    	@EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
    	public void _EntityDamageEvent(PlayerQuitEvent e)
    	{
    		
    		t.put(e.getPlayer().getName(), null);
    	}
    
    inside join
    
    		t.put(e.getPlayer().getName(), new HashMapAnchor());
    
    Try again!

    [18:24:16] [Finalizer/INFO]: HashMapAnchor is garbaged!

    Our object is garbaged each time we relogin, this means that our garbaging indication works.

    Answer is:

    Player object of cBukkit server is permanently leaked, this may be caused by some faulty plugin, or some faulty code o SHI~~ wait we can run dump heap analizer from eclipse!

    Relogin few times, run heap analizer (you can get it here http://download.eclipse.org/mat/1.3.1/update-site/) and search for CraftPlayer objects, then show reference map:

    [​IMG]

    Looks like players leaks in default implementation.
    Or we caused leak by out hashmap storage.

    You can try with fixed hashmap self.

    This extremely long and boring post was about "how developer shoud resolve issues and questions - by research"
     
Thread Status:
Not open for further replies.

Share This Page