Storing Players bad practice?

Discussion in 'Plugin Development' started by zachoooo, Jun 11, 2012.

Thread Status:
Not open for further replies.
  1. Offline

    zachoooo

    In one of my plugins it stores players in a hash map. I have heard from people that this can lead to memory leaks. This is a personal plugin and I will not be using multiworld. Can anyone explain to me if this rumor of storing players is actually true and if so why.
     
  2. Offline

    CorrieKay

    Yes. This will technically result in memory leaks, however, its not that big of a deal (meaning the loss of ramspace, not the underlying cause)

    The real problem exists in the fact that the player object will stay referenced in your hashmap until the end of time


    ...unless you have a listener to remove them from every hash map, in every class that has a hashmap that holds player objects.
     
  3. ^ The above, the EntityPlayer that is stored in the CraftPlayer that you store will need to be kept in memory. Besides, using Strings (the player's name) as a HashMap key and in HashSets is faster. CraftPlayer overrides hashCode() and equals() appropiately, though, so the performance difference here is not really big.

    And additionally, some method calls will cause problems when the player dis- or reconnects, because then the entity that is being referred to no longer exists on the server (and has been replaced by a new one).
     
  4. Offline

    Korvacs

    It should not be considered bad practice to store players in a collection.

    The reason people may believe this leads to memory leaks is because you might not remove players from the collection once you are finished with that player object, but even then it shouldn't pose a big issue, the player object itself is probably no bigger than 1kb at the most, so unless you plan on storing hundreds of thousands of player objects in there and forgetting about them you wouldn't even notice it. The other possibility is that you have some code that adds a player to a collection but allows for the possibility of duplications.

    So just make sure that you arnt adding someone to the collection who is already present, and remember to remove them once they are no longer needed and you will be perfectly fine and wont experience any memory leaks.
     
  5. Offline

    Sagacious_Zed Bukkit Docs

    The real problem is not the memory that it consumes, but that the player object itself is not always the same object, which leads to some unexpected results.
     
  6. read my signature for more information about this, :p
     
  7. But the player object holds references to chunks, a world, a list of others are invisible (I just saw that for myself:
    private Map<String, Player> hiddenPlayers = new MapMaker().softValues().makeMap();
    ), a packet (netServer) handler, ... And that objects may have future references...
    With that in mind the memory leak seems bigger than you first thought, doesn't it?

    LOL :D
     
  8. Offline

    Njol

    If you only need to store values for players while they are online you can use a WeakHashMap<Player, Whatever> to store them, as the values will automatically be removed from the map when the player objects are collected by the garbage collector.
     
  9. Offline

    ZNickq

    Except that's wrong...it doesn't keep anything from unloading, the player is removed from bukkit's online player list, so it's not accessed at all -.-
     
  10. fixed
     
  11. Not accessed != Removed. You don't understand how Java works, huh?

    Say code 1 (the MC server) has object A (the player).
    Now code 2 (your plugin) stores a reference to object A.
    Code 1 now removes the reference to object A. At that point Javas GC would see object A as useless and remove (unload) it from RAM. BUT: Now code 2 still has a reference, as such the GC sees object A as used, as such it won't get removed.
    Next step could be:
    Code 1 removes some references to objects (let's call them objects B, C, D, E, ... and let's assume that are chunks) that where referenced in object A. Javas GC can't remove them from RAM cause they are still referenced in object A which is still referenced in code 2.

    Being unable to remove code from RAM that should be removed is a memory leak...
     
  12. Offline

    nisovin

    I'm pretty sure all of these issues aren't actually issues if you manage your HashMap well by removing items from it when they're no longer needed.
     
  13. Offline

    CorrieKay

    aye, this is what i do in the last plugin i ever utilized player objects like that in. (i wont be doing that anymore, anyhow)

    However, i distinctly remember someone bringing up a point that argued against this stance...
     
  14. Offline

    Sagacious_Zed Bukkit Docs

    but why worry about all the cases that the player object can change if you just use a string?
     
    V10lator and h31ix like this.
  15. Offline

    CorrieKay

    My plugin that uses player objects actually has to do a lot of stuff when players log in or out, plus i already have methods for removing the player, so its really easy to just add
    removePlayer(event.getPlayer());
    to my player quit events.
     
  16. CorrieKay but are you sure a kicked player calls the PlayerQuitEvent, for example?
     
  17. Offline

    Korvacs

    Potentially, testing would need to be done to come up with a more accurate figure, but even so if they follows the steps I explained they wouldn't have any problems in terms of a memory leak. High memory usage thanks to reference to the map (although im unsure how minecraft actually utilises references to chunks etc, would be none nonsensical for every player to have a copy of the current chunk, so more likely just points to an existing copy in memory, so no additional memory usage here?) etc is another matter entirely, if this was a concern then potentially storing the UUID/entity id value would be more appropriate.

    If you were worried about keeping the objects up to date you could use the scheduler to update your collection every couple of seconds, or if your not so concerned every 30 seconds/1 minute.

    Please feel free to correct me if im wrong, i come from a C# background where i wrote emulators for game servers from the ground up, so alot of my knowledge is transferable but the details alter slightly when it comes to Java, and certainly minecrafts handling of memory.
     
  18. Offline

    CorrieKay

    Does it not call a PQE after the kicked player leaves?
     
  19. I don't know, that's why I'm asking if you're sure about that... ;)
     
  20. Offline

    CorrieKay

    Never had an issue with that... So i figured it does, since theyre kicked, the player leaves.. I believe ive seen player quit event code be executed on kicks, however i couldnt tell you 100% sure... Ill make sure to check.
     
  21. Offline

    ZNickq

    I understand java, you do not seem to understand english though... where exactly in my posts did i say that it's not a memory leak? I was talking about the load of bs in that signature, which basically said that multiworld can't unload the world, and that bukkit can't save the player's data (wat?)... just because you keep an instance of it!
     
  22. This is not what I said (quotes please...).

    I said basically that even after multiworld (or another plugin) unloaded the world parts of it hang (unused!) in RAM and that after a player unloads (and so after the data has been (successfully) written to disc) the player object itself still can't be removed by the GC.

    Also I did not say this is the thread, you just are able to read it in ferrybigs signature. Even if that's a (modified) quote from me I didn't say it in this thread, so please refer to what I really wrote for that topic.

    //EDIT: But you are right, english isn't my native language. Want to blame me for that instead of asking me if something sounds weird?
     
    CorrieKay likes this.
  23. Offline

    ZNickq

    ...
    The point being, i agree with what you said, i never said i don't, that guy's signature is wrong. Do we agree here? -.-
     
  24. Offline

    p000ison

    PlayerQuitEvent is called when the kick event is called.
     
Thread Status:
Not open for further replies.

Share This Page