diff --git a/core/object/worker_thread_pool.cpp b/core/object/worker_thread_pool.cpp index 84ef71c815a..2e0a01f1653 100644 --- a/core/object/worker_thread_pool.cpp +++ b/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)); while (true) { Task *task_to_process = nullptr; diff --git a/scene/resources/material.cpp b/scene/resources/material.cpp index d1945cfc15e..3cfb4a45520 100644 --- a/scene/resources/material.cpp +++ b/scene/resources/material.cpp @@ -684,28 +684,37 @@ void BaseMaterial3D::_update_shader() { return; //no update required in the end } - MutexLock lock(shader_map_mutex); - 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); + ShaderData *v = shader_map.getptr(current_key); + if (v) { + v->users--; + if (v->users == 0) { + // Deallocate shader which is no longer in use. + shader_rid = RID(); + RS::get_singleton()->free(v->shader); + shader_map.erase(current_key); + } + } + + current_key = mk; + + v = shader_map.getptr(mk); + if (v) { + shader_rid = v->shader; + v->users++; + + if (_get_material().is_valid()) { + RS::get_singleton()->material_set_shader(_get_material(), shader_rid); + } + + return; } } - current_key = mk; - - if (shader_map.has(mk)) { - shader_rid = shader_map[mk].shader; - shader_map[mk].users++; - - if (_get_material().is_valid()) { - RS::get_singleton()->material_set_shader(_get_material(), shader_rid); - } - - return; - } + // From this point, it is possible that multiple threads requesting the same key will + // race to create the shader. The winner, which is the one found in shader_map, will be + // used. The losers will free their shader. String texfilter_str; // Force linear filtering for the heightmap texture, as the heightmap effect @@ -1929,11 +1938,28 @@ void fragment() {)"; code += "}\n"; - ShaderData shader_data; - shader_data.shader = RS::get_singleton()->shader_create_from_code(code); - shader_data.users = 1; - shader_map[mk] = shader_data; - shader_rid = shader_data.shader; + // We must create the shader outside the shader_map_mutex to avoid potential deadlocks with + // other tasks in the WorkerThreadPool simultaneously creating materials, which + // may also hold the shared shader_map_mutex lock. + RID new_shader = RS::get_singleton()->shader_create_from_code(code); + + MutexLock lock(shader_map_mutex); + + ShaderData *v = shader_map.getptr(mk); + if (unlikely(v)) { + // We raced and managed to create the same key concurrently, so we'll free the shader we just created, + // given we know it isn't used, and use the winner. + RS::get_singleton()->free(new_shader); + } else { + ShaderData shader_data; + shader_data.shader = new_shader; + // ShaderData will be inserted with a users count of 0, but we + // increment unconditionally outside this if block, whilst still under lock. + v = &shader_map.insert(mk, shader_data)->value; + } + + shader_rid = v->shader; + v->users++; if (_get_material().is_valid()) { RS::get_singleton()->material_set_shader(_get_material(), shader_rid); @@ -1959,11 +1985,18 @@ void BaseMaterial3D::_check_material_rid() { } void BaseMaterial3D::flush_changes() { - MutexLock lock(material_mutex); + SelfList::List copy; + { + MutexLock lock(material_mutex); + while (SelfList *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 *E = copy.first()) { + E->self()->_update_shader(); + copy.remove(E); } } diff --git a/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp b/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp index 5debecb134d..b7b14ce6b86 100644 --- a/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp +++ b/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp @@ -141,8 +141,11 @@ void SceneShaderForwardClustered::ShaderData::set_code(const String &p_code) { actions.uniforms = &uniforms; - MutexLock lock(SceneShaderForwardClustered::singleton_mutex); - Error err = SceneShaderForwardClustered::singleton->compiler.compile(RS::SHADER_SPATIAL, code, &actions, path, gen_code); + Error err = OK; + { + MutexLock lock(SceneShaderForwardClustered::singleton_mutex); + err = SceneShaderForwardClustered::singleton->compiler.compile(RS::SHADER_SPATIAL, code, &actions, path, gen_code); + } if (err != OK) { if (version.is_valid()) { @@ -215,7 +218,6 @@ bool SceneShaderForwardClustered::ShaderData::casts_shadows() const { RS::ShaderNativeSourceCode SceneShaderForwardClustered::ShaderData::get_native_source_code() const { if (version.is_valid()) { - MutexLock lock(SceneShaderForwardClustered::singleton_mutex); return SceneShaderForwardClustered::singleton->shader.version_get_native_source_code(version); } else { return RS::ShaderNativeSourceCode(); @@ -406,7 +408,6 @@ RD::PolygonCullMode SceneShaderForwardClustered::ShaderData::get_cull_mode_from_ RID SceneShaderForwardClustered::ShaderData::_get_shader_variant(uint16_t p_shader_version) const { if (version.is_valid()) { - MutexLock lock(SceneShaderForwardClustered::singleton_mutex); ERR_FAIL_NULL_V(SceneShaderForwardClustered::singleton, RID()); return SceneShaderForwardClustered::singleton->shader.version_get_shader(version, p_shader_version); } else { @@ -441,7 +442,6 @@ uint64_t SceneShaderForwardClustered::ShaderData::get_vertex_input_mask(Pipeline bool SceneShaderForwardClustered::ShaderData::is_valid() const { if (version.is_valid()) { - MutexLock lock(SceneShaderForwardClustered::singleton_mutex); ERR_FAIL_NULL_V(SceneShaderForwardClustered::singleton, false); return SceneShaderForwardClustered::singleton->shader.version_is_valid(version); } else { @@ -459,7 +459,6 @@ SceneShaderForwardClustered::ShaderData::~ShaderData() { pipeline_hash_map.clear_pipelines(); if (version.is_valid()) { - MutexLock lock(SceneShaderForwardClustered::singleton_mutex); ERR_FAIL_NULL(SceneShaderForwardClustered::singleton); SceneShaderForwardClustered::singleton->shader.version_free(version); } @@ -482,8 +481,10 @@ void SceneShaderForwardClustered::MaterialData::set_next_pass(RID p_pass) { bool SceneShaderForwardClustered::MaterialData::update_parameters(const HashMap &p_parameters, bool p_uniform_dirty, bool p_textures_dirty) { if (shader_data->version.is_valid()) { + RID shader_rid = SceneShaderForwardClustered::singleton->shader.version_get_shader(shader_data->version, 0); + MutexLock lock(SceneShaderForwardClustered::singleton_mutex); - return update_parameters_uniform_set(p_parameters, p_uniform_dirty, p_textures_dirty, shader_data->uniforms, shader_data->ubo_offsets.ptr(), shader_data->texture_uniforms, shader_data->default_texture_params, shader_data->ubo_size, uniform_set, SceneShaderForwardClustered::singleton->shader.version_get_shader(shader_data->version, 0), RenderForwardClustered::MATERIAL_UNIFORM_SET, true, true); + return update_parameters_uniform_set(p_parameters, p_uniform_dirty, p_textures_dirty, shader_data->uniforms, shader_data->ubo_offsets.ptr(), shader_data->texture_uniforms, shader_data->default_texture_params, shader_data->ubo_size, uniform_set, shader_rid, RenderForwardClustered::MATERIAL_UNIFORM_SET, true, true); } else { return false; } diff --git a/servers/rendering/renderer_rd/shader_rd.cpp b/servers/rendering/renderer_rd/shader_rd.cpp index a8a9566f7e4..8910876ffa1 100644 --- a/servers/rendering/renderer_rd/shader_rd.cpp +++ b/servers/rendering/renderer_rd/shader_rd.cpp @@ -176,7 +176,11 @@ RID ShaderRD::version_create() { version.initialize_needed = true; version.variants.clear(); version.variant_data.clear(); - return version_owner.make_rid(version); + version.mutex = memnew(Mutex); + RID rid = version_owner.make_rid(version); + MutexLock lock(versions_mutex); + version_mutexes.insert(rid, version.mutex); + return rid; } void ShaderRD::_initialize_version(Version *p_version) { @@ -328,7 +332,6 @@ void ShaderRD::_compile_variant(uint32_t p_variant, CompileData p_data) { } if (!build_ok) { - MutexLock lock(variant_set_mutex); //properly print the errors ERR_PRINT("Error compiling " + String(current_stage == RD::SHADER_STAGE_COMPUTE ? "Compute " : (current_stage == RD::SHADER_STAGE_VERTEX ? "Vertex" : "Fragment")) + " shader, variant #" + itos(variant) + " (" + variant_defines[variant].text.get_data() + ")."); ERR_PRINT(error); @@ -343,8 +346,6 @@ void ShaderRD::_compile_variant(uint32_t p_variant, CompileData p_data) { ERR_FAIL_COND(shader_data.is_empty()); { - MutexLock lock(variant_set_mutex); - p_data.version->variants.write[variant] = RD::get_singleton()->shader_create_from_bytecode_with_samplers(shader_data, p_data.version->variants[variant], immutable_samplers); p_data.version->variant_data.write[variant] = shader_data; } @@ -355,6 +356,8 @@ RS::ShaderNativeSourceCode ShaderRD::version_get_native_source_code(RID p_versio RS::ShaderNativeSourceCode source_code; ERR_FAIL_NULL_V(version, source_code); + MutexLock lock(*version->mutex); + source_code.versions.resize(variant_defines.size()); for (int i = 0; i < source_code.versions.size(); i++) { @@ -481,12 +484,10 @@ bool ShaderRD::_load_from_cache(Version *p_version, int p_group) { for (uint32_t i = 0; i < variant_count; i++) { int variant_id = group_to_variant_map[p_group][i]; if (!variants_enabled[variant_id]) { - MutexLock lock(variant_set_mutex); p_version->variants.write[variant_id] = RID(); continue; } { - MutexLock lock(variant_set_mutex); RID shader = RD::get_singleton()->shader_create_from_bytecode_with_samplers(p_version->variant_data[variant_id], p_version->variants[variant_id], immutable_samplers); if (shader.is_null()) { for (uint32_t j = 0; j < i; j++) { @@ -527,7 +528,6 @@ void ShaderRD::_allocate_placeholders(Version *p_version, int p_group) { int variant_id = group_to_variant_map[p_group][i]; RID shader = RD::get_singleton()->shader_create_placeholder(); { - MutexLock lock(variant_set_mutex); p_version->variants.write[variant_id] = shader; } } @@ -554,7 +554,7 @@ void ShaderRD::_compile_version_start(Version *p_version, int p_group) { compile_data.version = p_version; compile_data.group = p_group; - WorkerThreadPool::GroupID group_task = WorkerThreadPool::get_named_pool(SNAME("ShaderCompilationPool"))->add_template_group_task(this, &ShaderRD::_compile_variant, compile_data, group_to_variant_map[p_group].size(), -1, true, SNAME("ShaderCompilation")); + WorkerThreadPool::GroupID group_task = WorkerThreadPool::get_singleton()->add_template_group_task(this, &ShaderRD::_compile_variant, compile_data, group_to_variant_map[p_group].size(), -1, true, SNAME("ShaderCompilation")); p_version->group_compilation_tasks.write[p_group] = group_task; } @@ -563,7 +563,7 @@ void ShaderRD::_compile_version_end(Version *p_version, int p_group) { return; } WorkerThreadPool::GroupID group_task = p_version->group_compilation_tasks[p_group]; - WorkerThreadPool::get_named_pool(SNAME("ShaderCompilationPool"))->wait_for_group_task_completion(group_task); + WorkerThreadPool::get_singleton()->wait_for_group_task_completion(group_task); p_version->group_compilation_tasks.write[p_group] = 0; bool all_valid = true; @@ -616,6 +616,8 @@ void ShaderRD::version_set_code(RID p_version, const HashMap &p_ Version *version = version_owner.get_or_null(p_version); ERR_FAIL_NULL(version); + MutexLock lock(*version->mutex); + _compile_ensure_finished(version); version->vertex_globals = p_vertex_globals.utf8(); @@ -651,6 +653,8 @@ void ShaderRD::version_set_compute_code(RID p_version, const HashMapmutex); + _compile_ensure_finished(version); version->compute_globals = p_compute_globals.utf8(); @@ -684,6 +688,8 @@ bool ShaderRD::version_is_valid(RID p_version) { Version *version = version_owner.get_or_null(p_version); ERR_FAIL_NULL_V(version, false); + MutexLock lock(*version->mutex); + if (version->dirty) { _initialize_version(version); for (int i = 0; i < group_enabled.size(); i++) { @@ -702,9 +708,17 @@ bool ShaderRD::version_is_valid(RID p_version) { bool ShaderRD::version_free(RID p_version) { if (version_owner.owns(p_version)) { + { + MutexLock lock(versions_mutex); + version_mutexes.erase(p_version); + } + Version *version = version_owner.get_or_null(p_version); + version->mutex->lock(); _clear_version(version); version_owner.free(p_version); + version->mutex->unlock(); + memdelete(version->mutex); } else { return false; } @@ -738,7 +752,9 @@ void ShaderRD::enable_group(int p_group) { version_owner.get_owned_list(&all_versions); for (const RID &E : all_versions) { Version *version = version_owner.get_or_null(E); + version->mutex->lock(); _compile_version_start(version, p_group); + version->mutex->unlock(); } } diff --git a/servers/rendering/renderer_rd/shader_rd.h b/servers/rendering/renderer_rd/shader_rd.h index e583ad86a7b..578e953e854 100644 --- a/servers/rendering/renderer_rd/shader_rd.h +++ b/servers/rendering/renderer_rd/shader_rd.h @@ -63,6 +63,7 @@ private: Vector immutable_samplers; struct Version { + Mutex *mutex = nullptr; CharString uniforms; CharString vertex_globals; CharString compute_globals; @@ -79,8 +80,6 @@ private: bool initialize_needed; }; - Mutex variant_set_mutex; - struct CompileData { Version *version; int group = 0; @@ -95,7 +94,9 @@ private: void _compile_ensure_finished(Version *p_version); void _allocate_placeholders(Version *p_version, int p_group); - RID_Owner version_owner; + RID_Owner version_owner; + Mutex versions_mutex; + HashMap version_mutexes; struct StageTemplate { struct Chunk { @@ -168,6 +169,8 @@ public: Version *version = version_owner.get_or_null(p_version); ERR_FAIL_NULL_V(version, RID()); + MutexLock lock(*version->mutex); + if (version->dirty) { _initialize_version(version); for (int i = 0; i < group_enabled.size(); i++) { diff --git a/servers/rendering/renderer_rd/storage_rd/material_storage.cpp b/servers/rendering/renderer_rd/storage_rd/material_storage.cpp index 1e4d4981541..2268fa523cf 100644 --- a/servers/rendering/renderer_rd/storage_rd/material_storage.cpp +++ b/servers/rendering/renderer_rd/storage_rd/material_storage.cpp @@ -2136,9 +2136,19 @@ void MaterialStorage::_material_queue_update(Material *material, bool p_uniform, } void MaterialStorage::_update_queued_materials() { - MutexLock lock(material_update_list_mutex); - while (material_update_list.first()) { - Material *material = material_update_list.first()->self(); + SelfList::List copy; + { + MutexLock lock(material_update_list_mutex); + while (SelfList *E = material_update_list.first()) { + DEV_ASSERT(E == &E->self()->update_element); + material_update_list.remove(E); + copy.add(E); + } + } + + while (SelfList *E = copy.first()) { + Material *material = E->self(); + copy.remove(E); bool uniforms_changed = false; if (material->data) { @@ -2147,8 +2157,6 @@ void MaterialStorage::_update_queued_materials() { material->texture_dirty = false; material->uniform_dirty = false; - material_update_list.remove(&material->update_element); - if (uniforms_changed) { //some implementations such as 3D renderer cache the material uniform set, so update is required material->dependency.changed_notify(Dependency::DEPENDENCY_CHANGED_MATERIAL);