Fix Memory Leak of Air Capability Listeners

[BlueAgent]

The memory leak was caused by registering new listeners to the same instance of a capability. The fix in this PR is to cache the listener delegates instead of creating new instances of them, relying on the listeners internally being stored in a set.

I think @Saereth might have already mentioned this to you. Feel free to close this PR or incorperate the code into the WIP fix if there's something useful. Tested this on our community server and it seems to work (stays at about a constant 3.1k instances of the lambda). Let me know if this should instead be retargeted to the 1.20.1 branch and then backported instead of forwardporting.

Some additional things changed that are not quite related to the above:

• Fixed air block entities not invalidating air capabilities. (Might have missed some)
• Changed the VacuumModule cached air capability to use an empty optional instead of null.

Heap Dump Summary

Screenshot of Above


[desht]

Thanks for that! There were definitely... issues there.

This is fine to be targeted at 1.19.2; I can handle cherry-picking it to 1.20.1.

[BlueAgent]

No problem.

Something extra that we could change is to check that the current LazyOptional is the same as the l in the invalidation listener before resetting. Sadly there is no method to unregister invalidation listeners on LazyOptional, so anything captured by the invalidation listener will be kept alive as long as the LazyOptional, even if it's not valid anymore. I guess that means we could also check if the BlockEntity that registered the listener is still valid too, but this is probably not as crucial.

[BlueAgent]

While checking out the code again to implement checking that the listener being invalidated is the same one as current, noticed I missed the RegulatorModule so implemented caching the invalidation listener for that as well.

But now there's kind of duplicated code between RegulatorModule and VacuumModule so perhaps we should move the getCachedNeighbourAirHandler method to a super class? Although a module might want to get a different direction so just leaving it for now.

[BlueAgent]

So I missed resetting to empty cap when it is not present so added that in. Also split the code into two methods now as well (one to get the current and one to get the cached), let me know if it's preferable to keep it as one method and I can revert it (or you can as well as maintainers have push access to the PR branch I think).

[desht]

Thanks again. I'll be reviewing these changes in detail tomorrow, with luck, although all looks good so far.

[desht]

But now there's kind of duplicated code between RegulatorModule and VacuumModule so perhaps we should move the getCachedNeighbourAirHandler method to a super class

This isn't ideal, but moving it to a superclass is tricky without having to move the fields it uses as well (neighbourCap and neighbourCapInvalidationListener). And I'd rather not have those fields in the superclass where they're only used by certain subclasses.

One option might be a NeighbourAirHandler interface, with default implementations for those two methods, and methods for the capability getter & setter and the invalidation listener getter which the implementing class provides trivial implementations for? E.g.

default LazyOptional<IAirHandlerMachine> getCurrentNeighbourAirHandler(PressureTubeBlockEntity pressureTube, Direction dir) {
   // implementation as before, but use methods below
}

default LazyOptional<IAirHandlerMachine> getCachedNeighbourAirHandler(PressureTubeBlockEntity pressureTube, Direction dir) {
   // implementation as before, but use methods below
}

LazyOptional<IAirHandlerMachine> getNeighbourCap();

void setNeighbourCap(LazyOptional<IAirHandlerMachine> cap);

NonNullConsumer<LazyOptional<IAirHandlerMachine>> getNeighbourCapInvalidationListener();

I've pushed a change with this, feedback welcome :)

[BlueAgent]

Using an interface instead of implementing it in a super class is definitely better. It does expose some methods that should ideally be private, through, it's internal to the mod so probably fine. It also makes it challenging if a module should extend from another and want to cache another direction in the future.

Inspired by the interface, noticed an opportunity to create a generic capability cache. Hopefully it's useable for other capabilities too in the future, let me know what you think.

This has the added benefit of the invalidation listener no longer preventing GC of the BlockEntity since the listener lambda no longer captures it.

Now, the naming perhaps isn't the best, e.g. CapabilityCache.get and CapabilityCache.getNeighbour might be a bit confusing, so feel free to rename stuff to be better.

[BlueAgent]

Fixed creating a lambda on every get operation for the cache. It's a lot cleaner now and got rid of getNeighbour directly on the capability cache.

[desht]

Inspired by the interface, noticed an opportunity to create a generic capability cache. Hopefully it's useable for other capabilities too in the future, let me know what you think.

Yeah, it had occurred to me this could be generalised further, so thanks for developing the concept :)

Having looked over it and played around with it in a world, it all looks good at this point, so I'm about ready to merge it, unless you had anything else planned?

[BlueAgent]

Didn't think of any more changes, so sounds good to merge when you're ready to. Thanks for reviewing and testing this PR.