SHsuperCM / CITResewn

Fabric implementation of mcpatcher's cit
MIT License
154 stars 76 forks source link

Improve performance of NBT String parsing #311

Closed wahfl2 closed 10 months ago

wahfl2 commented 11 months ago

Currently, performance with many string-dependent CIT items being rendered is pretty much unplayable on any system.

I have a Ryzen 7 3700x and an RTX 3060ti, and this is my performance in the Hypixel Skyblock lobby with a CIT-dependent resource pack. The cache invalidation time is at the default 50ms. 2023-08-29_18 40 35

With these changes: 2023-08-29_18 49 38

The performance could be improved more by implementing a less naïve cache system, but this fixes a few fatal performance flaws.

SHsuperCM commented 10 months ago

Although I fully agree that this is a very serious issue that will be looked into, this PR has several issues which stop it from being merged on its own.

 

https://github.com/SHsuperCM/CITResewn/blob/a55d205fb9f38bfdf1c21620db7694dcdad63c8e/defaults/src/main/java/shcm/shsupercm/fabric/citresewn/defaults/cit/conditions/ConditionNBT.java#L3-L7

 

https://github.com/SHsuperCM/CITResewn/blob/a55d205fb9f38bfdf1c21620db7694dcdad63c8e/defaults/src/main/java/shcm/shsupercm/fabric/citresewn/defaults/cit/conditions/ConditionNBT.java#L131-L144

I will look into optimizing the string matching code as soon as I'm able to. Thank you for bringing this to my attention.

wahfl2 commented 10 months ago

My bad, that whole thing was pretty much a micro-optimization. You can still improve the performance by about 2000% in the case above by using Text.Serializer.fromLenientJson() instead of letting it throw an error and proceeding to ignore it.