From: Pawel Wasowski Date: Tue, 2 Feb 2021 09:58:52 +0000 (+0100) Subject: [ML][common] Add synchronization of access to tensor info/data maps X-Git-Tag: submit/tizen/20210217.032056~3 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3a008e651c0a9c202224a4284c3333e5330d3358;p=platform%2Fcore%2Fapi%2Fwebapi-plugins.git [ML][common] Add synchronization of access to tensor info/data maps ACR: TWDAPI-273/TWDAPI-274 Pipeline's SinkCallbacks and CustomFilters may be called from threads other than the main thread. As they access members of Tensors{Data, Info}Managers, the access to these members should be synchronized to prevent 2 threads from modifying the same data concurrently. [Verification] Tested with code snippets used from other commits implementing Tensors{Data, Info} or Pipeline - the snippets still work. The snippets used for tests were found in commit messages of these changes: https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251435 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251699 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251524 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252001 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252002 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252289 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252290 https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252482 Change-Id: I32f5df4ba92e86034c78d751e901c77a9d60ca4e Signed-off-by: Pawel Wasowski --- diff --git a/src/ml/ml_instance.h b/src/ml/ml_instance.h index e905bfde..d790b3b4 100644 --- a/src/ml/ml_instance.h +++ b/src/ml/ml_instance.h @@ -60,6 +60,10 @@ class MlInstance : public common::ParsedInstance { void MLTensorsDataGetTensorType(const picojson::value& args, picojson::object& out); void MLTensorsDataSetTensorRawData(const picojson::value& args, picojson::object& out); + /* + * ########## IMPORTANT ########## + * Before changing these lines, see the comment above pipeline_manager_. + */ TensorsInfoManager tensors_info_manager_; TensorsDataManager tensors_data_manager_; // Common ML API end @@ -79,6 +83,28 @@ class MlInstance : public common::ParsedInstance { // Single API end // Pipeline API begin + /* + * ########## IMPORTANT ########## + * Ensure, that pipeline_manager_ field appears AFTER tensors_info_manager_ + * and tensors_data_manager_. This means, that when ~MlInstance() is called, + * the pipeline_manager_ is destroyed BEFORE the managers. This should + * prevent the risk of race conditions, which occur in + * the following scenario: + * 1. SinkListener or CustomFilter are registered for a pipeline. Note, that + * they are triggered by callbacks, which run in threads other than the main. + * 2. Pipeline is running and callbacks called in different threads + * use Tensor{Info, Data} objects, managed by tensor_{info, data}_manager_s. + * 3. The application exits or is reloaded. + * + * If managers would be destroyed before the pipeline, to which + * SinkListener/CustomFilter belongs, the callbacks (running in not-main + * threads) could still have pointers to freed Tensors{Info, Data}. + * + * When pipeline_manager_ is destroyed before + * tensors_{info, data}_manager_s, the Pipeline is destroyed and its + * SinkListeners/CustomFilters are unregistered when Tensors{Info, Data} are + * still valid. + */ PipelineManager pipeline_manager_; // PipelineManager::createPipeline() begin diff --git a/src/ml/ml_tensors_data_manager.cc b/src/ml/ml_tensors_data_manager.cc index 77711b9d..0551073c 100644 --- a/src/ml/ml_tensors_data_manager.cc +++ b/src/ml/ml_tensors_data_manager.cc @@ -320,6 +320,8 @@ TensorsDataManager::TensorsDataManager() : nextId_(0) { TensorsDataManager::~TensorsDataManager() { ScopeLogger(); + + std::lock_guard lock{map_and_next_id_mutex_}; map_.clear(); }; @@ -337,6 +339,7 @@ TensorsData* TensorsDataManager::CreateTensorsData(TensorsInfo* tensors_info) { return nullptr; } + std::lock_guard lock{map_and_next_id_mutex_}; int id = nextId_++; auto t = std::make_unique(tensors_data_handle, id, tensors_info); map_[id] = std::move(t); @@ -347,6 +350,7 @@ TensorsData* TensorsDataManager::CreateTensorsData(TensorsInfo* tensors_info) { TensorsData* TensorsDataManager::GetTensorsData(int id) { ScopeLogger("id: %d", id); + std::lock_guard lock{map_and_next_id_mutex_}; if (map_.end() != map_.find(id)) { return map_[id].get(); } @@ -370,6 +374,7 @@ PlatformResult TensorsDataManager::DisposeTensorsData(TensorsData* t) { return PlatformResult(ErrorCode::ABORT_ERR); } + std::lock_guard lock{map_and_next_id_mutex_}; map_.erase(t->Id()); return PlatformResult(ErrorCode::NO_ERROR); diff --git a/src/ml/ml_tensors_data_manager.h b/src/ml/ml_tensors_data_manager.h index 2ebc82e3..7b955a56 100644 --- a/src/ml/ml_tensors_data_manager.h +++ b/src/ml/ml_tensors_data_manager.h @@ -17,6 +17,7 @@ #ifndef __ML_TENSORS_DATA_MANAGER_H__ #define __ML_TENSORS_DATA_MANAGER_H__ +#include #include #include "common/logger.h" @@ -108,6 +109,12 @@ class TensorsDataManager { TensorsDataManager(TensorsDataManager const&) = delete; TensorsDataManager& operator=(TensorsDataManager const&) = delete; + /* + * For performance reasons and simplicity we use a single mutex + * to lock map_ and nextId_. They are often used together + * and we'd have to lock all of them anyway. + */ + std::mutex map_and_next_id_mutex_; std::unordered_map> map_; int nextId_; }; diff --git a/src/ml/ml_tensors_info_manager.cc b/src/ml/ml_tensors_info_manager.cc index 912987d6..80c03a2a 100644 --- a/src/ml/ml_tensors_info_manager.cc +++ b/src/ml/ml_tensors_info_manager.cc @@ -318,6 +318,7 @@ TensorsInfoManager::TensorsInfoManager(TensorsDataManager* tensors_data_manager) TensorsInfoManager::~TensorsInfoManager() { ScopeLogger(); + std::lock_guard lock{maps_and_next_id_mutex_}; map_by_id_.clear(); map_by_handle_.clear(); }; @@ -332,6 +333,7 @@ TensorsInfo* TensorsInfoManager::CreateTensorsInfo() { return nullptr; } + std::lock_guard lock{maps_and_next_id_mutex_}; int id = nextId_++; auto t = std::make_shared(handle, id); map_by_id_[id] = t; @@ -343,8 +345,10 @@ TensorsInfo* TensorsInfoManager::CreateTensorsInfo() { TensorsInfo* TensorsInfoManager::CreateTensorsInfo(ml_tensors_info_h handle) { ScopeLogger(); + std::lock_guard lock{maps_and_next_id_mutex_}; int id = nextId_++; auto t = std::make_shared(handle, id); + map_by_id_[id] = t; map_by_handle_[handle] = t; @@ -358,6 +362,7 @@ TensorsInfo* TensorsInfoManager::CloneTensorsInfo(TensorsInfo* src) { return nullptr; } + std::lock_guard lock{maps_and_next_id_mutex_}; int id = nextId_++; auto t = src->CreateClone(id); if (nullptr == t) { @@ -373,6 +378,7 @@ TensorsInfo* TensorsInfoManager::CloneTensorsInfo(TensorsInfo* src) { TensorsInfo* TensorsInfoManager::GetTensorsInfo(int id) { ScopeLogger("id: %d", id); + std::lock_guard lock{maps_and_next_id_mutex_}; if (map_by_id_.end() != map_by_id_.find(id)) { return map_by_id_[id].get(); } @@ -383,6 +389,7 @@ TensorsInfo* TensorsInfoManager::GetTensorsInfo(int id) { TensorsInfo* TensorsInfoManager::GetTensorsInfo(ml_tensors_info_h handle) { ScopeLogger(); + std::lock_guard lock{maps_and_next_id_mutex_}; if (map_by_handle_.end() != map_by_handle_.find(handle)) { return map_by_handle_[handle].get(); } @@ -413,6 +420,7 @@ PlatformResult TensorsInfoManager::DisposeTensorsInfo(TensorsInfo* t) { return PlatformResult(ErrorCode::ABORT_ERR); } + std::lock_guard lock{maps_and_next_id_mutex_}; map_by_handle_.erase(t->Handle()); map_by_id_.erase(t->Id()); diff --git a/src/ml/ml_tensors_info_manager.h b/src/ml/ml_tensors_info_manager.h index 96531f9f..2a36d05a 100644 --- a/src/ml/ml_tensors_info_manager.h +++ b/src/ml/ml_tensors_info_manager.h @@ -18,6 +18,7 @@ #define __ML_TENSORS_INFO_MANAGER_H__ #include +#include #include #include "common/logger.h" @@ -89,6 +90,13 @@ class TensorsInfoManager { private: TensorsInfoManager(TensorsInfoManager const&) = delete; TensorsInfoManager& operator=(TensorsInfoManager const&) = delete; + + /* + * For performance reasons and simplicity we use a single mutex + * to lock both maps and nextId_. They are often used together + * and we'd have to lock all of them anyway. + */ + std::mutex maps_and_next_id_mutex_; std::map> map_by_id_; std::map> map_by_handle_; int nextId_;