[1.20.1] Do not add duplicate invalidation listeners while caching machine neighbor capabilities #682

Merged

rezonant merged 2 commits from fix/680 into lts/1.20.1

2024-06-13 19:35:35 +00:00

rezonant commented

2024-06-12 07:22:32 +00:00

(Migrated from github.com)

Description

Fixes #680.
Targets 1.20.1 specifically.

Machine block entities add invalidation listeners to discovered neighbor capabilities as they enter the block's capability cache for the purpose of ensuring the machine does not keep a capability around after it is removed from the neighbor block. However machines do not track whether they have already seen a given capability, and add a new invalidation listener each time the neighbor capability cache is rebuilt. Unfortunately this produces a memory leak that can, over time, cause millions of callbacks to be added to the LazyOptional<T> instance for a given capability. Over time the leak will cause the GC to thrash, causing sporadic TPS drops and/or client/server lockups.

Interestingly, Forge does not provide a removeListener() on LazyOptional<T>, meaning that technically speaking every single usage of addListener() across all mods is an unavoidable memory leak assuming the LazyOptional<T> is long lived, which is normal. Additionally, there may be issues with invalidation working properly for some types of capabilities based on my discussions with community members at NeoForge, but a full accounting of those limitations will require an additional investigation.

This fixes the issue by maintaining a WeakHashMap of the LazyOptional<T> instances obtained via the capabilities system, and only attaching an invalidation listener if the capability was not already present in the hash map. The benefits of using WeakHashMap here (even though the value of the hash map is irrelevant) are that there is no "culling" of stale references to manage, as that is handled automatically by the JVM.

Before this change, I was able to cause an -Xmx2G JVM to crash in an hour or so with a capacitor bank setup. Capacitor banks cause lots of cache invalidations as the energy they contain changes due to normal I/O, so the issue is particularly pronounced for them, even though this issue does affect all EnderIO machines. After this change, the same setup remained well under the 2GB limit for 24 hours, suggesting that the memory leak is solved.

It is certainly not ideal that energy updates cause the cache to be invalidated at all and improving that should certainly be investigated, but given that the capabilities system is overhauled in NeoForge 1.20.3 and up and EIO has 1.20.1 as a long term support branch, it seems prudent to fix this with the simplest possible fix and target further improvements (both to caching and to capacitor banks) in the current development branch (1.20.6).

Breaking Changes

None.

Checklist

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you).
(N/A) I have made corresponding changes to the documentation.
(N/A) My changes are ready for review from a contributor.

# Description Fixes #680. Targets 1.20.1 specifically. Machine block entities add invalidation listeners to discovered neighbor capabilities as they enter the block's capability cache for the purpose of ensuring the machine does not keep a capability around after it is removed from the neighbor block. However machines do not track whether they have already seen a given capability, and add a new invalidation listener each time the neighbor capability cache is rebuilt. Unfortunately this produces a memory leak that can, over time, cause millions of callbacks to be added to the `LazyOptional<T>` instance for a given capability. Over time the leak will cause the GC to thrash, causing sporadic TPS drops and/or client/server lockups. Interestingly, Forge does not provide a `removeListener()` on `LazyOptional<T>`, meaning that technically speaking every single usage of `addListener()` across all mods is an unavoidable memory leak assuming the `LazyOptional<T>` is long lived, which is normal. Additionally, there may be issues with invalidation working properly for some types of capabilities based on my discussions with community members at NeoForge, but a full accounting of those limitations will require an additional investigation. This fixes the issue by maintaining a `WeakHashMap` of the `LazyOptional<T>` instances obtained via the capabilities system, and only attaching an invalidation listener if the capability was not already present in the hash map. The benefits of using `WeakHashMap` here (even though the value of the hash map is irrelevant) are that there is no "culling" of stale references to manage, as that is handled automatically by the JVM. Before this change, I was able to cause an `-Xmx2G` JVM to crash in an hour or so with a capacitor bank setup. Capacitor banks cause lots of cache invalidations as the energy they contain changes due to normal I/O, so the issue is particularly pronounced for them, even though this issue does affect all EnderIO machines. After this change, the same setup remained well under the 2GB limit for 24 hours, suggesting that the memory leak is solved. It is certainly not ideal that energy updates cause the cache to be invalidated at all and improving that should certainly be investigated, but given that the capabilities system is overhauled in NeoForge 1.20.3 and up and EIO has 1.20.1 as a long term support branch, it seems prudent to fix this with the simplest possible fix and target further improvements (both to caching and to capacitor banks) in the current development branch (1.20.6). # Breaking Changes None. # Checklist - [x] My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you). - [x] (N/A) I have made corresponding changes to the documentation. - [x] (N/A) My changes are ready for review from a contributor.

Rover656 (Migrated from github.com) approved these changes

2024-06-13 19:35:29 +00:00

Rover656 (Migrated from github.com) left a comment

Thanks for investing the time to investigate this issue thoroughly; I'm happy with how this has been fixed and I think this will be an important test case to check for in EnderIO 7.

I'm going to merge this in for inclusion in the upcoming 6.1 release.

Thanks for investing the time to investigate this issue thoroughly; I'm happy with how this has been fixed and I think this will be an important test case to check for in EnderIO 7. I'm going to merge this in for inclusion in the upcoming 6.1 release.