TeamJM / journeymap

JourneyMap mod for Minecraft
http://journeymap.info
122 stars 10 forks source link

[Bug]: Memory Leak in combo with pregen #685

Open Speiger opened 3 months ago

Speiger commented 3 months ago

What happened?

I added JourneyMap support into ChunkPregen. And during testing i realized that my testing would run out of memory within 100k chunks being pregenerated (8GB free) on a vanilla system while when JourneyMap not being present it could just do it with far less ram. Edit: In fact turning of journeyMap mapping while it almost running out of ram basically frees it entirely to normal levels.

And I am pretty sure that is my fault due to how I implemented it. But I would love to fix this memory leak but journeyMap is pretty complicated internally so debugging it is basically impossible.

If you don't want to help out that is fine too. But if you do that would be epic.

This is the event handler that takes chunkpregens internal events and converts them into journeyMapClass. Since the Chunks are technically server sided I am cloning them and making them client sided chunks. (This is only done in singleplayer)

public class EventListener
{
    @SubscribeEvent
    public void onChunkGenerated(Generated event)
    {
        Minecraft mc = Minecraft.getInstance();
        if(mc.player == null || mc.level == null || !event.getWorld().dimension().equals(mc.level.dimension())) return;
        ChunkAccess access = event.getChunk();
        if(access instanceof LevelChunk) {
            JourneymapClient map = JourneymapClient.getInstance();
            if(!map.isMapping()) map.startMapping();
            try { map.queueOneOff(new MappingTask(toClientChunk((LevelChunk)access, mc.level), event.getWorld().dimension())); }
            catch(Exception e) { e.printStackTrace(); } 
        }
    }

    private LevelChunk toClientChunk(LevelChunk serverChunk, Level clientLevel) {
        ChunkPos pos = serverChunk.getPos();
        LevelChunk chunk = new LevelChunk(clientLevel, pos);
        ClientboundLevelChunkPacketData data = new ClientboundLevelChunkPacketData(serverChunk);
        chunk.replaceWithPacketData(data.getReadBuffer(), data.getHeightmaps(), data.getBlockEntitiesTagsConsumer(pos.x, pos.z));
        return chunk;
    }
}

This is the pregens mapping task that is based on JourneyMaps internal implementation. All it does is to trigger journeyMap into mapping these chunks so the data is being displayed.

public class MappingTask implements Runnable
{
    LevelChunk chunk;
    ResourceKey<Level> level;

    public MappingTask(LevelChunk chunk, ResourceKey<Level> level)
    {
        this.chunk = chunk;
        this.level = level;
    }

    @Override
    public void run()
    {
        JourneymapClient client = JourneymapClient.getInstance();
        CoreProperties props = client.getCoreProperties();
        boolean surfaceAllowed = FeatureManager.getInstance().isAllowed(Feature.MapSurface);
        boolean cavesAllowed = FeatureManager.getInstance().isAllowed(Feature.MapCaves);
        boolean topoAllowed = FeatureManager.getInstance().isAllowed(Feature.MapTopo) && props.mapTopography.get().booleanValue();
        boolean biomeAllowed = FeatureManager.getInstance().isAllowed(Feature.MapBiome) && props.mapBiome.get().booleanValue();
        if (!surfaceAllowed && !cavesAllowed && !topoAllowed && !biomeAllowed) return;

        EntityDTO player = DataCache.getPlayer();
        LivingEntity playerEntity = player.entityLivingRef.get();
        if (playerEntity == null) return;
        List<MapType> maps = new ObjectArrayList<>();
        if(surfaceAllowed) {
            maps.add(MapType.day(level));
            maps.add(MapType.night(level));
        }
        if(cavesAllowed) maps.add(MapType.underground(player));
        if(biomeAllowed) maps.add(MapType.biome(level));
        if(topoAllowed) maps.add(MapType.topo(level));

        File file = FileHandler.getJMWorldDir(Minecraft.getInstance());
        ChunkMD md = new ChunkMD(chunk);
        DataCache.INSTANCE.addChunkMD(md);
        for(MapType type : maps)
        {
            RegionCoord rCoord = RegionCoord.fromChunkPos(file, type, md.getCoord().x, md.getCoord().z);
            RegionDataStorageHandler.Key key = new RegionDataStorageHandler.Key(rCoord, type);
            RegionData regionData = RegionDataStorageHandler.getInstance().getRegionData(key);
            try
            {
                JourneymapClient.getInstance().getChunkRenderController().renderChunk(rCoord, type, md, regionData);
            }
            catch(Exception e)
            {
                e.printStackTrace();
            }
        }
        DataCache.INSTANCE.removeChunkMD(md);
    }

}

Thank you for at least looking at this ^^"

Mod Loader

Forge

Minecraft Version

1.19.2

Version

5.9.18

Relevant log output

No response

mysticdrew commented 3 months ago

Your best bet is to use a profiler like YourKit and see if you can find out what is using a lot of memory. It would also probably be easier if you came into our discord to work through this.

Speiger commented 3 months ago

@mysticdrew i wanted to gauge interest first before I decided to ping you on the discord. ^^"

I usually would use VisualVM but ever since 1.16+ that became useless due to their stack spam. But see ya in a second.

mysticdrew commented 3 months ago

I recently made the switch to YourKit from visualVM, it is much better than visualVM and with opensource products, it is pretty easy to get a free license. No need to ping, just say hey in the projects channel.

mysticdrew commented 2 months ago

You left the discord server. Do you have any updates on this?

Speiger commented 2 months ago

@mysticdrew atm no, i simply don't have time at the moment to deal with this problem. Once my IRL project is done (which will take aproximately 3 months) I will sit down and look at the problem deeply. And I do plan to fix it for all versions i see this problem in.

This only affects large singleplayer scans, and it can be fixed with simply disabling mapping/disabling the plugin support from my side so it isn't a "must be fixed right now queue".

(Also i tend to leave servers if my Business is done because i hide like that, i don't mean it in a bad way ^^" sorry to confuse you.)

Speiger commented 3 weeks ago

@mysticdrew just to keep you updated. At about the end of this month i should have some serious time to investigate/test things again.

I did though check into your code every now and then and I recently found 2 lines of code I never ran.

    RegionImageCache.INSTANCE.flushToDiskAsync(true);
    DataCache.INSTANCE.stopChunkMDRetention();

Basically my ChunkMDs never got force flushed and released. This could be the reason for the memory leak if stale so many stale references stayed present. It could also be that RegionImageSets simply don't get the notifier that they should free ram after them being done and that accumulates over time and these two functions solve this ^^"

Anyways i will start in a couple weeks doing some true investigations.

mysticdrew commented 3 weeks ago

Sounds good!