Memory leak in `Godot.Collections.Array<T>`

[markdibarry]

Godot version

4.1.0-dev

System information

Windows 10

Issue description

When a circular reference is needed between two Resources, if one is in a Godot.Collections.Array<T> the object will not be set for garbage collection unless all references are manually set to null. This may also occur with other Godot objects and not just Resources, but I haven't done extensive testing nor know the root cause.

Steps to reproduce

Run MRP project somewhere where you have memory profiling like Visual Studio.

Press "ui_select" (Spacebar) a bunch of times and then "ui_cancel" (Esc) to force GC. Take a memory snapshot and note that none of the objects created that have now gone out of scope were freed.

Press "ui_accept" (Enter) a bunch of times and then "ui_cancel" (Esc) again to force GC. Take another memory snapshot and note that these new objects were collected after manually setting all references to null.

Feel free to try other scenarios like switching the Godot.Collections.Array to a System.Collections.List, and the GC will work correctly.

Minimal reproduction project

ResourceLeakTest.zip

[RedworkDE]

This is a fundamental limitation of using reference counts for memory management (as the godot core does).

To be able to delete these objects, the cycle must be broken at some point, either by using Godot.WeakRef, if the cycle needs to remain accessible outside of C# or by going through a managed only object to hide that part of the cycle from native code.

[AThousandShips]

Here's some further reading on it, as well as the documentation

[markdibarry]

@RedworkDE @AThousandShips It isn't clear from your comments. Are you saying this is a problem with Godot Collections memory management that needs fixed, or is this expected and I need to wrap all reference counted Godot objects in WeakRefs? It's not clear why a circular reference between two Resources wouldn't be a problem, but putting one inside a Godot Collection would prevent garbage collection.

[AThousandShips]

It is an inherent issue with references and circular references are to be avoided, as mentioned on WeakRef

Without weakrefs, using these classes could lead to memory leaks, since both references keep each other from being released. Making part of the variables a weakref can prevent this cyclic dependency, and allows the references to be released.

[markdibarry]

Right. That implies that this problem would occur with any circular reference, but that isn't the case. I have tons of ref counted Godot objects that have circular references and there's no GC problems. It's only when inside a Godot Collection that the collection is prevented. Why do you need a WeakRef only when using collections, and more specifically Godot Collections, since the issue doesn't happen with System Collections?

I'd avoid using Resource and Godot.Collection both for the overhead alone (I have tens of thousands of these objects), but there's no other way to be able to edit them in the editor.

[AThousandShips]

This leaks always at least in GDScript if you have circular references, it might be that this breaks in collections specifically in C# but the issue still stands with objects interacting with the engine

Why can't you use weak references?

[markdibarry]

Why can't you use weak references?

I can. It's always been an option, but I haven't up until now for the same reason I don't use the System.WeakReference class for all normal C# circular references: because the GC can (and is designed to be able to) handle them, so there's no need to.

I'm not that familiar with GDScript, so I can't speak on that language, but it sounds like from what you describe that the behavior is consistent for GDScript if all circular referenced RefCounted objects cause memory leaks. The behavior in C#, however, is different, inconsistent, undocumented, and (from what it sounds like) not fully understood, considering (until now) it was assumed that all circular referenced RefCounted objects cause memory leaks on their own (the opposite is observable in the MRP).

What I'd like is to know what scenarios cause the GC to fail with RefCounted objects, so we can either fix the documentation to let users know when to use a WeakRef or handle the issue so a WeakRef isn't needed.

[AThousandShips]

My bad I was not aware that circular references were not an issue in general C#, this should be added to the documentation about C#

But (and I'm not familiar in depth with C# in Godot so I'm not sure) but the types that interface with Godot are likely to be restricted by the restrictions of C++ as the data has to be accessible to it, as I am assuming types integrated with C++ have to be available for the engine to use

[markdibarry]

No worries! We're both trying to understand the behavior. It just took me weeks of trying to track down these memory leaks, and if some extra documentation or a potential bug-fix can save someone else the same trouble, it's worth it to me.

[AThousandShips]

Tbh i think RefCounted (and possibly Resource) in the documentation should have a mention of reference cycles as you have to look at WeakRef to be informed about this which is a bit of a pitfal for new users

[RedworkDE]

Right. That implies that this problem would occur with any circular reference, but that isn't the case. I have tons of ref counted Godot objects that have circular references and there's no GC problems. It's only when inside a Godot Collection that the collection is prevented. Why do you need a WeakRef only when using collections, and more specifically Godot Collections, since the issue doesn't happen with System Collections?

Because of how exactly the C# bindings work and deal with ref counts, if only managed references are involved, proper cycles do not happen and the GC can collect things normally. But if a native reference to a managed script exists that script can no longer be collected and thus every native and managed object it references is kept alive as well.

Basically as long as everything is managed, this happens to just work; if not care has to be taken.

[markdibarry]

Hmm I wonder if there's a good way to sum that up in the docs.

[dandeliondino]

Thank you all for documenting this behavior. I believe you've just saved me a lot of time troubleshooting.

I'm not new to Godot, but I am new to manual memory management. I had assumed that RefCounted objects stored in Dictionaries and Arrays would be cleaned up when the reference to their container was lost. If I'm understanding this issue correctly, this is not the case, and is also likely the cause of the progressive slowdown/memory leak in my current project (an EditorPlugin in GDScript / Godot 4.0.3).

I apologize if this is not the best place to ask this question, but this is the first and only time I've seen this behavior documented:

What is the best practice in GDScript when the only persistent reference to the RefCounted objects is in an Array or Dictionary?

My plugin creates hundreds to thousands of RefCounted objects every time it performs a calculation and stores them in Dictionaries. I realize that using objects this way is inherently slow in GDScript, but performance has been acceptable. At least until these functions have been called a dozen or so times and the editor's memory noticeably increases.

Besides the dictionaries, I only have temporary local variables referencing the RefCounted objects at the time of creation, and when they are retrieved. So I don't think using Weakref to store the objects in the dictionaries is an option. It will mean their counter will be at 0 at a time when I still need the objects in memory.

In cases where the dictionaries remain under my control, I can just change the objects' classes to Object and manually free them when no longer needed.

But there are cases where they aren't under my control -- like dictionaries of Refcounted objects passed as bound arguments to the EditorUndoRedoManager. Is the best option to refactor to avoid objects entirely in this case? I am assuming nested Dictionaries (which would be the alternative) will be cleaned up when their reference is lost?

[dandeliondino]

I apologize -- I believe I misunderstood this issue. Perhaps it's only applicable to C# or I misinterpreted the cause. I'm posting again to correct my prior comment so someone does not find it and be misled.

More testing showed that RefCounted objects inside Dictionaries in GDScript behave as I'd previously thought. When their containing Dictionary is no longer in scope, their reference counter goes down, they get freed, and memory use returns to the prior level. I confirmed in 4.0.3 and 4.1-beta-3, both with an MRP and with the RefCounted objects in my actual project. I don't know what's causing the progressive slowdown and memory increase while my plugin is running in the editor, but I don't think I can blame it on these objects.

Here is a screenshot of some of the tests I did. By adding breakpoint statements, I confirmed system memory use went up from 50MB to 175MB, then down to 100MB, and, if I cleared the weakref dict (or did not populate it), memory returned to around the original 50MB on return to the _ready() function. local_dict.clear() made no difference in the results. There was also no progressive increase in memory use with subsequent calls. There seems to be no sign of a memory leak.