ResourceLoader threaded loading causes memory leak

[crackxy1]

Tested versions

• Reproducible in: 4.3.dev3, 4.3.dev5
• Not reproducible in: 4.2.1.stable, 4.2.2.rc1

System information

Godot v4.3.dev5 - Windows 10.0.19045 - GLES3 (Compatibility) - Radeon RX 560X Series (Advanced Micro Devices, Inc.; 31.0.21910.5) - AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx (8 Threads)

Issue description

Using ResourceLoader's threaded loading mechanism causes memory leaks.

Here is a short script which demonstrates it. It just loads a white texture repeatedly. Note how it doesn't hold any references to the loaded texture by the end of _physics_process.

extends Node2D

const PATH = 'res://white_1024.png' # a white 1024x1024 texture, can be any resource
const MAX_LOAD_COUNT = 500 # total of ~2GB RAM usage

var is_loading := false
var load_count := 0

func _physics_process(__):
	if not is_loading:
		is_loading = true
		ResourceLoader.load_threaded_request(PATH)
	else:
		if ResourceLoader.load_threaded_get_status(PATH) == ResourceLoader.THREAD_LOAD_LOADED:
			print('Load count = %s' % load_count)
			var test := ResourceLoader.load_threaded_get(PATH)
			assert(ResourceLoader.has_cached(PATH) == true)
			test = null
			assert(ResourceLoader.has_cached(PATH) == false)

			load_count += 1
			if load_count < MAX_LOAD_COUNT:
				is_loading = false # load again

Steps to reproduce

1. Run the MRP.
2. In the Debugger > Monitors tab, observe how the RAM usage and Objects counter keeps increasing.
3. Observe how the RAM usage and Objects counter doesn't go down even after all the loading is done (after MAX_LOAD_COUNT is reached).

Not sure if related, but notice how the texture memory usage didn't really change throughout.

Sample Run:

Minimal reproduction project (MRP)

resource_threaded_mem_leak_MRP.zip

[sacrificerXY]

After further testing, I think the problem starts at 4.3.dev3. Can't reproduce it in 4.2.2.rc2 or 4.3.dev2.

(I'm OP. Didn't notice I used an alt account when posting this issue.)

[sacrificerXY]

This problem might be specific to textures. I tested loading a WAV and a Curve resource and this is what I got in my machine:

• WAV file (44MB, loaded 500 times in 37 secs)
  

• Curve resource (4.2MB, 100k points, loaded 100 times in 50 secs)
  

• The PNG inside MRP.zip in my post (1024x1024 pure white texture, loaded 500 times in 17 secs)
  

This is using v4.3.dev3. I added a 2 second grace period after reaching the maximum number of iterations before ending the test.

Notice how by the end, the WAV and Curve resource tests went back down to around ~47MB memory usage while the PNG test stayed at ~2GB till the end.

[sacrificerXY]

Just to clarify, using ResourceLoader.load directly doesn't cause any issues.

extends Node2D

const PATH = 'res://white_1024.png'
const MAX_LOAD_COUNT = 2000
var load_count := 0

func _physics_process(__):
	if load_count < MAX_LOAD_COUNT:
		var test := ResourceLoader.load(PATH)
		assert(ResourceLoader.has_cached(PATH) == true)
		test = null
		assert(ResourceLoader.has_cached(PATH) == false)
		load_count += 1

The resulting memory usage graph is a straight line. It didn't budge at all.

[sacrificerXY]

I've bisected it and the problem starts with v4.3.dev.custom_build [ae418f9] which is part of #86587. Something about WorkerThreadPool and CommandQueueMT.

I hope someone else can take a look at this since I'm not touching anything threading related 😆

[akien-mga]

CC @RandomShaper

[Zylann]

I just spent a day bisecting to the same commit: ae418f9

My voxel engine generates meshes at runtime very often in threads. It looks like an amount of memory similar to the size of a mesh is getting leaked every time I generate chunks. This lead to very bad memory consumption and eventually crashes.
Object count doesn't increase, only static memory.

It doesn't reproduce with a minimal project that just creates meshes on the main thread with add_surface_from_arrays, so there is probably more to that story that I'm not sure about.
My code is not using WorkerThreadPool or CommandQueueMT, but seems what I'm doing leads Godot to use it indirectly I suppose.

Regarding ae418f9, I had a brief look and one thing that strikes me is that before, there was a call to the destructor of CommandBase:

cmd->~CommandBase(); //should be done, so erase the command

But I can't find it anymore in the new version.
The new code says // Won't be called., so I suppose it's expected? Why not call it tho? What calls it then? If it's not called, why is it existing? Is it because subtypes only ever stores stuff that doesnt need freeing? There is so much macro magic it's hard to investigate if that's true.
Otherwise I'm not sure what's up here.

[Zylann]

Just tested this:

@@ -374,10 +374,12 @@ class CommandQueueMT {
 			cmd->call();
 			if (sync_sem) {
 				sync_sem->sem.post(); // Release in case it needs sync/ret.
 			}
 
+			cmd->~CommandBase();
+

Which gets rid of the leak. It comes in total contradiction with // Won't be called. though, so I don't know what the intent was here.

[RandomShaper]

@Zylann, that's very interesting. My assessment was indeed that the destructor is trivial and therefore I could avoid calling it. However, that was not the primary reason for removing the call, but something that gave me peace of mind in removing it, the main reason being how reentrancy was being handled. Now reentrancy is handled differently, it can be run, regardless trivial or not. I will add the fix in a PR I'm working on that adds more fixes to threaded servers. I was finding some leaks that may be tracked down to this same issue. I'm very curious as to why my rationale was wrong. I'll report when I know.

[RandomShaper]

Oh, of course. Although they are certainly hidden in the macro magic, derivatives of CommandBase for forwarding of server functions have all the arguments declared as member variables. Not good not calling the destructor for stuff like Ref<>.

[akien-mga]

Confirmed on Linux.