BentoBox icon indicating copy to clipboard operation
BentoBox copied to clipboard

Memory leak with cache

Open HamtaBot opened this issue 1 year ago • 2 comments

Expected behavior

analyzing a heapdump showed me 172k entries on cache which seems to be a lot considering there is only 12k islands on my server , we have no auto purge enabled https://discord.com/channels/272499714048524288/310623455462686720/1238209472015503381 after talking here on discord with BONNe he told me to open an issue on github

Observed/Actual behavior

ttt i can give you in private a link to download the heapdump if needed

Steps/models to reproduce

N/A

BentoBox version

bbox ver [21:28:00 INFO]: Serveur Invalide (UNIVERSESPIGOT) 1.20.1. [21:28:00 INFO]: Version de BentoBox : 1.24.1-SNAPSHOT-b2435 [21:28:00 INFO]: Base de données : MARIADB [21:28:00 INFO]: Mondes de jeu chargés : [21:28:00 INFO]: oneblock_world (OneBlock) : Overworld [21:28:00 INFO]: Addons chargés : [21:28:00 INFO]: AOneBlock 1.14.1 (ENABLED) [21:28:00 INFO]: Bank 1.7.0-SNAPSHOT-b83 (ENABLED) [21:28:00 INFO]: Biomes 2.2.0-SNAPSHOT-b251 (ENABLED) [21:28:00 INFO]: Chat 1.1.4 (ENABLED) [21:28:00 INFO]: ControlPanel 1.13.1-SNAPSHOT-b61 (ENABLED) [21:28:00 INFO]: Level 2.13.0-SNAPSHOT-b565 (ENABLED) [21:28:00 INFO]: Likes 2.4.0-SNAPSHOT-b97 (ENABLED)

Plugin list

No response

Other

No response

HamtaBot avatar May 09 '24 19:05 HamtaBot

How often do you restart your server? I'm interested how this number grew. Do you have a lot of island resets?

tastybento avatar May 11 '24 14:05 tastybento

There has been work done recently on the memory structures in the IslandCache code (it hadn't been touched for 6 years) and I found some bugs where islands would not be fully removed from the grid so I cleaned it up recently. Would you be able to move to the latest BentoBox build?

tastybento avatar May 11 '24 14:05 tastybento

There has been work done recently on the memory structures in the IslandCache code (it hadn't been touched for 6 years) and I found some bugs where islands would not be fully removed from the grid so I cleaned it up recently. Would you be able to move to the latest BentoBox build?

i am still running under 1.20.1 i'm not sure if bentobox latest can run on this version , and i'm doing daily restarts

HamtaBot avatar May 11 '24 18:05 HamtaBot

Yes, it'll work with 1.20.1.

tastybento avatar May 11 '24 20:05 tastybento

Yes, it'll work with 1.20.1.

nice, is there a list of changes from my version to the lastest ? since i edited all lang files and stuff i wants to be sure to know what changed

HamtaBot avatar May 11 '24 20:05 HamtaBot

did not got any OUt of memory since i updated bentobox, i'll do an heapdump when there is less players

HamtaBot avatar May 16 '24 17:05 HamtaBot

well bad news there is still a lot of entries image

HamtaBot avatar May 17 '24 10:05 HamtaBot

this heapdump was made when there was 110 players online If needed i can send you a link to download the Heapdump on mediafire

HamtaBot avatar May 17 '24 10:05 HamtaBot

well bad news there is still a lot of entries

image

Can you check what version of BentoBox you are running? The head dump shows a field called islandsByLocation but that was removed as part of the improvements I made. You can see in the cache code it's not there: https://github.com/BentoBoxWorld/BentoBox/blob/develop/src/main/java/world/bentobox/bentobox/managers/island/IslandCache.java

So, could you still be using an older build?

tastybento avatar May 17 '24 13:05 tastybento

I am running 2.3.0 IMG_5999

HamtaBot avatar May 17 '24 13:05 HamtaBot

That explains it. That's the release version but the changes are in 2.4.0-SNAPSHOT. You can download the latest from https://ci.codemc.io/job/BentoBoxWorld/job/BentoBox/ However SNAPSHOTs are not released and may be unstable so of course, if you use it then take backups.

tastybento avatar May 17 '24 14:05 tastybento

That explains it. That's the release version but the changes are in 2.4.0-SNAPSHOT. You can download the latest from https://ci.codemc.io/job/BentoBoxWorld/job/BentoBox/ However SNAPSHOTs are not released and may be unstable so of course, if you use it then take backups.

well i dont reallly want to break my server , is it really unstable ? or only a few small bugs

HamtaBot avatar May 17 '24 15:05 HamtaBot

i uploaded the latest CI version we will see tomorrow at server restart

HamtaBot avatar May 17 '24 20:05 HamtaBot

lets hope it doesnt break slimefun im not using the latest version cuz we edited it to translate it since we cant translate the plugin without changing the code

HamtaBot avatar May 17 '24 20:05 HamtaBot

That explains it. That's the release version but the changes are in 2.4.0-SNAPSHOT. You can download the latest from https://ci.codemc.io/job/BentoBoxWorld/job/BentoBox/ However SNAPSHOTs are not released and may be unstable so of course, if you use it then take backups.

updated and did an heapdump again and it seems the field is not here anymore but bentobox still uses a lot of memory image and as far as i can tell it uses A LOT of memory since the first on the list is slimefun with 45% of usage because slimefun being slimefun so if i remove slimefun we can expect bentobox to use atleast 80% of the whole heap dump

the good thing is that there is less entries than before which means we already have an improvement

HamtaBot avatar May 18 '24 08:05 HamtaBot

How many islands do you actually have in the database? They would be in the Islands table.

tastybento avatar May 18 '24 13:05 tastybento

How many islands do you actually have in the database? They would be in the Islands table.

around 12.5k islands

HamtaBot avatar May 18 '24 15:05 HamtaBot

I reviewed some heap dumps and I don't see a "memory leak" per se, but the cache does load all the islands in at startup so there were a lot of Island objects in there. Previously, the islands were stored in 3 maps in IslandCache, and with the lasted PR, now it's just one. In checking the heap dump, this should reduce the memory usage further.

tastybento avatar May 19 '24 05:05 tastybento

I reviewed some heap dumps and I don't see a "memory leak" per se, but the cache does load all the islands in at startup so there were a lot of Island objects in there. Previously, the islands were stored in 3 maps in IslandCache, and with the lasted PR, now it's just one. In checking the heap dump, this should reduce the memory usage further.

i'll test this pr at next daily server restart and keep you updated

HamtaBot avatar May 19 '24 11:05 HamtaBot

image this is how it looks now

HamtaBot avatar May 21 '24 17:05 HamtaBot

Okay, I bit the bullet and we no longer load all the islands into memory. The cache is built up over time as and when islands are accessed. Assuming that most servers restart regularly, this should keep memory usage down to the minimum. The latest SNAPSHOT will have this in it.

tastybento avatar May 24 '24 04:05 tastybento

Okay, I bit the bullet and we no longer load all the islands into memory. The cache is built up over time as and when islands are accessed. Assuming that most servers restart regularly, this should keep memory usage down to the minimum. The latest SNAPSHOT will have this in it.

Alright , I’ll test this out at the weekend , there is 2 benefit of this actually (less RAM usage, and better startup time )

HamtaBot avatar May 24 '24 05:05 HamtaBot

The only thing I am confused is why the memory report says it has more than 85k entries for islands. I would expect to have as many entires loaded as islands in the database, not more.

BONNe avatar May 24 '24 07:05 BONNe

I've been researching that too and from what I can tell those are not taking up hardly any memory - I think they are just references to keys in the maps. Maybe due to hashmap loading. They may be pending GC. Not sure.

tastybento avatar May 24 '24 14:05 tastybento

I've been researching that too and from what I can tell those are not taking up hardly any memory - I think they are just references to keys in the maps. Maybe due to hashmap loading. They may be pending GC. Not sure.

did an heapdump with the new snapshot image i didnt changed anything

HamtaBot avatar May 25 '24 16:05 HamtaBot

bbox version [18:30:03 INFO]: Serveur PAPER 1.20.1. [18:30:03 INFO]: (git-UniverseSpigot-"8ee0821" (MC: 1.20.1)) [18:30:03 INFO]: Version de BentoBox : 2.4.0-SNAPSHOT-b2628 [18:30:03 INFO]: Base de données : MARIADB [18:30:03 INFO]: Mondes de jeu chargés : [18:30:03 INFO]: oneblock_world (OneBlock) : Overworld [18:30:03 INFO]: Addons chargés : [18:30:03 INFO]: AOneBlock 1.14.1 (ENABLED) [18:30:03 INFO]: Bank 1.7.0-SNAPSHOT-b83 (ENABLED) [18:30:03 INFO]: Biomes 2.2.0-SNAPSHOT-b251 (ENABLED) [18:30:03 INFO]: Chat 1.1.4 (ENABLED) [18:30:03 INFO]: ControlPanel 1.13.1-SNAPSHOT-b61 (ENABLED) [18:30:03 INFO]: Level 2.13.0-SNAPSHOT-b565 (ENABLED)

HamtaBot avatar May 25 '24 16:05 HamtaBot

Yes, I can replicate with these builds. It looks like something got changed somewhere because the build I have locally works fine. I'm away this weekend, but I'll try and find out where the change got made.

tastybento avatar May 25 '24 17:05 tastybento

I found the issue and fixed it, but I also uncovered a lot of items that were depending on all the islands being loaded so I had to do a lot of work to handle those. For example, if you reset the flags for all islands then that kicks off an async task to go through all the islands and do that without loading them into cache, unless they are already there. There were also other times when the previously "cheap" call of getting all the islands had to be reworked with other approaches. Anyway, the upshot of this is that once I merge the changes in, it should all work, but there could be unfound bugs that crop up due to the changes. https://github.com/BentoBoxWorld/BentoBox/pull/2383

tastybento avatar May 26 '24 04:05 tastybento

I found the issue and fixed it, but I also uncovered a lot of items that were depending on all the islands being loaded so I had to do a lot of work to handle those. For example, if you reset the flags for all islands then that kicks off an async task to go through all the islands and do that without loading them into cache, unless they are already there. There were also other times when the previously "cheap" call of getting all the islands had to be reworked with other approaches. Anyway, the upshot of this is that once I merge the changes in, it should all work, but there could be unfound bugs that crop up due to the changes. #2383

do you guys have beta testers ? since this is a pretty significative change i dont want to break my server ahah

HamtaBot avatar May 26 '24 04:05 HamtaBot

ok i just experienced the thing u was talking the server freezed for a solid 15 seconds, i just updated to latest dev builds, i'll tell you if there is issues

HamtaBot avatar May 26 '24 11:05 HamtaBot