GDScript resources are not unloaded due to an extra reference

[dalexeev]

Godot version

v4.0.3.stable.official [5222a99], master

System information

Kubuntu 23.04

Issue description

It looks like we have an extra reference to the script somewhere, which prevents it from being freed at the right time.

See also the comment.

CC @adamscott

Steps to reproduce

const RES_PATH = "res://a.gd"
#const RES_PATH = "res://icon.svg"

var i := 0
var wr: WeakRef

func _ready() -> void:
    print("has_cached = ", ResourceLoader.has_cached(RES_PATH))

func _on_timer_timeout() -> void:
    print("===")
    if i % 2:
        print("has_cached = ", ResourceLoader.has_cached(RES_PATH))
        print("wr = ", wr.get_ref())
    else:
        var res = load(RES_PATH)
        wr = weakref(res)
        print("has_cached = ", ResourceLoader.has_cached(RES_PATH))
        print("ref count = ", res.get_reference_count())
        print("wr = ", wr.get_ref())
    i += 1

has_cached = false
===
has_cached = true
ref count = 2
wr = <GDScript#-9223372010413882237>
===
has_cached = true
wr = <GDScript#-9223372010413882237>
===
has_cached = true
ref count = 2
wr = <GDScript#-9223372010413882237>
===
has_cached = true
wr = <GDScript#-9223372010413882237>

has_cached = false
===
has_cached = true
ref count = 1
wr = <CompressedTexture2D#-9223372010380327805>
===
has_cached = false
wr = <null>
===
has_cached = true
ref count = 1
wr = <CompressedTexture2D#-9223372010162223997>
===
has_cached = false
wr = <null>

Minimal reproduction project

bug-script-ref-count.zip

[adamscott]

It looks like we have an extra reference to the script somewhere, which prevents it from being freed at the right time.

It's by design to enable cyclic references. Each GDScript loaded has a Ref instance living here:

godot/modules/gdscript/gdscript_cache.h

Lines 79 to 81 in e188d61

	HashMap<String, Ref<GDScript>> shallow_gdscript_cache;
	HashMap<String, Ref<GDScript>> full_gdscript_cache;
	HashMap<String, Ref<GDScript>> static_gdscript_cache;

But the garbage collection part is not yet implemented, unfortunately.

[dalexeev]

Does this mean that @static_unload doesn't really matter now: scripts aren't unloaded anyway?

[vnen]

It's by design to enable cyclic references. Each GDScript loaded has a Ref instance living [in the cache]

I believe we talked about this in the chat but let me register here as well for the record. The cache should not be keeping extra references to scripts. It's fine if the scripts themselves are in a cycle, since that is unavoidable (and where a cycle collector would be handy), but in the case there are no cycles the scripts should be naturally freed.

Scripts sometimes preloads scenes which can get quite heavy so keeping them in memory can ended up with a lot of used space which is not obvious where it comes from. Without the collector it's much worse because there's currently no way to unload scripts.

So this needs to be addressed. I do plan to work on this (probably next month if nobody beats me to it). I've also notice some other issues with the cache, like keeping non-compiled scripts as if they were compiled which gives another sort of problems.

[adamscott]

The cache should not be keeping extra references to scripts.

I agree. I was the one that modified the cache to make it so that it keeps an extra reference to the script.

But the reason why references are kept in the cache is because a script reference count would drop to zero even before having the opportunity to cyclically reference itself. So if we're able to fix that issue, I'm all in to drop these references.

Maybe we could try to store the refs shallow scripts, but once a script is fully loaded, only cache the pointer (not a ref of a script).

[jinyangcruise]

Looking forward to any progress on this👀