From c450f4d667e9f2462cb506cd3a32ca882c49ba68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Thu, 5 Sep 2024 08:35:05 +0200 Subject: [PATCH] ResourceLoader: Simplify handling of unregistered tasks --- core/io/resource_loader.cpp | 70 ++++++++++++++++--------------------- core/io/resource_loader.h | 4 ++- 2 files changed, 34 insertions(+), 40 deletions(-) diff --git a/core/io/resource_loader.cpp b/core/io/resource_loader.cpp index 7cf101b0de5..a5f860e1671 100644 --- a/core/io/resource_loader.cpp +++ b/core/io/resource_loader.cpp @@ -234,17 +234,22 @@ void ResourceLoader::LoadToken::clear() { // User-facing tokens shouldn't be deleted until completely claimed. DEV_ASSERT(user_rc == 0 && user_path.is_empty()); - if (!local_path.is_empty()) { // Empty is used for the special case where the load task is not registered. - DEV_ASSERT(thread_load_tasks.has(local_path)); - ThreadLoadTask &load_task = thread_load_tasks[local_path]; - if (load_task.task_id && !load_task.awaited) { - task_to_await = load_task.task_id; + if (!local_path.is_empty()) { + if (task_if_unregistered) { + memdelete(task_if_unregistered); + task_if_unregistered = nullptr; + } else { + DEV_ASSERT(thread_load_tasks.has(local_path)); + ThreadLoadTask &load_task = thread_load_tasks[local_path]; + if (load_task.task_id && !load_task.awaited) { + task_to_await = load_task.task_id; + } + // Removing a task which is still in progress would be catastrophic. + // Tokens must be alive until the task thread function is done. + DEV_ASSERT(load_task.status == THREAD_LOAD_FAILED || load_task.status == THREAD_LOAD_LOADED); + thread_load_tasks.erase(local_path); } - // Removing a task which is still in progress would be catastrophic. - // Tokens must be alive until the task thread function is done. - DEV_ASSERT(load_task.status == THREAD_LOAD_FAILED || load_task.status == THREAD_LOAD_LOADED); - thread_load_tasks.erase(local_path); - local_path.clear(); + local_path.clear(); // Mark as already cleared. } } @@ -521,9 +526,7 @@ Ref ResourceLoader::_load_start(const String &p_path, Ref load_token; bool must_not_register = false; - ThreadLoadTask unregistered_load_task; // Once set, must be valid up to the call to do the load. ThreadLoadTask *load_task_ptr = nullptr; - bool run_on_current_thread = false; { MutexLock thread_load_lock(thread_load_mutex); @@ -578,12 +581,11 @@ Ref ResourceLoader::_load_start(const String &p_path, } } - // If we want to ignore cache, but there's another task loading it, we can't add this one to the map and we also have to finish within scope. + // If we want to ignore cache, but there's another task loading it, we can't add this one to the map. must_not_register = ignoring_cache && thread_load_tasks.has(local_path); if (must_not_register) { - load_token->local_path.clear(); - unregistered_load_task = load_task; - load_task_ptr = &unregistered_load_task; + load_token->task_if_unregistered = memnew(ThreadLoadTask(load_task)); + load_task_ptr = load_token->task_if_unregistered; } else { DEV_ASSERT(!thread_load_tasks.has(local_path)); HashMap::Iterator E = thread_load_tasks.insert(local_path, load_task); @@ -591,9 +593,7 @@ Ref ResourceLoader::_load_start(const String &p_path, } } - run_on_current_thread = must_not_register || p_thread_mode == LOAD_THREAD_FROM_CURRENT; - - if (run_on_current_thread) { + if (p_thread_mode == LOAD_THREAD_FROM_CURRENT) { // The current thread may happen to be a thread from the pool. WorkerThreadPool::TaskID tid = WorkerThreadPool::get_singleton()->get_caller_task_id(); if (tid != WorkerThreadPool::INVALID_TASK_ID) { @@ -606,11 +606,8 @@ Ref ResourceLoader::_load_start(const String &p_path, } } // MutexLock(thread_load_mutex). - if (run_on_current_thread) { + if (p_thread_mode == LOAD_THREAD_FROM_CURRENT) { _run_load_task(load_task_ptr); - if (must_not_register) { - load_token->res_if_unregistered = load_task_ptr->resource; - } } return load_token; @@ -738,7 +735,10 @@ Ref ResourceLoader::_load_complete_inner(LoadToken &p_load_token, Erro *r_error = OK; } - if (!p_load_token.local_path.is_empty()) { + ThreadLoadTask *load_task_ptr = nullptr; + if (p_load_token.task_if_unregistered) { + load_task_ptr = p_load_token.task_if_unregistered; + } else { if (!thread_load_tasks.has(p_load_token.local_path)) { if (r_error) { *r_error = ERR_BUG; @@ -809,22 +809,14 @@ Ref ResourceLoader::_load_complete_inner(LoadToken &p_load_token, Erro load_task.error = FAILED; } - Ref resource = load_task.resource; - if (r_error) { - *r_error = load_task.error; - } - return resource; - } else { - // Special case of an unregistered task. - // The resource should have been loaded by now. - Ref resource = p_load_token.res_if_unregistered; - if (!resource.is_valid()) { - if (r_error) { - *r_error = FAILED; - } - } - return resource; + load_task_ptr = &load_task; } + + Ref resource = load_task_ptr->resource; + if (r_error) { + *r_error = load_task_ptr->error; + } + return resource; } bool ResourceLoader::_ensure_load_progress() { diff --git a/core/io/resource_loader.h b/core/io/resource_loader.h index f75bf019fb5..34ac1ba3e92 100644 --- a/core/io/resource_loader.h +++ b/core/io/resource_loader.h @@ -106,6 +106,8 @@ class ResourceLoader { MAX_LOADERS = 64 }; + struct ThreadLoadTask; + public: enum ThreadLoadStatus { THREAD_LOAD_INVALID_RESOURCE, @@ -124,7 +126,7 @@ public: String local_path; String user_path; uint32_t user_rc = 0; // Having user RC implies regular RC incremented in one, until the user RC reaches zero. - Ref res_if_unregistered; + ThreadLoadTask *task_if_unregistered = nullptr; void clear();