[Feature]: Performance Improvements
Opened this issue · 6 comments
What is the new feature or improvement?
Curios in 1.20.1 has a huge performance cost due to 3 reasons:
- Retrieving capability from entities (already addressed by the Attachment system)
- All mods are iterating over all slots
- Some methods are called far too frequently
My suggestion:
- EnderMask could be called probably hundreds of times per tick, when player is in the end. It could be causing a lot of lag. One way to address it is to split ender mask method into 2 methods:
isEnderMask()with result{YES, DEPENDS, NO}andboolean isEnderMask(ItemStack, LivingEntity, EnderMan)if it returnsDEPENDS. This allows curios to cache the value if nobody returnsDEPENDSand make this part 100+ times faster. - A lot of mods are just trying to query
boolean contains(Item)orboolean contains(TagKey<Item>). These 2 use cases can be cached to reduce slot iterations. Queries can be cached and invalidated when curios changed on player (or rebuild cache per tick, which is enough already). - For use cases such as
Optional<ItemStack> findFirst(Item), instead of iterating through all slots, it could be faster if it's just iterating over slots that could contain the item, by checking which curios tags the item has (pluscuriosslot). I tried it in my 1.20.1 mods, and it reduced mspt of my mod by 50%, with the remaining 50% coming from capability fetch and ResourceLocation validation. I believe those can be addressed in 1.21 as well. - If suggestion 2 and 3 were to be implemented, I would recommend to deprecate previous methods to encourage mod devs to use the better performing methods.
Curios is currently consuming 5%~8% of game mspt in large adventure oriented modpacks, being the top 5 of resource intensive mods in modpack of 200+ mods, higher than AlexMobs and IceAndFire. These performance costs are primarily induced by other mods constantly fetching and checking curios, so it’s hard to test without a mature large modpack. This improvement could help a lot to improve performance.
These changes were discussed with C4 and reached an agreement that it’s too much of a breaking change for 1.20.1 and better to leave it for next Minecraft version. Now this is a chance to make Curios performing better.
I had some time to look into this. Adding a cache to endermask calculation was simple and indeed saves hundreds of lookups per tick. It is now 1 lookup per player per game tick where it was per enderman too before.
For 2 and 3 it looks like all the lookups end up in CurioInventoryCapability.findFirstCurio so I added a cache there as well. I can't change the method signature because the context needed to reduce the lookup based on which slots the item can be in is not present. Specifically contains is called by vanilla in Inventory.contains.
I could look up the tags of the itemstack passed in and I might do that in the future to limit the slot checks, but I won't be adding a way for the list of slots to be passed to the method.
The commit is here if you want to have a look 66c15bd
I’m not sure hash of a predicate would consistently reflect the internal nature of the predicate. Is it a good idea?
I know caching item / tag based query will work, but I’m not certain about predicates as most of the case it’s lambda
did this instead, that should be a safe cache key for the two methods
Thanks. I think it works. What about item?
I find that the actual entry point for curio search is here: https://github.com/JDKDigital/Adorned/blob/dev-1.21.0/neoforge/src/main/java/top/theillusivec4/curios/common/capability/CurioInventoryCapability.java
Could findCurios(Item) and findFirstCurio(Item) be optimized as well?