Memory leak somewhere

Discussion in 'Spigot Plugin Development' started by Jaume, May 21, 2017.

  1. Ok, so what I'm doing is I'm constantly (every 5 server ticks) setting the velocity of invisible armorstands with a slime as passenger to vector (1, 0, 0). I'm using two BukkitRunnables. One of them spawns ArmorStands with a Slime every 80 ticks and the other keeps them moving while seemingly floating in the air (that's the reason I'm using an invisible armor stand, if I set the gravity of the slime to false it doesn't move I still don't know why). The armor stands are saved in an ArrayList so I can always know which ones to move, and they (and the slimes) get removed from the world and from the list when they are more than 60 blocks away from their origin.

    All this stuff seems to work, with the exception that sometimes the slimes spawn later than they should (there are 7 "slime cannons" facing the same direction and they should spawn an armor stand with a slime every 80 ticks, which they usually do but not every time). However, the big problem is that if I leave it running for a while (~half an hour or an hour) the game (client-size) lags a lot when I approach the part of the world where this happens.

    I've made kind of a debugging command to check for the number of alive slimes and armor stands, and also the number of armor stands in the list (so I know how many are being moved by the BukkitRunnable). The numbers are OK, the entities get despawned and removed as they should.

    The server TPS start at 20 and drop to about 19.3 or 19.4 when I notice the client is lagging (not talking about a specific moment, it slowly happens). However, the client shows non-moving slimes and slimes moving through them and lots of lag only where all of this is happening (If I tp away it runs fine).

    I will post code if you ask me to, and I can also post screenshots of the moving slimes (and the stuck ones during lag) if needed. Thanks in advance!

    EDIT: changed something to make it more understandable.
     
    #1 Jaume, May 21, 2017
    Last edited: May 21, 2017
  2. Asking.

    Need to re-read to try understand what exactly you mean, but without that it's going to be difficult for people to help.
     
  3. Post screenshots, code and a timings.
     
  4. There's a lot of code... I'll try posting the two BukkitRunnables and explain the variables involved. I'll post screenshots when possible!
     
  5. So the spawner runnable:
    Code (Java):
    @Override
        public void run() {
            if (locs.size() > 0) { //locs is a list containing the spawn locations. Checking if there are any spawn locations.
                for (Location location : locs) {
                    ArmorStand armorStand = (ArmorStand) location.getWorld().spawnEntity(location, EntityType.ARMOR_STAND); //spawn armorstand
                    Slime slime = (Slime) location.getWorld().spawnEntity(location, EntityType.SLIME); //spawn slime
                    slime.setAI(false); //disable slime's AI.
                    slime.setSize(1); //set size to small
                    armorStand.setVisible(false);//make the armorstand invisible
                    armorStand.addPassenger(slime);//add the slime to the armorstand as passenger

                    //At this point, there's a non-moving floating slime. All ok.

                    GardenManager.getManager(plugin).getGarden(gardenId).getPeaShooterMover().addArmorStand(armorStand, slime); //Add the armorstand to the list of armorstands located in the class that makes them move.
                }
            }
        }
    And the slime mover runnable:
    Code (Java):
        @Override
        public void run() {
            for (ArmorStand armorStand : armorStands) { //armorStands contains all the armorStands that need to be moved.
                if (armorStand.getLocation().getX() > 60) { //armorStands (with a Slime as a passenger) need to move towards X and 60 blocks. Checking if the armorstand has reached its destination.
                    for (Entity entity : armorStand.getPassengers()) {
                        entity.remove(); //remove the slime from the world
                    }
                    armorStand.remove(); //remove the armorstand from the world
                    armorStandsToRemove.add((ArmorStand) armorStand); //add the armorstand to another list because it can't be removed from the list you are iterating through.
                } else {
                    armorStand.setVelocity(velocity); //if it hasn't reached its destination, set its velocity to the predefined velocity.
                }
            }

            for (ArmorStand armorStand : armorStandsToRemove) {
                armorStands.remove(armorStand); //after moving all the armorstands and removing the ones that reached their destination from the map, remove them from the list.
            }

            armorStandsToRemove.clear(); //clear the temporal list.
        }
    BTW, I'm using a SyncRepeatingTask to run both runnables.

    EDIT: changed code type to Java.
     
  6. Maybe the issue is the following, without looking into the craftbukkit implementation details in order to verify this:
    When no player is around the area where you spawn those armor stands the server will unload those chunks, saving the entities to their chunk files. Now, those chunks will probably be reloaded again once your spawn-task runs (in order to be able to spawn new armor stands), or when you go near that area again. Now the issue is: The armor stand references you have in your list are no longer valid. When minecraft unloads a chunk, craftbukkit marks all entities in it as invalid (entity.isValid()). And when minecraft loads a chunk again, craftbukkit creates new entity wrapper objects for all of the loaded mincraft entities in that chunk.

    So your attempts to remove those armor stands will probably fail if your stored entity references are no longer valid (if the chunk has been unloaded in the meantime).
    Try the following: If the entity you try to remove/move is not valid, make sure the chunk it is/was in is loaded. Load it if it isn't. This might be a bit tricky though, as you don't know for sure where the entity is currently located it. The chunk might have been reloaded again in the meantime and the entity might then have kept moving into another chunk. You will somehow have to find out the chunk(s) the entity might be in. Maybe load all chunks in a certain range in front of your spawn locations.
    After the chunk is loaded you can get the current entity by its entity id. And then remove it.

    An other alternative might be to listen for ChunkUnloadEvents and react there. For example by either removing the entities there, or at least store their last known chunk. And get the new entity references once the chunk is loaded again (ChunkLoadEvent).
     
    #6 blablubbabc, May 21, 2017
    Last edited: May 21, 2017
  7. I've further tested and it looks like it only happens when the computer gets locked which is acceptable... Sorry about that D:

    Still, I'd like to fix these little inconsistencies with the spawn times. Screenshots:
    [​IMG]
    [​IMG]

    EDIT: Just to clear things up, the bad timing occurs at the spawn time. They all get spawned at the exact same time always but sometimes some of them start moving later. Once they have started moving, they keep on moving without any problems.
     
  8. Thanks for your answer and for the effort it took, I really appreciate it. I'll try it out if I finally discover it's not because of the locked computer.
     
  9. I just checked the craftbukkit code and I am pretty sure you won't be able to remove those entities if the chunk got unloaded in the meantime. Minecraft removes the entities when it unloads a chunk. And it creates new entity instances when it loads the chunk. Marking those old, no longer existing entities for removal won't actually remove the new / currently valid entity instance of that entity. So you will have to somehow handle chunk unloads / invalid entity references.
     
  10. Thanks, let's hope I don't need to xD. Do you know what could be causing the delays in some of the slimes? Should I try to run it async or will it break it all?
     
  11. I have no idea. Don't run it async though.
     
  12. OK, I won't run it async. I guess it's just lag... any ideas on how to optimize it? I've thought about moving them by casting them to a CraftSlime and using #getHandle()#getNavigation()#a() but I don't really know how it works... Can someone explain? Would the slimes keep jumping to their location or would they uniformly move (as I need)? Would I need the armor stands in such a case?
     
  13. You should principally be fine with using pure bukkit API here to move the slimes. I don't know what is causing your slimes to start moving at different times, but I doubt that is caused by using setVelocity.
    You could do some debugging: Output the x positions of all slimes in 'a row' once one is moving behind, to verify that they are really behind on the server and not just visually for the client.
    Also check if this is also happening if you are constantly standing nearby, to make sure that this timing issue isn't related to the chunk unloading issue mentioned above.
    Also which server version are you using? Maybe try an older version or the very latest version. There was a commit 2 days ago which might have effects on entity ticking if unloaded chunks are nearby: https://hub.spigotmc.org/stash/proj...mits/cda27c992d261aad19886e7427c2caa22b3f0c12

    You could also make sure all your spawners are in the same chunk to check if this issue only occurs if spawners are in differing chunks.
     
  14. Wouldn't it be more efficient to make them move rather than updating their velocity 5 times a second?
     
  15. I think navigation is for pathfinding. setVelocity directly sets the entity's motion, which will affect the entity's movement in the next tick. I am pretty sure navigation is basicly also simply modifying the entites motion / velocity in the end, but depending on the found path to its target location and entity behavior / attributes (movement speed, etc.). So you should be fine with using setVelocity.