(1.20) Basic Inventory Hoppers cause mspt to increase linearly with count and available storage slots
pythonmcpi opened this issue ยท 1 comments
Inventory network behavior appears to have been at least partially rewritten since 1.20, so the issue might not occur on 1.21. This issue may be related to #556.
This issue report is based on 1.20-1.7.1-fabric using barrels to provide slots for the storage network. Forge code uses different apis for item transfer, so performance may differ and this issue might not be present at all.
TL;DR inventory hoppers in output mode will cause lag, especially if there is no matching items in your storage network or if the hopper's destination container is full. This scales with number of (non-empty) slots in the storage network.
Context/Vocab
If you're familiar with the technical side of minecraft, you probably already know this and can skip this section.
A minecraft server (including the internal server that runs when you play singleplayer) updates in discrete time steps known as ticks. A normal server targets 20 ticks per second (tps). Milliseconds per tick (mspt) measures how long a single tick takes. If ticks take more than 50 milliseconds to finish, the server will not be able to hit the target of 20 tps and players will experience lag. This lag can be directly observed by slower redstone, lagbacks with entity movement, and longer eating durations. Therefore, lower mspt is better and means less lag.
Trigger Conditions
- Basic Inventory Hoppers should be in output mode (removes items from the storage network). Input mode doesn't appear to have this issue.
- Basic Inventory Hoppers must have a filter set, or they do not appear to affect mspt significantly. I have only tested with setting regular items as filters, but I expect the impact to be the same or worse when using filter items.
- Basic Inventory Hoppers must be pointing into a valid container. I haven't measured the difference between an empty destination container and a full one; I have been testing with empty ones.
- Basic Inventory Hoppers must not be powered by redstone, or they do not appear to affect mspt significantly.
- Basic Inventory Hoppers must be connected to a valid storage network. Impact on mspt seems to also scale by storage slots attached to the network, or items on the network.
- The storage network does not have the item that the filter is set to. I would expect significantly less mspt impact when the item is available and the destination container is not full (see code analysis below).
Expected Behavior
Adding a small number of Basic Inventory Hoppers does not significantly increase mspt.
Observed Behavior
Adding 27 Basic Inventory Hoppers to a storage network with 14491 filled slots out of 17064 available slots increased mspt from ~20 to ~40. The majority of items are bulk resources (logs, cobblestone, netherack) with no additional nbt data. The top 15 items take up 11879 of the filled slots. There are 639 unique items (different item type or nbt) in the storage.
Fabric code analysis
We start in the updateServer() function of AbstractInventoryHopperBlockEntity. There is code that detects the direction of the hopper for switching between input and output mode, which runs once a second on the server. It appears to run a breadth first search on all inventory cables until it finds the inventory connector or it hits the configured max cable limit. While this can be a source of lag, it doesn't match the observed behavior related to having/not having an inventory hopper filter set.
Further down in the updateServer() function, we see a call to the update() function, which is implemented in BasicInventoryHopperBlockEntity. There are two main sections in the function, separated by an empty line. The first half does the following:
- Updates an internal filter predicate function. Not the root cause of lag, as using regular items as the filter should make
setFilter()complete in constant-time. A nbt tag is not passed when creating theItemVariant, but I'm not sure if this means the predicate will match all items of that type or only items of that type with an empty nbt tag. - Early return if the hopper is in output mode and does not have a filter set. This matches the observed behavior relating to increased mspt, if the code below is the cause of the lag.
- Ticks the hopper cooldown, and returns early if the cooldown has not reached zero yet.
- Early return if the hopper is not enabled. This appears to allow for disabling the hopper via redstone. Enabled status seems to be managed by
BasicInventoryHopperBlock.
The second section enters a fabric api item transfer Transaction before calling StorageUtil.move() from the fabric api to transfer at most one item that matches the filter. The javadoc for StorageUtil.move() says.
Move resources between two storages, matching the passed filter, and return the amount that was successfully transferred.
If the move is successful (moved items > 0), the storage transaction is committed and the hopper cooldown is set to 10. If the move fails, the hopper cooldown is set to 5. Due to how the cooldown logic is implemented, the actual cooldown appears to be 11 and 6 ticks, respectively.
StorageUtil's documentation for the class also states the following:
Note that the functions that take a predicate iterate over the entire inventory in the worst case. If the resource is known, there will generally be a more performance efficient way.
Looking at the implementation for StorageUtil.move(), we see that it loops over all "nonEmptyViews" of the inventory connector. StorageView<T> is "the view of a single stored resource in a Storage", so at best looping once over each unique item (type/nbt) would be required. There is an early return if the maximum amount of items is moved, but no early return if the destination container is completely full (and so all of the move attempts fail).
InventoryConnectorBlockEntity.getInventory() is called by AbstractInventoryHopperBlockEntity when the inventory connector of a network is found during the once-per-second scan. This returns an instance of MergedStorage, which implements Storage<ItemVariant> and is the source that is passed to StorageUtil.move().
Fabric API's Storage<T> has a default nonEmptyViews() implementation, which gets the iterator() and filters out views that have no items. MergedStorage's implementation of iterator() returns a CombinedIterator instance.
CombinedIterator simply loops over each StorageView<ItemVariant> provided by each storage block attached to the storage connector and returns those. This means that for every Basic Inventory Hopper meeting the conditions listed previously, StorageUtil.move() will loop over every non-empty slot of every container in the storage network until a matching item is found or all the slots are checked, every 6 or 11 game ticks.
In a worst case scenario (no matching item in network or destination container full), mspt scales linearly with non-empty slots in a storage network (empty slots likely have some impact, just less) and linearly with the number of inventory hoppers pulling from the network. This can easily occur with normal mod usage.
There also doesn't appear to be a cap on the number of tags in a tag filter and the number of items in a polyfilter seems infinitely expandable by attaching another inventory connector to a poly filter, which don't fall under normal mod usage but would multiply mspt impact based on number of tags/number of unique items,