Renderer: Reduce scope of mutex locks to prevent common deadlocks #105138

stuartcarnie · 2025-04-08T05:15:29Z

Introduces fine-grained locking to ShaderRD, so that each version has its own lock, making ShaderRD version APIs thread safe.

Note

It is expected that one of the initialize APIs are called prior to querying version, which is a requirement. The initialize APIs mutate some of the state, after which point it is read-only.

Additional changes to BaseMaterial3D and MaterialStorage to extract the queued updates into a separate list (non-allocating) and then releases the lock. This is to ensure that any shader updates are not performed under the shared locks of BaseMaterial3D and MaterialStorage.

Now that accessing shader versions of ShaderRD is thread-safe, we remove a number of calls to the shared mutex in SceneShaderForwardClustered when querying shader versions, which was a source of many deadlocks.

ShaderRD no longer uses a separate named WorkerThreadPool.

Note

We may want to introduce a platform-specific Thread implementation for Apple, so that we can increase the size of the stack for worker threads.

stuartcarnie

Left a few notes

stuartcarnie · 2025-04-08T05:20:04Z

core/object/worker_thread_pool.cpp

@@ -182,6 +182,7 @@ void WorkerThreadPool::_process_task(Task *p_task) {

 void WorkerThreadPool::_thread_function(void *p_user) {
 	ThreadData *thread_data = (ThreadData *)p_user;
+	Thread::set_name(vformat("WorkerThread %d", thread_data->index));


Make it easy to find the worker threads

stuartcarnie · 2025-04-08T05:20:44Z

scene/resources/material.cpp

-	if (shader_map.has(current_key)) {
-		shader_map[current_key].users--;
-		if (shader_map[current_key].users == 0) {
-			// Deallocate shader which is no longer in use.
-			RS::get_singleton()->free(shader_map[current_key].shader);
-			shader_map.erase(current_key);
+	{
+		MutexLock lock(shader_map_mutex);
+		if (ShaderData *v = shader_map.getptr(current_key); v) {
+			v->users--;
+			if (v->users == 0) {
+				// Deallocate shader which is no longer in use.
+				RS::get_singleton()->free(v->shader);


An improvement, as we no longer perform multiple lookups into shader_map too

This is a great pattern that is worth using more widely. Calling has() is almost always a waste of cycles

stuartcarnie · 2025-04-08T05:21:13Z

scene/resources/material.cpp

-	if (shader_map.has(mk)) {
-		shader_rid = shader_map[mk].shader;
-		shader_map[mk].users++;
+		if (ShaderData *v = shader_map.getptr(mk); v) {
+			shader_rid = v->shader;
+			v->users++;


Same here – let's not perform multiple lookups into shader_map.

stuartcarnie · 2025-04-08T05:24:18Z

scene/resources/material.cpp

+	if (ShaderData *v = shader_map.getptr(mk); v) {
+		// We raced and managed to sneak the same key in concurrently, so we'll destroy the one we just created,
+		// given we know it isn't used, and use the winner.
+		RS::get_singleton()->free(shader_data.shader);


@clayjohn I had to remove this, as it caused a crash in the List destructor of this list, as it wasn't empty:

godot/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.h

Line 291 in 324512e

SelfList<ShaderData>::List shader_list;

Makes sense

stuartcarnie · 2025-04-08T05:25:09Z

scene/resources/material.cpp

+	SelfList<BaseMaterial3D>::List copy;
+	{
+		MutexLock lock(material_mutex);
+		while (SelfList<BaseMaterial3D> *E = dirty_materials.first()) {
+			dirty_materials.remove(E);
+			copy.add(E);
+		}
+	}

-	while (dirty_materials.first()) {
-		dirty_materials.first()->self()->_update_shader();
-		dirty_materials.first()->remove_from_list();
+	while (SelfList<BaseMaterial3D> *E = copy.first()) {
+		E->self()->_update_shader();
+		copy.remove(E);


This doesn't allocate anything, which is convenient! We just remove from the dirty_materials list into the local copy.

stuartcarnie · 2025-04-08T05:27:23Z

servers/rendering/renderer_rd/shader_rd.h

-	Mutex variant_set_mutex;
-


No longer needed, as we have a mutex per version.

stuartcarnie · 2025-04-08T05:27:36Z

servers/rendering/renderer_rd/shader_rd.h

@@ -95,7 +94,9 @@ class ShaderRD {
 	void _compile_ensure_finished(Version *p_version);
 	void _allocate_placeholders(Version *p_version, int p_group);

-	RID_Owner<Version> version_owner;
+	RID_Owner<Version, true> version_owner;


Make this thread-safe now

stuartcarnie · 2025-04-08T05:28:32Z

servers/rendering/renderer_rd/storage_rd/material_storage.cpp

+	SelfList<Material>::List copy;
+	{
+		MutexLock lock(material_update_list_mutex);
+		while (SelfList<Material> *E = material_update_list.first()) {
+			DEV_ASSERT(E == &E->self()->update_element);
+			material_update_list.remove(E);
+			copy.add(E);
+		}
+	}
+
+	while (SelfList<Material> *E = copy.first()) {
+		Material *material = E->self();
+		copy.remove(E);


This change was necessary to limit the scope of the lock, as update_parameters could cause shader compilation.

scene/resources/material.cpp

Fixes godotengine#102877

stuartcarnie · 2025-04-12T20:57:23Z

@clayjohn I've switched to the preferred style

clayjohn

Thanks!

Repiteo · 2025-04-15T00:43:24Z

Thanks!

stuartcarnie requested review from a team as code owners April 8, 2025 05:15

bruvzg self-requested a review April 8, 2025 05:16

stuartcarnie force-pushed the fix_hangs branch from 0b310b0 to 9521403 Compare April 8, 2025 05:17

stuartcarnie commented Apr 8, 2025

View reviewed changes

clayjohn requested changes Apr 8, 2025

View reviewed changes

scene/resources/material.cpp Outdated Show resolved Hide resolved

scene/resources/material.cpp Outdated Show resolved Hide resolved

scene/resources/material.cpp Outdated Show resolved Hide resolved

akien-mga added bug topic:rendering labels Apr 8, 2025

akien-mga added this to the 4.5 milestone Apr 8, 2025

clayjohn added high priority topic:core labels Apr 8, 2025

clayjohn mentioned this pull request Apr 10, 2025

Scenes can't be fully loaded and Godot freezes #102877

Closed

Renderer: Reduce scope of mutex locks to prevent common deadlocks

09282c3

Fixes godotengine#102877

stuartcarnie force-pushed the fix_hangs branch from 9521403 to 09282c3 Compare April 12, 2025 20:56

clayjohn approved these changes Apr 12, 2025

View reviewed changes

Repiteo merged commit f56a4d4 into godotengine:master Apr 15, 2025
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renderer: Reduce scope of mutex locks to prevent common deadlocks #105138

Renderer: Reduce scope of mutex locks to prevent common deadlocks #105138

stuartcarnie commented Apr 8, 2025 •

edited

Loading

stuartcarnie left a comment

stuartcarnie Apr 8, 2025

stuartcarnie Apr 8, 2025

clayjohn Apr 8, 2025

stuartcarnie Apr 8, 2025

stuartcarnie Apr 8, 2025

clayjohn Apr 8, 2025

stuartcarnie Apr 8, 2025

stuartcarnie Apr 8, 2025

stuartcarnie Apr 8, 2025

stuartcarnie Apr 8, 2025

stuartcarnie commented Apr 12, 2025

clayjohn left a comment

Repiteo commented Apr 15, 2025

Renderer: Reduce scope of mutex locks to prevent common deadlocks #105138

Renderer: Reduce scope of mutex locks to prevent common deadlocks #105138

Conversation

stuartcarnie commented Apr 8, 2025 • edited Loading

stuartcarnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartcarnie commented Apr 12, 2025

clayjohn left a comment

Choose a reason for hiding this comment

Repiteo commented Apr 15, 2025

stuartcarnie commented Apr 8, 2025 •

edited

Loading