Hopper Performance - Research and Suggestions

[oloflarsson]

Introduction

Thank you for the recent work on push based hoppers! They are great. Sadly we still get around 3 TPS on MassiveCraft due to huge hopper contraptions. I figured it must be possible to optimize hoppers further and did some research.

I tested on a brand new server with just WorldEdit to paste a schematic. This schematic contain 10 repetition of an actual hopper contraption built by a player on our server. After pasting the TPS drops to ~3.

Then I used VisualVM to find bottlenecks and it turns out that just by changing two small things, the performance impact of hopper transfers can be cut in half (approximately).

How to create hopper lag

1. Create folder /.
2. Create folder /plugins.
3. Create folder /plugins/WorldEdit.
4. Create folder /plugins/WorldEdit/schematics.
5. Download https://ci.destroystokyo.com/job/PaperSpigot/837/ and place in /.
6. Download http://builds.enginehub.org/job/worldedit/9350 and place in /plugins.
7. Download http://files.massivecraft.com/tmp/hoppers.schematic and place in /plugins/WorldEdit/schematics.
8. Create a minimalistic start.bat containing java -Xmx2G -Dcom.mojang.eula.agree=true -jar paperclip.jar.
9. Leave paper.yml and spigot.yml as is (default configuration).
10. Start the server.
11. Log on to the server using your Minecraft client.
12. Console command op <yourname>.
13. Ingame command /schematic load hoppers.
14. Ingame command //paste.
15. Wait a few minutes.
16. Ingame command /tps.
17. The TPS is horrible (about 3 TPS on my hardware).

Idea 1

Saves: ~13% CPU
Fix Complexity: Very Easy
Short Description: Avoid calls to org.bukkit.event.inventory.InventoryMoveItemEvent.getItem() since it clones in vain.
Long Description: The method org.bukkit.event.inventory.InventoryMoveItemEvent.getItem() is used three times in net.minecraft.server.TileEntityHopper.a(IHopper ihopper, IInventory iinventory, int i, EnumDirection enumdirection). In none of those thee cases is a clone operation required. The source code piece event.getItem() could simply be replaced with oitemstack. The same mistake is also made two times in net.minecraft.server.TileEntityHopper.H().
Screenshots:

• https://gyazo.com/efd7c3b5310d4207bbb80480cf2612c2
• https://gyazo.com/fd5d47a60170dc80ea77c5b6d1987bea

Idea 2

Saves: ~40% CPU
Fix Complexity: Easy
Short Description: BlockStateEnum is an immutable class. Cache or precalculate the hash code.
Long Description: Updating adjecent comparators uses loads of CPU. This is caused by hashCode recalculation of the MODE field in BlockRedstuneComparator. Luckily the BlockStateEnum is immutable and we can precalculate or cache the hash codes of this class after the first calculation.
Screenshots:

• https://gyazo.com/4bc21f271f2a8942c42a5c749244627c
• https://gyazo.com/6df55f6fea9057c455cad80feec39b2e
• https://gyazo.com/22c58b7827539ad054d0fe59014190fb
• https://gyazo.com/b0d60ed25fab0fe462a1805015b38899

[oloflarsson]

Interestingly I still only get ~3 TPS when I set the following options in spigot.yml:

ticks-per:
  hopper-transfer: 512
  hopper-check: 512
  hopper-amount: 64

Does this mean that the hopper based pushers bypasses the ticks-per.hopper-transfer configuration option?

[sgdc3]

I'm sure that since the push-based hopper patch the hopper-check setting is ignored

[Techcable]

@sgdc3 The push-based hopper makes hopper-check unnecessary.

[aikar]

I will look into this. But yes cloning is a huge issue, as there are a few more clones going on beyond what you listed too.

I got rid of those on my server and received major gains.

Just, I completely stripped the InventoryMoveItemEvent, making it easier for me to get rid of the unnecessary clones, which makes it harder to port my change. but when I Get time im gonna try.

[AlfieC]

I also tested caching the hashcode - almost completely eliminates those from any noticeable effect when I profile.

[AlfieC]

After a little investigation - not all of the event.getItem() calls can be converted to itemstack. This is because InventoryMoveItemEvent allows you to manually set the item. But we can eliminate all except the first - as things like "event.getItem().getAmount()" can be performed from 1 initial event.getItem(). Will investigate further to implement the changes that @aikar talked about.

[Techcable]

You could add an option to disable InventoryMoveItemEvent like I do in TacoSpigot.
The BlockStateEnum issue seems like a really easy fix, although I propose we utilize Maps.immutableEnumMap() as it's even faster.

[kashike]

Someone has opened a PR: #394

[AlfieC]

Not all of the event.getItem() calls can be replaced with oitemstack - but what can be done is one of two things:

Since event.getItem() returns a clone, we can make event.setItem() set a boolean field to true. If true, then we can use 1 event.getItem() call - but if false we can simply use oitemstack.

If the above solution is out of scope we can just replace a lot of event.getItem() calls with 1 and use the temporary itemstack.

[oloflarsson]

@AlfieC - I see your point. We don't need to clone however. How about creating a method InventoryMoveItemEvent#getItemRaw() that does not clone and then use this new method instead?

[AlfieC]

@oloflarsson I tested this out and it appears to work perfectly. Will PR it tonight/tomorrow, along with some cloning reductions. @aikar mentioned that he has some hopper work that he's working on, so I'll wait for him to push those first.

[zachbr]

The suggested improvements to BlockStateEnum have been made, with multiple people saying that the Event improvements are on the way.

[AlfieC]

I'm still waiting for @aikar to show me some of the hopper stuff he was talking about - not sure what to PR until then. No point me PRing stuff if it'll be reverted/changed a few days later.

[aikar]

I apparently lost the work, hit me up on irc for link to my patch and ill discuss my goal.

[oloflarsson]

I'm really glad to see this is being worked on 😸. Better hopper performance is going to make a lot of server owners happy 👍.

[AlfieC]

Without removing the event, I've managed to make hoppers almost entirely disappear from timings. Now I just have to do a little more bug testing and then I can PR and start working on redstone :)

[Talabrek]

@AlfieC Any update on these hopper changes that are supposed to make them disappear from the timings entirely? Hoppers have been an ongoing issue for me and are the #1 source of lag on my skyblock server, so I am very interested in anything that can improve their performance.

[Phoenix616]

I opened a pr (#445) that adds the ability to configure how the InventoryMoveItemEvent behaves. The major problem with it is that it gets called even when no item can move into the destination inventory when it is full. This results in protection plugins needing to check protections on every attempt to move even when no move occurs.

From the three settings the patch adds I prefer the one enabled by default which is calling the InventoryMoveItemEvent as cancelled if no move can occur. This is in line with other events that fire as cancelled if the Vanilla behaviour is to do nothing.

Most protection plugins already listen on the event with ignoreCancelled set to true and will therefor never know about if if the item wouldn't move. (There are two other options: completely disabling calling of the event on all moves or only for moves that wouldn't occur) You could potentially get even more performance out of these options if you completely rewrite the move logic as my current solution checks the target inventory twice (once for the original item before the event and the potentially modified after).

Regarding "Idea 1" in the original post from @oloflarsson:

Short Description: Avoid calls to org.bukkit.event.inventory.InventoryMoveItemEvent.getItem() since it clones in vain.
Long Description: The method org.bukkit.event.inventory.InventoryMoveItemEvent.getItem() is used three times in net.minecraft.server.TileEntityHopper.a(IHopper ihopper, IInventory iinventory, int i, EnumDirection enumdirection). In none of those thee cases is a clone operation required. The source code piece event.getItem() could simply be replaced with oitemstack. The same mistake is also made two times in net.minecraft.server.TileEntityHopper.H().
Screenshots:

https://gyazo.com/efd7c3b5310d4207bbb80480cf2612c2
https://gyazo.com/fd5d47a60170dc80ea77c5b6d1987bea

This solution would result in the issue that event.getItem().equals(oitemstack) would always compare the exact same object. (Which is why a clone is passed to the InventoryMoveItemEvent instead of the reference to the original CraftItemStack) In short: We would loose the information about the original item.

[aikar]

This will be resolved with #976