From a48a398e7c05e3930eda71a903d19859f32f6502 Mon Sep 17 00:00:00 2001 From: Aliaksei Sandryhaila Date: Thu, 28 Sep 2017 14:22:06 -0700 Subject: [PATCH] Use RAII instead of pointers in cf_info_map Summary: There is no need for smart pointers in cf_info_map, so use RAII. This should also placate valgrind. Closes https://github.com/facebook/rocksdb/pull/2943 Differential Revision: D5932941 Pulled By: asandryh fbshipit-source-id: 2c37df88573a9df2557880a31193926e4425e054 --- monitoring/thread_status_updater.cc | 58 +++++++++-------------- monitoring/thread_status_updater.h | 3 +- monitoring/thread_status_updater_debug.cc | 3 +- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/monitoring/thread_status_updater.cc b/monitoring/thread_status_updater.cc index 7441c35f8..67a42be16 100644 --- a/monitoring/thread_status_updater.cc +++ b/monitoring/thread_status_updater.cc @@ -177,19 +177,15 @@ Status ThreadStatusUpdater::GetThreadList( // use "memory_order_relaxed" to load the cf_key. auto cf_key = thread_data->cf_key.load( std::memory_order_relaxed); - auto iter = cf_info_map_.find(cf_key); - auto* cf_info = iter != cf_info_map_.end() ? - iter->second.get() : nullptr; - const std::string* db_name = nullptr; - const std::string* cf_name = nullptr; + ThreadStatus::OperationType op_type = ThreadStatus::OP_UNKNOWN; ThreadStatus::OperationStage op_stage = ThreadStatus::STAGE_UNKNOWN; ThreadStatus::StateType state_type = ThreadStatus::STATE_UNKNOWN; uint64_t op_elapsed_micros = 0; uint64_t op_props[ThreadStatus::kNumOperationProperties] = {0}; - if (cf_info != nullptr) { - db_name = &cf_info->db_name; - cf_name = &cf_info->cf_name; + + auto iter = cf_info_map_.find(cf_key); + if (iter != cf_info_map_.end()) { op_type = thread_data->operation_type.load( std::memory_order_acquire); // display lower-level info only when higher-level info is available. @@ -206,10 +202,11 @@ Status ThreadStatusUpdater::GetThreadList( } } } + thread_list->emplace_back( thread_id, thread_type, - db_name ? *db_name : "", - cf_name ? *cf_name : "", + iter != cf_info_map_.end() ? iter->second.db_name : "", + iter != cf_info_map_.end() ? iter->second.cf_name : "", op_type, op_elapsed_micros, op_stage, op_props, state_type); } @@ -236,8 +233,9 @@ void ThreadStatusUpdater::NewColumnFamilyInfo( // a consistent view of global column family table (cf_info_map). std::lock_guard lck(thread_list_mutex_); - cf_info_map_[cf_key].reset( - new ConstantColumnFamilyInfo(db_key, db_name, cf_name)); + cf_info_map_.emplace(std::piecewise_construct, + std::make_tuple(cf_key), + std::make_tuple(db_key, db_name, cf_name)); db_key_map_[db_key].insert(cf_key); } @@ -245,25 +243,19 @@ void ThreadStatusUpdater::EraseColumnFamilyInfo(const void* cf_key) { // Acquiring same lock as GetThreadList() to guarantee // a consistent view of global column family table (cf_info_map). std::lock_guard lck(thread_list_mutex_); + auto cf_pair = cf_info_map_.find(cf_key); - if (cf_pair == cf_info_map_.end()) { - return; + if (cf_pair != cf_info_map_.end()) { + // Remove its entry from db_key_map_ by the following steps: + // 1. Obtain the entry in db_key_map_ whose set contains cf_key + // 2. Remove it from the set. + ConstantColumnFamilyInfo& cf_info = cf_pair->second; + auto db_pair = db_key_map_.find(cf_info.db_key); + assert(db_pair != db_key_map_.end()); + size_t result __attribute__((unused)) = db_pair->second.erase(cf_key); + assert(result); + cf_info_map_.erase(cf_pair); } - - auto* cf_info = cf_pair->second.get(); - assert(cf_info); - - // Remove its entry from db_key_map_ by the following steps: - // 1. Obtain the entry in db_key_map_ whose set contains cf_key - // 2. Remove it from the set. - auto db_pair = db_key_map_.find(cf_info->db_key); - assert(db_pair != db_key_map_.end()); - size_t result __attribute__((unused)) = db_pair->second.erase(cf_key); - assert(result); - - cf_pair->second.reset(); - result = cf_info_map_.erase(cf_key); - assert(result); } void ThreadStatusUpdater::EraseDatabaseInfo(const void* db_key) { @@ -277,15 +269,11 @@ void ThreadStatusUpdater::EraseDatabaseInfo(const void* db_key) { return; } - size_t result __attribute__((unused)) = 0; for (auto cf_key : db_pair->second) { auto cf_pair = cf_info_map_.find(cf_key); - if (cf_pair == cf_info_map_.end()) { - continue; + if (cf_pair != cf_info_map_.end()) { + cf_info_map_.erase(cf_pair); } - cf_pair->second.reset(); - result = cf_info_map_.erase(cf_key); - assert(result); } db_key_map_.erase(db_key); } diff --git a/monitoring/thread_status_updater.h b/monitoring/thread_status_updater.h index 69b4d4f7e..6706d159d 100644 --- a/monitoring/thread_status_updater.h +++ b/monitoring/thread_status_updater.h @@ -218,8 +218,7 @@ class ThreadStatusUpdater { // globally instead of inside DB is to avoid the situation where DB is // closing while GetThreadList function already get the pointer to its // CopnstantColumnFamilyInfo. - std::unordered_map< - const void*, std::unique_ptr> cf_info_map_; + std::unordered_map cf_info_map_; // A db_key to cf_key map that allows erasing elements in cf_info_map // associated to the same db_key faster. diff --git a/monitoring/thread_status_updater_debug.cc b/monitoring/thread_status_updater_debug.cc index eec52e188..d4fd4ea23 100644 --- a/monitoring/thread_status_updater_debug.cc +++ b/monitoring/thread_status_updater_debug.cc @@ -24,8 +24,7 @@ void ThreadStatusUpdater::TEST_VerifyColumnFamilyInfoMap( auto iter __attribute__((unused)) = cf_info_map_.find(cfd); if (check_exist) { assert(iter != cf_info_map_.end()); - assert(iter->second); - assert(iter->second->cf_name == cfd->GetName()); + assert(iter->second.cf_name == cfd->GetName()); } else { assert(iter == cf_info_map_.end()); }