Update lastCachedStamp on cache hit

[NickNackGus]

By not invalidating cached CIT values that were recently used, FPS is dramatically improved, particularly in areas with many armor stands.

[Combustible]

Please consider merging this PR - we are using a resource pack with more than a thousand items in it, and one particular room on our server with ~10 armor stands gives me 15 FPS without this patch and 250 FPS with it.

[SHsuperCM]

This PR is very likely to be merged, I've briefly looked over it and it is definitely a good idea that'll most likely be merged in the next version.

However, my main goal right now with CIT Resewn it to make sure these fixes are applied for every mc version and not just 1.20. So I am not changing anything until it is properly multiversioned which is what I am doing currently.

[NickNackGus]

However, my main goal right now with CIT Resewn it to make sure these fixes are applied for every mc version and not just 1.20. So I am not changing anything until it is properly multiversioned which is what I am doing currently.

I'll be interested to see how you implement this. I've got a simple mod that I'd like to support multiple versions, but I'm not sure how to do so and maintain it with how Fabric patches the Minecraft code.

[SHsuperCM]

If you're interested, take a look at recent commits starting from ab4fcee as well as following my other project SHsuperCM/Stonecutter

[NickNackGus]

Neat - so that commit adds the infrastructure to add new versions using placeholders, and this commit adds 1.19 support into the mix? https://github.com/SHsuperCM/CITResewn/commit/4c04ceb7ee4e9b532f3677c1424433e0e3955a08

[SHsuperCM]

Essentially, yes. Currently I am also working on some regex stuff to ease the 1.19.2-4 port with stonecutter.

[Dev0Louis]

Is their progress on this?

[SHsuperCM]

I'm trying to understand this PR right now, is this not essentially just disabling cache invalidation, essentially the same as raising the config for cache time to infinity? How is this change supposed to handle external changes that dont invalidate the cache?

[NickNackGus]

Actually, it doesn't set the cache time to infinity - any textures that aren't in use get successfully discarded, including discarding textures when reloading resource packs and changes made to the items/mobs. The difference is, if a texture is being rendered before the cache expires, then the time until the texture is discarded is refreshed.

If you were to start up your mod without this patch in an item-heavy area, especially with a lot of armor stands, the frame rate can plummet into the single digits or less. Increasing the cache duration causes the game to play at a reasonable FPS for the duration of the cache, and then have a single frame that takes an excessively long time, repeating once every cache duration.

For best results, the minimum cache time in the config needs to be increased to two frames, preferably more, when this gets merged - doing so completely eliminates the issue. I can record a comparison for reference.

[SHsuperCM]

Not the cache time, the cache invalidation time to be pedantic. Doing some tests with your PR, it appears that the cache is just never invalidated when it should do so periodically. The only time the cit gets rebuilt for an item is when something fundamentally changes in the item nbt. And it makes sense that it would do that with the PR as every time you check the cache you are resetting the need to refresh the cache, which is the same as setting the cache invalidation time to infinity. You are just counting to 10 but every time you say the next number you reset back to 0, making 10 never appear(unless there is a pre-existing lag spike preventing you from counting up).

[NickNackGus]

Hopefully showing what's happening in video form works better - if not, I'll reply in the morning :) https://www.youtube.com/watch?v=uEHt29gzgtI

[SHsuperCM]

...

[NickNackGus]

Wow. That was not the video I thought I had copied. It's 3 AM here and I'm not operating at my finest. This is the video I meant to attach. https://www.youtube.com/watch?v=UxpyW9AB1z4

[NickNackGus]

The idea being that the cache only evicts entries if there hasn't been a cache hit in a reasonable amount of time. So in that sense, yes the cache entries can be held indefinitely.

[SHsuperCM]

Barring the video link error, I am well aware that it makes performance better to not invalidate the cache. However it still fundamentally breaks intended behavior. Only thing I'm taking away from this that could potentially help is finding a way to stagger the cache refreshes to happen across multiple frames instead of all in one frame.

[NickNackGus]

I'm sorry for the confusion from the other night. I should have waited to reply until I was well rested and ready to think through my response properly.

Fundamentally, the reason the cache exists is that loading the data without one takes orders of magnitude more time. I agree that storing data in the cache indefinitely would be problematic. However, I find the time-based First In, First Out cache eviction policy is inappropriate for this use case, as textures that were used on one frame are very likely to be used on the next frame.

This PR changes the eviction policy to a Least Recently Used eviction policy. Textures that have been used recently are retained until they have not been used for a certain period of time, after which they are evicted, and are not loaded again until they are next required. Evicting textures from the cache a certain amount of time since they were last used significantly reduces the number of cache misses over evicting them based on the time they were added to the cache, which improves FPS.

I understand your concerns about failing to evict data that is out of date. However, my testing did not find such a case. Enabling, disabling, or reloading the resource packs cause the old data to be evicted properly, and modifications to the items or entities cause immediate updates to the textures used. If you have further concerns that I may have missed, I am happy to address them.

[SHsuperCM]

I can tell you that there are going to be conditions that dont rely on the item fundamentally changing to yield different results. Having the cache states stay as is without being verified again at the very least every so often is not something I am willing to trade off for extra performance.

After fixing a major issue and making the refreshes staggered this really doesnt matter in most cases, only thing to trigger this push for performance in v1.1.4 was a pack of 4k CITs I saw which is not the normal use case, and even then it was not that bad on my pc and was not felt at all when raising the invalidation time. Sometimes slower pcs just need to lower the graphics settings yknow?