Adorned (Curios)

Adorned (Curios)

93 Downloads

[Feature]: Performance Improvements

lcy0x1 opened this issue · 6 comments

commented

What is the new feature or improvement?

Curios in 1.20.1 has a huge performance cost due to 3 reasons:

  1. Retrieving capability from entities (already addressed by the Attachment system)
  2. All mods are iterating over all slots
  3. Some methods are called far too frequently

My suggestion:

  1. 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} and boolean isEnderMask(ItemStack, LivingEntity, EnderMan) if it returns DEPENDS. This allows curios to cache the value if nobody returns DEPENDS and make this part 100+ times faster.
  2. A lot of mods are just trying to query boolean contains(Item) or boolean 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).
  3. 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 (plus curios slot). 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.
  4. 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.

commented

Thanks. I think it works. What about item?

commented

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

commented

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

commented

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?

commented

Aye, findFirstCurio(Item) should have had it already but I apparently forgot to add it. I added the same type of cache to findCurios

0515357