[1.12.2] Rat inventory interaction performance issues with blacklist/whitelist upgrades
noobanidus opened this issue ยท 1 comments
Hiya! I was attempting to help Joel and Genny locate some sources of lag on their server for Genny's latest modpack. While I did have access to samples, I didn't have direct access to the server, so I'm afraid I'm not able to provide quite as much data to back up the following as I can.
(Also a disclaimer that I wrote this while relatively sleep deprived. If something makes absolutely no sense, it's entirely likely that I've made a complete mistake, mistyped something or just straight up written something different from what I meant to write.)
Due to the nature of how the contents of the blacklist and whitelist are stored (as NBT, similarly to a shulker box), and the fact that these values aren't cached, EntityRat::canRatPickup
can cause a small amount of deadlock on servers with a large quantity of other mods.
Specifically, as ItemStackHelper.loadAllItems
is just short-hand for deserializing all of the items contained in the NBTCompoundTag
into the NonNullList
, this results in the creation of new ItemStack
s every time canRatPickup
is called. In packs with large numbers of mods, ItemStack
creation can have a not insignificant overhead in the form of capabilities -- as each capability handler for Capability<ItemStack>
is fired.
This itself wouldn't generally be a problem, but unfortunately the RatAIPickupFromInventory
task consults RatUtils::getItemSlotFromHandler
every time it's updated. I'm uncertain of how frequently this is, but I believe it has the potential to be every tick.
In turn, RatUtils::getItemSlotFromHandler
checks EntityRat::canRatPickupItem
for every slot in the inventory the rat is currently attached to. In some cases, specifically on Genny's server, this can be upwards of 300 slots per AI task update (potentially every tick).
While this is the only sample I still have a screenshot of, and it was of a very short period of time, you can see that a significant portion of the entity update time is spent on the inventory-related tasks of the rat:
In this case also, you can see that the overall cost of creating the ItemStack
(on average) is around 1% of server tick time. That's not necessarily significant, but on Genny's server I believe this was specifically just one rat attached to an RFTools Storage device with a large number of slots.
You can also see the capability attachments: thankfully, in Genny's pack, there are only two Capability<ItemStack>
handlers to fire. In a previous 1.12.2 pack that I was running for Arcane Archives, there were upwards of 15 handlers registered to that -- and each of them were fired for every ItemStack
created, regardless of whether or not the stack even met their criteria (that is, because the event itself is supposed to determine that fact).
Suggestions for improving the situation:
- Adjusting
RatUtils::getItemSlotFromItemHandler
at around line 111 to add a clause similar to:
if (stack.isEmpty()) {
continue;
}
This would prevent EntityRat::canRatPickupItem(stack)
from ever being fired in the first place, reducing a considerable amount of overhead when the inventories just contain a vast number of empty stacks.
- The actual process of using
IItemHandler::extractItem
is obviously the most desired way of handling inventory interactions. However, this too actually returns a copy of the item stored; if you aren't modifying the item (as is clear in this case), you can bypass some of this (extremely minor, but it all adds up) overhead with something similar to:
ItemStack stack = handler.getStackInSlot(i);
- Modifying
EntityRat::canRatPickupItem
with a similar clause at the start of the function to handle empty stacks: there's no point loading the blacklist or the whitelist. Something similar to the first
(admittedly extremely simple) example I provided (except returning false).
As I don't full understand how your (I believe modular) upgrade system functions, I can't give an exact example of how it could be improved specifically. However, I would definitely suggest that you adopt something similar:
- Deprecate the method of storing blacklists/whites during run-time as NBT (or as serialized via
ItemStackHelper
). - Instead, have each blacklist/whitelist type store an
ItemStackHandler
(perhaps as a map against the ItemType). Return this whenever consulted. - Serialize each of these into the entity's NBT whenever it's unloaded and vice versa when unloaded.
So long as the rat remains loaded, this will hopefully reduce the amount of times that a serialisation to/from NBT will occur to around once when the chunk loads (that is, in Big-O notation, O(1), or more accurate O(n) where n is the number of times the chunk loads/unloads), as opposed to what it currently stands at, which I believe would be represented as O(su) (where s is the number of slots and u is the number of times the AI task is updated a second). My Big-O notation is something I haven't done a refresher on in ages, so it's possible I'm over/understating things.
I hope this information is helpful! Hopefully Joel or Genny can add any further observations they've had.
Specifically after putting the rats that were doing inventory-based operations into nets and then restarting the server, they appeared to have had a significant improvement on TPS.
Obviously, replacing the rats into the positions they were previously in and seeing if there is degradation to the same degree that they were previously experiencing on the server would move closer towards confirming this as their specific issue.
Regardless, the numbers seem to support that there is an issue with the current blacklist/whitelist storage and serialization/deserialization, although I did forget to ask Joel for for a longer profile to get a better picture of what else was also happening on their server.
Cheers!