diff options
author | reduz <reduzio@gmail.com> | 2022-06-22 13:46:46 +0200 |
---|---|---|
committer | reduz <reduzio@gmail.com> | 2022-06-22 13:46:46 +0200 |
commit | e772b65d92dbd5b36fb003458d7fe0fd528abcea (patch) | |
tree | aca74602fef05e8f80689345af9ba2e33afe846d /core | |
parent | c18d0f20357a11bd9cfa2f57b8b9b500763413bc (diff) |
Remake resource thread safety and API
* Ensures thread safety when resources are destroyed.
* Simplified API by always forcing `ResourceCache::get_ref`, which needs less hacks and is fully thread safe.
* Removed RWLock for resources because its not possible to use for the new logic. Should not be a problem.
Supersedes #57533
Diffstat (limited to 'core')
-rw-r--r-- | core/io/resource.cpp | 96 | ||||
-rw-r--r-- | core/io/resource.h | 4 | ||||
-rw-r--r-- | core/io/resource_format_binary.cpp | 10 | ||||
-rw-r--r-- | core/io/resource_loader.cpp | 51 |
4 files changed, 80 insertions, 81 deletions
diff --git a/core/io/resource.cpp b/core/io/resource.cpp index ed7228d0b9..fec5ca5c7b 100644 --- a/core/io/resource.cpp +++ b/core/io/resource.cpp @@ -52,41 +52,36 @@ void Resource::set_path(const String &p_path, bool p_take_over) { return; } + if (p_path.is_empty()) { + p_take_over = false; // Can't take over an empty path + } + + ResourceCache::lock.lock(); + if (!path_cache.is_empty()) { - ResourceCache::lock.write_lock(); ResourceCache::resources.erase(path_cache); - ResourceCache::lock.write_unlock(); } path_cache = ""; - ResourceCache::lock.read_lock(); - bool has_path = ResourceCache::resources.has(p_path); - ResourceCache::lock.read_unlock(); + Ref<Resource> existing = ResourceCache::get_ref(p_path); - if (has_path) { + if (existing.is_valid()) { if (p_take_over) { - ResourceCache::lock.write_lock(); - Resource **res = ResourceCache::resources.getptr(p_path); - if (res) { - (*res)->set_name(""); - } - ResourceCache::lock.write_unlock(); + existing->path_cache = String(); + ResourceCache::resources.erase(p_path); } else { - ResourceCache::lock.read_lock(); - bool exists = ResourceCache::resources.has(p_path); - ResourceCache::lock.read_unlock(); - - ERR_FAIL_COND_MSG(exists, "Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion)."); + ResourceCache::lock.unlock(); + ERR_FAIL_MSG("Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion)."); } } + path_cache = p_path; if (!path_cache.is_empty()) { - ResourceCache::lock.write_lock(); ResourceCache::resources[path_cache] = this; - ResourceCache::lock.write_unlock(); } + ResourceCache::lock.unlock(); _resource_path_changed(); } @@ -380,7 +375,7 @@ void Resource::set_as_translation_remapped(bool p_remapped) { return; } - ResourceCache::lock.write_lock(); + ResourceCache::lock.lock(); if (p_remapped) { ResourceLoader::remapped_list.add(&remapped_list); @@ -388,7 +383,7 @@ void Resource::set_as_translation_remapped(bool p_remapped) { ResourceLoader::remapped_list.remove(&remapped_list); } - ResourceCache::lock.write_unlock(); + ResourceCache::lock.unlock(); } bool Resource::is_translation_remapped() const { @@ -455,9 +450,9 @@ Resource::Resource() : Resource::~Resource() { if (!path_cache.is_empty()) { - ResourceCache::lock.write_lock(); + ResourceCache::lock.lock(); ResourceCache::resources.erase(path_cache); - ResourceCache::lock.write_unlock(); + ResourceCache::lock.unlock(); } if (owners.size()) { WARN_PRINT("Resource is still owned."); @@ -469,7 +464,7 @@ HashMap<String, Resource *> ResourceCache::resources; HashMap<String, HashMap<String, String>> ResourceCache::resource_path_cache; #endif -RWLock ResourceCache::lock; +Mutex ResourceCache::lock; #ifdef TOOLS_ENABLED RWLock ResourceCache::path_cache_lock; #endif @@ -491,46 +486,67 @@ void ResourceCache::reload_externals() { } bool ResourceCache::has(const String &p_path) { - lock.read_lock(); - bool b = resources.has(p_path); - lock.read_unlock(); + lock.lock(); + + Resource **res = resources.getptr(p_path); - return b; + if (res && (*res)->reference_get_count() == 0) { + // This resource is in the process of being deleted, ignore its existence. + (*res)->path_cache = String(); + resources.erase(p_path); + res = nullptr; + } + + lock.unlock(); + + if (!res) { + return false; + } + + return true; } -Resource *ResourceCache::get(const String &p_path) { - lock.read_lock(); +Ref<Resource> ResourceCache::get_ref(const String &p_path) { + Ref<Resource> ref; + lock.lock(); Resource **res = resources.getptr(p_path); - lock.read_unlock(); + if (res) { + ref = Ref<Resource>(*res); + } - if (!res) { - return nullptr; + if (res && !ref.is_valid()) { + // This resource is in the process of being deleted, ignore its existence + (*res)->path_cache = String(); + resources.erase(p_path); + res = nullptr; } - return *res; + lock.unlock(); + + return ref; } void ResourceCache::get_cached_resources(List<Ref<Resource>> *p_resources) { - lock.read_lock(); + lock.lock(); for (KeyValue<String, Resource *> &E : resources) { p_resources->push_back(Ref<Resource>(E.value)); } - lock.read_unlock(); + lock.unlock(); } int ResourceCache::get_cached_resource_count() { - lock.read_lock(); + lock.lock(); int rc = resources.size(); - lock.read_unlock(); + lock.unlock(); return rc; } void ResourceCache::dump(const char *p_file, bool p_short) { #ifdef DEBUG_ENABLED - lock.read_lock(); + lock.lock(); HashMap<String, int> type_count; @@ -562,7 +578,7 @@ void ResourceCache::dump(const char *p_file, bool p_short) { } } - lock.read_unlock(); + lock.unlock(); #else WARN_PRINT("ResourceCache::dump only with in debug builds."); #endif diff --git a/core/io/resource.h b/core/io/resource.h index a45bc6e1e4..a2cde87990 100644 --- a/core/io/resource.h +++ b/core/io/resource.h @@ -153,7 +153,7 @@ public: class ResourceCache { friend class Resource; friend class ResourceLoader; //need the lock - static RWLock lock; + static Mutex lock; static HashMap<String, Resource *> resources; #ifdef TOOLS_ENABLED static HashMap<String, HashMap<String, String>> resource_path_cache; // Each tscn has a set of resource paths and IDs. @@ -166,7 +166,7 @@ class ResourceCache { public: static void reload_externals(); static bool has(const String &p_path); - static Resource *get(const String &p_path); + static Ref<Resource> get_ref(const String &p_path); static void dump(const char *p_file = nullptr, bool p_short = false); static void get_cached_resources(List<Ref<Resource>> *p_resources); static int get_cached_resource_count(); diff --git a/core/io/resource_format_binary.cpp b/core/io/resource_format_binary.cpp index 2469e1a4be..b1c50e829c 100644 --- a/core/io/resource_format_binary.cpp +++ b/core/io/resource_format_binary.cpp @@ -693,7 +693,7 @@ Error ResourceLoaderBinary::load() { } if (cache_mode == ResourceFormatLoader::CACHE_MODE_REUSE && ResourceCache::has(path)) { - Ref<Resource> cached = ResourceCache::get(path); + Ref<Resource> cached = ResourceCache::get_ref(path); if (cached.is_valid()) { //already loaded, don't do anything error = OK; @@ -717,10 +717,10 @@ Error ResourceLoaderBinary::load() { if (cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE && ResourceCache::has(path)) { //use the existing one - Resource *r = ResourceCache::get(path); - if (r->get_class() == t) { - r->reset_state(); - res = Ref<Resource>(r); + Ref<Resource> cached = ResourceCache::get_ref(path); + if (cached->get_class() == t) { + cached->reset_state(); + res = cached; } } diff --git a/core/io/resource_loader.cpp b/core/io/resource_loader.cpp index fb21db1a19..2cd455475c 100644 --- a/core/io/resource_loader.cpp +++ b/core/io/resource_loader.cpp @@ -335,23 +335,15 @@ Error ResourceLoader::load_threaded_request(const String &p_path, const String & thread_load_mutex->unlock(); ERR_FAIL_V_MSG(ERR_INVALID_PARAMETER, "Attempted to load a resource already being loaded from this thread, cyclic reference?"); } - //lock first if possible - ResourceCache::lock.read_lock(); - - //get ptr - Resource **rptr = ResourceCache::resources.getptr(local_path); - - if (rptr) { - Ref<Resource> res(*rptr); - //it is possible this resource was just freed in a thread. If so, this referencing will not work and resource is considered not cached - if (res.is_valid()) { - //referencing is fine - load_task.resource = res; - load_task.status = THREAD_LOAD_LOADED; - load_task.progress = 1.0; - } + + Ref<Resource> existing = ResourceCache::get_ref(local_path); + + if (existing.is_valid()) { + //referencing is fine + load_task.resource = existing; + load_task.status = THREAD_LOAD_LOADED; + load_task.progress = 1.0; } - ResourceCache::lock.read_unlock(); } if (!p_source_resource.is_empty()) { @@ -530,27 +522,18 @@ Ref<Resource> ResourceLoader::load(const String &p_path, const String &p_type_hi } //Is it cached? - ResourceCache::lock.read_lock(); - - Resource **rptr = ResourceCache::resources.getptr(local_path); - if (rptr) { - Ref<Resource> res(*rptr); + Ref<Resource> existing = ResourceCache::get_ref(local_path); - //it is possible this resource was just freed in a thread. If so, this referencing will not work and resource is considered not cached - if (res.is_valid()) { - ResourceCache::lock.read_unlock(); - thread_load_mutex->unlock(); - - if (r_error) { - *r_error = OK; - } + if (existing.is_valid()) { + thread_load_mutex->unlock(); - return res; //use cached + if (r_error) { + *r_error = OK; } - } - ResourceCache::lock.read_unlock(); + return existing; //use cached + } //load using task (but this thread) ThreadLoadTask load_task; @@ -867,7 +850,7 @@ String ResourceLoader::path_remap(const String &p_path) { } void ResourceLoader::reload_translation_remaps() { - ResourceCache::lock.read_lock(); + ResourceCache::lock.lock(); List<Resource *> to_reload; SelfList<Resource> *E = remapped_list.first(); @@ -877,7 +860,7 @@ void ResourceLoader::reload_translation_remaps() { E = E->next(); } - ResourceCache::lock.read_unlock(); + ResourceCache::lock.unlock(); //now just make sure to not delete any of these resources while changing locale.. while (to_reload.front()) { |