Slimefun4 icon indicating copy to clipboard operation
Slimefun4 copied to clipboard

Performance impact when clearing blockdata from a whole chunk

Open Skizzles opened this issue 2 years ago • 4 comments

❗ Checklist

  • [X] I am using the official english version of Slimefun and did not modify the jar.
  • [X] I am using an up to date "DEV" (not "RC") version of Slimefun.
  • [X] I am aware that issues related to Slimefun addons need to be reported on their bug trackers and not here.
  • [X] 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: https://github.com/BG-Software-LLC/SuperiorSkyblock2/issues/850

Slimefun hook: https://github.com/BG-Software-LLC/SuperiorSkyblock2/blob/003ed9c0e4264b925c98898ebb619f13d4beb46a/Hook_Slimefun/src/main/java/com/bgsoftware/superiorskyblock/hooks/support/SlimefunHook.java#L87

📷 Screenshots / Videos

https://i.imgur.com/Q0ZMh7P.png

📜 Server Log

https://pastebin.com/kYyu871z

📂 /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

Skizzles avatar Jan 18 '22 06:01 Skizzles

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.

TheBusyBiscuit avatar Jan 18 '22 10:01 TheBusyBiscuit

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.

https://github.com/BG-Software-LLC/SuperiorSkyblock2/blob/003ed9c0e4264b925c98898ebb619f13d4beb46a/Hook_Slimefun/src/main/java/com/bgsoftware/superiorskyblock/hooks/support/SlimefunHook.java#L105-L111

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.

TheBusyBiscuit avatar Jan 18 '22 10:01 TheBusyBiscuit

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.

Skizzles avatar Jan 18 '22 11:01 Skizzles

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 avatar Jan 18 '22 12:01 OmerBenGera

@OmerBenGera this was finally added so you could implement its use for SSB

Skizzles avatar Jun 24 '23 23:06 Skizzles

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

Skizzles avatar Jul 10 '23 23:07 Skizzles