search

progwml6 / ironchest

Iron Chest minecraft mod by CPW
https://minecraft.curseforge.com/projects/iron-chests
GNU General Public License v3.0
135 stars 86 forks source link

Potential or partial solution to #179: remove ItemStack::copy. #192

Closed noobanidus closed 5 years ago

noobanidus commented 5 years ago

From my own profiling (which unfortunately I don't have a copy of) of a situation which involved a single chicken on a hopper attached to a diamond chest, I noticed that there was a significant slowdown related to IronChests's item handler.

Specifically (and perhaps exacerbated by the hopper's functionality) every time an item was "inserted", capability events would be fired and responded to by a number of mods, resulting in slowdown. This was compounded as it happened continuously every time it tried to insert the egg for every single slot.

This seems to be caused by the ItemStack copy in both of the main inventory handlers.

Looking at their Vanilla equivalents, there doesn't appear to be any canonical reason for this (as Vanilla does not copy), and it appears as though it is to be expected that insertItem will modify your itemstack in some way.

HenryLoenwind commented 5 years ago

it is to be expected that insertItem will modify your itemstack

image

Just FYI, as I stumbled upon this commit randomly.

PS: After having a second look, https://github.com/progwml6/ironchest/blob/1.12/src/main/java/cpw/mods/ironchest/common/lib/ICChestInventoryHandler.java#L59 etc. now violates that contract. Especially https://github.com/progwml6/ironchest/blob/1.12/src/main/java/cpw/mods/ironchest/common/lib/ICChestInventoryHandler.java#L65 is realyy bad, as it modifies the parameter stack in simulation mode, effectively voiding items in the source inventory.

noobanidus commented 5 years ago

I confess to mainly focusing on the slow-down caused by the item copying. I didn't extensively examine the rest of the code to see what modifications were made, although I presumed someone with more knowledge of the situation would hopefully review the PR first.

Honestly, I'm not sure why ItemStackHandler isn't being used in this situation instead of retaining the functionality of IInventory (which is what it seems like).

Alternatively, InvWrapper could either be used or used as an example.

HenryLoenwind commented 5 years ago

If I had to guess about the why, I'd say it's because Forge's "example" code is known to usually be on the bad and/or incomplete side. e.g. that insert-stacked helper method looks good but ignores so many edge cases one's surprised it even inserts something most of the time. ;/ But, tbh, that's typical for "clean room" non-battle-hardened code...