From 9d7ab477387bb9afc00181dfa70e1e0d1a27b28b Mon Sep 17 00:00:00 2001 From: iwubcode Date: Sun, 4 Jun 2023 23:01:29 -0500 Subject: [PATCH] VideoCommon: add additional locks around asset access and usage to ensure thread safety --- .../Core/VideoCommon/Assets/CustomAsset.cpp | 3 + Source/Core/VideoCommon/Assets/CustomAsset.h | 9 ++- .../VideoCommon/Assets/CustomAssetLoader.cpp | 10 +-- .../VideoCommon/Assets/CustomAssetLoader.h | 21 +++-- .../Assets/DirectFilesystemAssetLibrary.cpp | 76 +++++++++++-------- .../Assets/DirectFilesystemAssetLibrary.h | 11 ++- .../Core/VideoCommon/Assets/TextureAsset.cpp | 18 +++-- 7 files changed, 92 insertions(+), 56 deletions(-) diff --git a/Source/Core/VideoCommon/Assets/CustomAsset.cpp b/Source/Core/VideoCommon/Assets/CustomAsset.cpp index 545b60e093..1591f93f91 100644 --- a/Source/Core/VideoCommon/Assets/CustomAsset.cpp +++ b/Source/Core/VideoCommon/Assets/CustomAsset.cpp @@ -16,6 +16,7 @@ bool CustomAsset::Load() const auto load_information = LoadImpl(m_asset_id); if (load_information.m_bytes_loaded > 0) { + std::lock_guard lk(m_info_lock); m_bytes_loaded = load_information.m_bytes_loaded; m_last_loaded_time = load_information.m_load_time; } @@ -29,6 +30,7 @@ CustomAssetLibrary::TimeType CustomAsset::GetLastWriteTime() const const CustomAssetLibrary::TimeType& CustomAsset::GetLastLoadedTime() const { + std::lock_guard lk(m_info_lock); return m_last_loaded_time; } @@ -39,6 +41,7 @@ const CustomAssetLibrary::AssetID& CustomAsset::GetAssetId() const std::size_t CustomAsset::GetByteSizeInMemory() const { + std::lock_guard lk(m_info_lock); return m_bytes_loaded; } diff --git a/Source/Core/VideoCommon/Assets/CustomAsset.h b/Source/Core/VideoCommon/Assets/CustomAsset.h index 6fd890e4bc..563955188c 100644 --- a/Source/Core/VideoCommon/Assets/CustomAsset.h +++ b/Source/Core/VideoCommon/Assets/CustomAsset.h @@ -30,6 +30,7 @@ public: // Queries the last time the asset was modified or standard epoch time // if the asset hasn't been modified yet + // Note: not thread safe, expected to be called by the loader CustomAssetLibrary::TimeType GetLastWriteTime() const; // Returns the time that the data was last loaded @@ -48,8 +49,10 @@ protected: private: virtual CustomAssetLibrary::LoadInfo LoadImpl(const CustomAssetLibrary::AssetID& asset_id) = 0; CustomAssetLibrary::AssetID m_asset_id; + + mutable std::mutex m_info_lock; std::size_t m_bytes_loaded = 0; - CustomAssetLibrary::TimeType m_last_loaded_time; + CustomAssetLibrary::TimeType m_last_loaded_time = {}; }; // An abstract class that is expected to @@ -70,7 +73,7 @@ public: // they want to handle reloads [[nodiscard]] std::shared_ptr GetData() const { - std::lock_guard lk(m_lock); + std::lock_guard lk(m_data_lock); if (m_loaded) return m_data; return nullptr; @@ -78,7 +81,7 @@ public: protected: bool m_loaded = false; - mutable std::mutex m_lock; + mutable std::mutex m_data_lock; std::shared_ptr m_data; }; diff --git a/Source/Core/VideoCommon/Assets/CustomAssetLoader.cpp b/Source/Core/VideoCommon/Assets/CustomAssetLoader.cpp index d2189e655b..bb404064d2 100644 --- a/Source/Core/VideoCommon/Assets/CustomAssetLoader.cpp +++ b/Source/Core/VideoCommon/Assets/CustomAssetLoader.cpp @@ -30,7 +30,7 @@ void CustomAssetLoader::Init() std::this_thread::sleep_for(TIME_BETWEEN_ASSET_MONITOR_CHECKS); - std::lock_guard lk(m_assets_lock); + std::lock_guard lk(m_asset_load_lock); for (auto& [asset_id, asset_to_monitor] : m_assets_to_monitor) { if (auto ptr = asset_to_monitor.lock()) @@ -50,11 +50,11 @@ void CustomAssetLoader::Init() { if (ptr->Load()) { - if (m_max_memory_available >= m_total_bytes_loaded + ptr->GetByteSizeInMemory()) + std::lock_guard lk(m_asset_load_lock); + const std::size_t asset_memory_size = ptr->GetByteSizeInMemory(); + if (m_max_memory_available >= m_total_bytes_loaded + asset_memory_size) { - m_total_bytes_loaded += ptr->GetByteSizeInMemory(); - - std::lock_guard lk(m_assets_lock); + m_total_bytes_loaded += asset_memory_size; m_assets_to_monitor.try_emplace(ptr->GetAssetId(), ptr); } else diff --git a/Source/Core/VideoCommon/Assets/CustomAssetLoader.h b/Source/Core/VideoCommon/Assets/CustomAssetLoader.h index da1f54b133..6e4596b8b2 100644 --- a/Source/Core/VideoCommon/Assets/CustomAssetLoader.h +++ b/Source/Core/VideoCommon/Assets/CustomAssetLoader.h @@ -52,12 +52,17 @@ private: { auto [it, inserted] = asset_map.try_emplace(asset_id); if (!inserted) - return it->second.lock(); + { + auto shared = it->second.lock(); + if (shared) + return shared; + } std::shared_ptr ptr(new AssetType(std::move(library), asset_id), [&](AssetType* a) { - asset_map.erase(a->GetAssetId()); - m_total_bytes_loaded -= a->GetByteSizeInMemory(); - std::lock_guard lk(m_assets_lock); - m_assets_to_monitor.erase(a->GetAssetId()); + { + std::lock_guard lk(m_asset_load_lock); + m_total_bytes_loaded -= a->GetByteSizeInMemory(); + m_assets_to_monitor.erase(a->GetAssetId()); + } delete a; }); it->second = ptr; @@ -66,6 +71,7 @@ private: } static constexpr auto TIME_BETWEEN_ASSET_MONITOR_CHECKS = std::chrono::milliseconds{500}; + std::map> m_textures; std::map> m_game_textures; std::thread m_asset_monitor_thread; @@ -75,7 +81,10 @@ private: std::size_t m_max_memory_available = 0; std::map> m_assets_to_monitor; - std::mutex m_assets_lock; + + // Use a recursive mutex to handle the scenario where an asset goes out of scope while + // iterating over the assets to monitor which calls the lock above in 'LoadOrCreateAsset' + std::recursive_mutex m_asset_load_lock; Common::WorkQueueThread> m_asset_load_thread; }; } // namespace VideoCommon diff --git a/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.cpp b/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.cpp index f65ea9ccc9..68d63a2466 100644 --- a/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.cpp +++ b/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.cpp @@ -27,6 +27,7 @@ std::size_t GetAssetSize(const CustomTextureData& data) CustomAssetLibrary::TimeType DirectFilesystemAssetLibrary::GetLastAssetWriteTime(const AssetID& asset_id) const { + std::lock_guard lk(m_lock); if (auto iter = m_assetid_to_asset_map_path.find(asset_id); iter != m_assetid_to_asset_map_path.end()) { @@ -47,50 +48,47 @@ DirectFilesystemAssetLibrary::GetLastAssetWriteTime(const AssetID& asset_id) con CustomAssetLibrary::LoadInfo DirectFilesystemAssetLibrary::LoadTexture(const AssetID& asset_id, CustomTextureData* data) { - if (auto iter = m_assetid_to_asset_map_path.find(asset_id); - iter != m_assetid_to_asset_map_path.end()) + const auto asset_map = GetAssetMapForID(asset_id); + + // Raw texture is expected to have one asset mapped + if (asset_map.empty() || asset_map.size() > 1) + return {}; + const auto& asset_path = asset_map.begin()->second; + + const auto last_loaded_time = std::filesystem::last_write_time(asset_path); + auto ext = asset_path.extension().string(); + Common::ToLower(&ext); + if (ext == ".dds") { - const auto& asset_map_path = iter->second; - - // Raw texture is expected to have one asset mapped - if (asset_map_path.empty() || asset_map_path.size() > 1) + LoadDDSTexture(data, asset_path.string()); + if (data->m_levels.empty()) [[unlikely]] + return {}; + if (!LoadMips(asset_path, data)) return {}; - const auto& asset_path = asset_map_path.begin()->second; - const auto last_loaded_time = std::filesystem::last_write_time(asset_path); - auto ext = asset_path.extension().string(); - Common::ToLower(&ext); - if (ext == ".dds") - { - LoadDDSTexture(data, asset_path.string()); - if (data->m_levels.empty()) [[unlikely]] - return {}; - if (!LoadMips(asset_path, data)) - return {}; + return LoadInfo{GetAssetSize(*data), last_loaded_time}; + } + else if (ext == ".png") + { + // If we have no levels, create one to pass into LoadPNGTexture + if (data->m_levels.empty()) + data->m_levels.push_back({}); - return LoadInfo{GetAssetSize(*data), last_loaded_time}; - } - else if (ext == ".png") - { - // If we have no levels, create one to pass into LoadPNGTexture - if (data->m_levels.empty()) - data->m_levels.push_back({}); + if (!LoadPNGTexture(&data->m_levels[0], asset_path.string())) + return {}; + if (!LoadMips(asset_path, data)) + return {}; - if (!LoadPNGTexture(&data->m_levels[0], asset_path.string())) - return {}; - if (!LoadMips(asset_path, data)) - return {}; - - return LoadInfo{GetAssetSize(*data), last_loaded_time}; - } + return LoadInfo{GetAssetSize(*data), last_loaded_time}; } return {}; } -void DirectFilesystemAssetLibrary::SetAssetIDMapData( - const AssetID& asset_id, std::map asset_path_map) +void DirectFilesystemAssetLibrary::SetAssetIDMapData(const AssetID& asset_id, + AssetMap asset_path_map) { + std::lock_guard lk(m_lock); m_assetid_to_asset_map_path[asset_id] = std::move(asset_path_map); } @@ -145,4 +143,16 @@ bool DirectFilesystemAssetLibrary::LoadMips(const std::filesystem::path& asset_p return true; } + +DirectFilesystemAssetLibrary::AssetMap +DirectFilesystemAssetLibrary::GetAssetMapForID(const AssetID& asset_id) const +{ + std::lock_guard lk(m_lock); + if (auto iter = m_assetid_to_asset_map_path.find(asset_id); + iter != m_assetid_to_asset_map_path.end()) + { + return iter->second; + } + return {}; +} } // namespace VideoCommon diff --git a/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.h b/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.h index d04981dac3..4f02d107cf 100644 --- a/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.h +++ b/Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "VideoCommon/Assets/CustomAssetLibrary.h" @@ -18,6 +19,8 @@ class CustomTextureData; class DirectFilesystemAssetLibrary final : public CustomAssetLibrary { public: + using AssetMap = std::map; + LoadInfo LoadTexture(const AssetID& asset_id, CustomTextureData* data) override; // Gets the latest time from amongst all the files in the asset map @@ -26,12 +29,16 @@ public: // Assigns the asset id to a map of files, how this map is read is dependent on the data // For instance, a raw texture would expect the map to have a single entry and load that // file as the asset. But a model file data might have its data spread across multiple files - void SetAssetIDMapData(const AssetID& asset_id, - std::map asset_path_map); + void SetAssetIDMapData(const AssetID& asset_id, AssetMap asset_path_map); private: // Loads additional mip levels into the texture structure until _mip texture is not found bool LoadMips(const std::filesystem::path& asset_path, CustomTextureData* data); + + // Gets the asset map given an asset id + AssetMap GetAssetMapForID(const AssetID& asset_id) const; + + mutable std::mutex m_lock; std::map> m_assetid_to_asset_map_path; }; } // namespace VideoCommon diff --git a/Source/Core/VideoCommon/Assets/TextureAsset.cpp b/Source/Core/VideoCommon/Assets/TextureAsset.cpp index 21f897fc19..0715dc6492 100644 --- a/Source/Core/VideoCommon/Assets/TextureAsset.cpp +++ b/Source/Core/VideoCommon/Assets/TextureAsset.cpp @@ -9,31 +9,35 @@ namespace VideoCommon { CustomAssetLibrary::LoadInfo RawTextureAsset::LoadImpl(const CustomAssetLibrary::AssetID& asset_id) { - std::lock_guard lk(m_lock); auto potential_data = std::make_shared(); const auto loaded_info = m_owning_library->LoadTexture(asset_id, potential_data.get()); if (loaded_info.m_bytes_loaded == 0) return {}; - m_loaded = true; - m_data = std::move(potential_data); + { + std::lock_guard lk(m_data_lock); + m_loaded = true; + m_data = std::move(potential_data); + } return loaded_info; } CustomAssetLibrary::LoadInfo GameTextureAsset::LoadImpl(const CustomAssetLibrary::AssetID& asset_id) { - std::lock_guard lk(m_lock); auto potential_data = std::make_shared(); const auto loaded_info = m_owning_library->LoadGameTexture(asset_id, potential_data.get()); if (loaded_info.m_bytes_loaded == 0) return {}; - m_loaded = true; - m_data = std::move(potential_data); + { + std::lock_guard lk(m_data_lock); + m_loaded = true; + m_data = std::move(potential_data); + } return loaded_info; } bool GameTextureAsset::Validate(u32 native_width, u32 native_height) const { - std::lock_guard lk(m_lock); + std::lock_guard lk(m_data_lock); if (!m_loaded) {