search

rutgerkok / BlockLocker

Protect chests, doors and other blocks in Minecraft from being opened using protection signs
https://www.spigotmc.org/resources/blocklocker.3268/
MIT License
69 stars 43 forks source link

Performance improvements and adding Crafter to the default protection list #184

Closed Ghost-chu closed 8 months ago

Ghost-chu commented 8 months ago

Performance improvements

I've observed BlockLocker-induced performance degradation on our servers.

7fe95fc656c3cead45fb1b9181778819

The method getInventoryBlockOrNull(Inventory) attempts to acquire the InventoryHolder on each call, and this behavior indirectly creates a new BlockState capture on the block.

What's worse, however, is that this method is called repeatedly every tick, by the large number of Hoppers running on the server. And Hopper Minecart makes it even worse.

This PR checks the type of the requested Inventory. A fast algorithm is applied to the InventoryType that is not in COMPLEX_SEARCH_INVENTORY list. The unique Block object is returned directly from the Inventory's Location.

Also, for inventory that are in the COMPLEX_SEARCH_INVENTORY list, a quick double-chest check is performed, and if it's not a possible double-chest, we use the same quick algorithm as for the other InventoryTypes, and just return the Block object obtained by Location.

For Double-Chest, they use the same algorithms as in the past, and currently the Bukkit API still doesn't have a good interface to handle them efficiently.

Crafter

Minor change to add Crafter to the default list.

Ghost-chu commented 8 months ago

I have put this PR in draft status and we are applying this change from the test environment to the production environment. This PR status will be updated once we confirm that the change is not causing issues.

Ghost-chu commented 8 months ago

I am certain that this patch improves performance and does not introduce compelling issues. It works fine on a production environment.

Status Compare Image
Before
After

Here we go!

rutgerkok commented 8 months ago

Thanks! Didn't know that Inventory.getHolder() was so expensive.

I guess we can't just always return inventory.getLocation().getBlock() (with added null check) because of chest minecarts and other entities? But I see a problem here, as minecarts with chest also have the CHEST inventory type.

I'll have a look at it tomorrow.

Ghost-chu commented 8 months ago

Thanks! Didn't know that Inventory.getHolder() was so expensive.

I guess we can't just always return inventory.getLocation().getBlock() (with added null check) because of chest minecarts and other entities? But I see a problem here, as minecarts with chest also have the CHEST inventory type.

I'll have a look at it tomorrow.

I also noticed the Minecart issue when I was modifying this part of the code, but it seems that BlockLocker doesn't deal with it in the first place (Minecart can't be protected by Sign). I also tested the Hopper Minecart and was unable to remove items from protected containers.

rutgerkok commented 8 months ago

Then I might as well always return the block at the inventory, no matter what. 🙂

I've merged your pull request, thanks!