Performance impact when clearing blockdata from a whole chunk
Skizzles opened this issue · 6 comments
❗ Checklist
- I am using the official english version of Slimefun and did not modify the jar.
- I am using an up to date "DEV" (not "RC") version of Slimefun.
- I am aware that issues related to Slimefun addons need to be reported on their bug trackers and not here.
- I searched for similar open issues and could not find an existing bug report on this.
📍 Description
When using SuperiorSkyblock, it Hooks into Slimefun to remove items on Island reset, this causes a large tps drop when resetting or disbanding an island.
📑 Reproduction Steps
Install SSB
Place down a large amount of Slimefun content
Attempt to disband / reset an Island
💡 Expected Behavior
Ideally we (the server) and @OmerBenGera would love if this could be more optimized. So we are reaching out to receive assistance with doing so.
Issue:
BG-Software-LLC/SuperiorSkyblock2#850
📷 Screenshots / Videos
https://i.imgur.com/Q0ZMh7P.png
📜 Server Log
📂 /error-reports/ folder
N/A
💻 Server Software
Purpur
🎮 Minecraft Version
1.18.x
⭐ Slimefun version
Dev 996
(Happens regardless of addons installed or not)
🧭 Other plugins
SuperiorSkyblock2
It's no surprise BlockStorage is utterly slow, there is a reason its due for a rewrite.
Unfortunately, this will still take quite some time, have a look at the roadmap #3170
I am closing this issue though as there is little you can do besides waiting for the rewrite.
Nevermind, I just looked a bit deeper into the code from SuperiorSkyblock.
Yeah, there is definitely something that can be improved here in the mean time.
This right here, will be responsible for pretty much most of the impact.
First off all, it uses reflection which itself can already be pretty heavy on performance, as well as Java Streams and .forEach operators. All of those can be pretty heavy on their own but the bigger problem here is that all of those operate on a ConcurrentMap which will lock and unlock the object for the current thread quite repeatedly.
Added to that, BlockStorage.clearBlockInfo communicates with an async tasks that internally uses a ConcurrentMap too.
In total, this can really add up quite fast.
There are a lot of better ways to do this, the best performance improvement would probably be to cache all the blocks first and then add them in a single operation in order to not stress out the locks of ConcurrentHashMap.
But I reckon it would make Omer's life a lot easier to just provide a method like .clearAllBlocks() for this on our side.
Then he wouldn't need to rely on reflection or Java Streams anymore which would minimize the impact even further.
It won't solve the issue with BlockStorage being extremely slow anyway but seeing as this is a really inefficient way to do this and Slimefun offers no built-in way for it either, we can surely provide a temporary method here until the system is rewritten entirely.
Thank you for taking the time to respond! I am sure it would make Omer and other similar plugin devs lives easier indeed! I had tagged Omer in my above post so he should review and respond when he is available.
It looks like a good solution. However, I only need a way to clear data in a specific chunk, not the entire cache. If that what you meant, then it will be amazing.
I will keep the old method as well as adding support for the new improved methods.
Tag me here once you have added the method :)
@OmerBenGera this was finally added so you could implement its use for SSB
Seems to still have a performance drop. Here is a spark of it being used in SuperiorSkyblock (https://github.com/BG-Software-LLC/SuperiorSkyblock2):
https://spark.lucko.me/OfOiCTmISl