P1X-in / tanks-of-freedom-ii

Indie Turn Based Strategy in Isometric Voxel Art http://tof.p1x.in
Other
194 stars 19 forks source link

Opening and closing project cause memory leak #11

Open qarmin opened 2 years ago

qarmin commented 2 years ago

Opening and closing project with

godot -q -v

shows this memory leak

Leaked instance: Reference:31190
Leaked instance: Reference:35321
Leaked instance: Reference:3636
Hint: Leaked instances typically happen when nodes are removed from the scene tree (with `remove_child()`) but not freed (with `free()` or `queue_free()`).
ERROR: Resources still in use at exit (run with --verbose for details).
   at: clear (core/resource.cpp:417)
Resource still in use: res://scenes/map/tile.gd (GDScript)
Resource still in use: res://scenes/map/tile_fragment.gd (GDScript)
Orphan StringName: res://scenes/map/tile_fragment.gd
Orphan StringName: has_friendly_unit
Orphan StringName: is_neighbour
Orphan StringName: NORTH
Orphan StringName: can_pass_through

I'm not sure what is cause of it

czlowiekimadlo commented 2 years ago

I have to try using that option more often. The game does indeed appear to be leaking objects somewhere. I suspect it is the AI, although I do not have a solid proof, nor any idea where it could do so.

czlowiekimadlo commented 5 months ago

I have tried to reproduce this, but the results are not very satisfying. Running the editor itself with options mentioned did not result in any indication of memory leaks. Running an exported game with these options does indeed report some resources being still in use at exit (just like in the report), but this is not followed by the actual list of resources held.

As to what may be causing it, I might have an idea. It seems that, if an object has a signal attached to another object outside of it's scene (for example to a global singleton handling mouse clicks or online communication), this scene might not get garbage collected when it's no longer needed, and potentially persist through exit.

dfgworm commented 2 months ago

if an object has a signal attached to another object outside of it's scene

I do not think that can be the case. I have just tested it, and signal connections do not seem to count as references, at least for the purpose of RefCounted. Connected object simply gets destroyed and connection disconnected.

this scene might not get garbage collected when it's no longer needed

As for scenes, Nodes in godot do not get garbage collected at all. If you lose references to a node, it will persist in memory, causing a memory leak. That is called an 'Orphan node'. Nodes must be destroyed using free() or queue_free()

czlowiekimadlo commented 2 months ago

That is called an 'Orphan node'. Nodes must be destroyed using free() or queue_free()

I have fixed orphan nodes some time ago, checked with in-editor debugger and it didn't show any of these anymore. I don't know how the memory management works, but it is leaking somewhere. All you have to do is start a match and then keep hitting Restart mission button from ESC menu to grow the usage. It might be connected to the OP, or might not.

dfgworm commented 2 months ago

In that case it's probably not related to nodes at all, since freeing the node forcefully dereferences it throughout the project. You also might have a memory leak if you specifically extend 'Object' class, which also does not get reference counted.

You might want to try forcefully freeing some other objects as well, like resources, instead of just dereferencing them. You shouldn't have to do that, but it might help you locate the issue. EDIT: Nope, apparently you can't free RefCounted.

You can also try freeing important singletons one by one to see if that frees the leaked memory (You might need to leak a noticeable amount of it first). Might try literally calling get_tree().root.get_child(0).queue_free(), though you might have to get around a bunch of errors if you do it in editor. That won't solve the problem of course, but at least narrow it down.

If you free every single node in your tree and the leak still remains, then problem must lie outside the tree, which leaves godot servers, like DisplayServer and PhysicsServer3D. You can check ones that you use in your project, for example if you do any machinations directly with shapes in the physics server, those might be the leak. Such objects are usually handled through RIDs. Maybe there is some generic way of getting a list of used RIDs, i do not know.

It's also possible that leak is not your fault at all, and is caused by some complex feature you are using. like generating textures through subviewports or something. If nothing comes to mind, the most generic approach would be to try disabling large chunks of functionality one by one. For example not creating any entities or not generating any tiles.

czlowiekimadlo commented 2 months ago

Thats a lot of useful tips! I'll try that out when I have some time. From the list of things you mentioned I only use subviewport textures, and even that is very sparsely used.

Overall this is not that big of an issue because the memory footprint of the game is low. Every reload adds dozens of megabytes, so a player would either have to play a LOT or savescum to even notice there is a leak.

dfgworm commented 2 months ago

I just found out that GDScript does not handle cyclic RefCounted references

This code immediately shows a huge memory leak:

class A:
    var b:B

class B:
    var a:A

func _process(delta: float) -> void:
    for i in 1000:
        var a = A.new()
        var b = B.new()
        a.b = b
        b.a = a

Which is quite shocking to me. Apparently this is not even considered a bug, you are supposed to somehow use WeakRef to avoid such cyclic references.

This fact complicates things a lot. Freeing singletons will not actually free such a leak. Though disabling chunks of functionality to narrow down the problem is still a viable method.

Also using nodes instead of RefCounted makes situation easier, since they will be freed anyway. You might even consider inheriting Object directly and freeing it. It's more responsibility, but at least it gives guarantees.