Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Conversation

Ghost-chu
Copy link
Contributor

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 Ghost-chu marked this pull request as draft February 12, 2024 16:01
@Ghost-chu
Copy link
Contributor Author

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
Copy link
Contributor Author

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!

@Ghost-chu Ghost-chu marked this pull request as ready for review February 13, 2024 06:31
@rutgerkok
Copy link
Owner

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
Copy link
Contributor Author

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 rutgerkok merged commit 5a00cf3 into rutgerkok:master Feb 16, 2024
2 checks passed
@rutgerkok
Copy link
Owner

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

I've merged your pull request, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants