From cca71d03831693dc2c60ff82dc182d86cddba77b Mon Sep 17 00:00:00 2001 From: Jie Yao Date: Wed, 20 Sep 2023 06:54:07 -0700 Subject: [PATCH] format code and fix comments --- src/lib/homestore/heap_chunk_selector.cpp | 90 ++++++------ src/lib/homestore/heap_chunk_selector.h | 29 ++-- src/lib/homestore/tests/CMakeLists.txt | 5 + .../tests/test_heap_chunk_selector.cpp | 132 ++++++++++++++++++ 4 files changed, 194 insertions(+), 62 deletions(-) create mode 100644 src/lib/homestore/tests/test_heap_chunk_selector.cpp diff --git a/src/lib/homestore/heap_chunk_selector.cpp b/src/lib/homestore/heap_chunk_selector.cpp index 53773073..535b254f 100644 --- a/src/lib/homestore/heap_chunk_selector.cpp +++ b/src/lib/homestore/heap_chunk_selector.cpp @@ -6,70 +6,70 @@ namespace homeobject { -//https://github.com/facebook/folly/blob/61c11d77eb9a8bdc60f673017fccfbe900125cb6/folly/AtomicHashMap.h#L42 -//AtomichashMap has a max size limit of ~18x initial size. - -HeapChunkSelector::HeapChunkSelector() : -m_pdev_heap_map(PdevHeapMap(pdev_atomicmap_init_size)), -m_pdev_avalable_blk_map(pdev_atomicmap_init_size), -m_chunks(chunk_atomicmap_init_size) {} - -HeapChunkSelector::HeapChunkSelector(const uint32_t& pdev_heap_map_initial_size, -const uint32_t& pdev_avalable_blk_map_initial_size, const uint32_t& chunk_initial_num) : -m_pdev_heap_map(PdevHeapMap(pdev_heap_map_initial_size)), -m_pdev_avalable_blk_map(PdevAvalableBlkMap(pdev_avalable_blk_map_initial_size)), -m_chunks(ChunkMap(chunk_initial_num)) {} - - -//add_chunk may be called in Homeobject(when sealing a shard) or in Homestore(when adding chunks to vdev) concurently +// https://github.com/facebook/folly/blob/61c11d77eb9a8bdc60f673017fccfbe900125cb6/folly/AtomicHashMap.h#L42 +// AtomichashMap has a max size limit of ~18x initial size. + +HeapChunkSelector::HeapChunkSelector() : + m_pdev_heap_map(PdevHeapMap(pdev_atomicmap_init_size)), + m_pdev_avalable_blk_map(pdev_atomicmap_init_size), + m_chunks(chunk_atomicmap_init_size) {} + +HeapChunkSelector::HeapChunkSelector(const uint32_t& pdev_heap_map_initial_size, + const uint32_t& pdev_avalable_blk_map_initial_size, + const uint32_t& chunk_initial_num) : + m_pdev_heap_map(PdevHeapMap(pdev_heap_map_initial_size)), + m_pdev_avalable_blk_map(PdevAvalableBlkMap(pdev_avalable_blk_map_initial_size)), + m_chunks(ChunkMap(chunk_initial_num)) {} + +// add_chunk may be called in Homeobject(when sealing a shard) or in Homestore(when adding chunks to vdev) concurently void HeapChunkSelector::add_chunk(csharedChunk& chunk) { VChunk vchunk(chunk); auto&& pdevID = vchunk.get_pdev_id(); - auto&& [it, _] = m_pdev_heap_map.emplace(pdevID, std::make_shared>()); + auto&& [it, _] = m_pdev_heap_map.emplace(pdevID, std::make_shared< std::pair< std::mutex, VChunkHeap > >()); const auto& available_blk_num = vchunk.available_blks(); auto&& [ignore, happened] = m_pdev_avalable_blk_map.emplace(pdevID, available_blk_num); - if(!happened) { - auto&& avalableBlkCounter = m_pdev_avalable_blk_map.find(pdevID)->second; + if (!happened) { + auto& avalableBlkCounter = m_pdev_avalable_blk_map.find(pdevID)->second; avalableBlkCounter.fetch_add(available_blk_num); } - + m_chunks.emplace(vchunk.get_chunk_id(), chunk); auto& heapLock = it->second->first; auto& heap = it->second->second; - std::lock_guard l(heapLock); + std::lock_guard< std::mutex > l(heapLock); heap.emplace(chunk); } -//select_chunk will only be called in homestore when creating a shard. +// select_chunk will only be called in homestore when creating a shard. csharedChunk HeapChunkSelector::select_chunk(homestore::blk_count_t count, const homestore::blk_alloc_hints& hint) { auto& chunkIdHint = hint.chunk_id_hint; - if(chunkIdHint.has_value()) { + if (chunkIdHint.has_value()) { LOGINFO("should not allocated a chunk with exiting chunk_id {} in hint!", chunkIdHint.value()); return nullptr; } - //shardid -> chunkid map is maintained by ShardManager - //pg_id->pdev_id map is maintained by PgManager - //chunselector will not take care of the two maps for now. + // shardid -> chunkid map is maintained by ShardManager + // pg_id->pdev_id map is maintained by PgManager + // chunselector will not take care of the two maps for now. uint32_t pdevID = 0; auto& pdevIdHint = hint.pdev_id_hint; - if(!pdevIdHint.has_value()) { + if (!pdevIdHint.has_value()) { // this is the first shard of this pg, select a pdev with the most available blocks for it - auto&& it = std::max_element(m_pdev_avalable_blk_map.begin(), m_pdev_avalable_blk_map.end(), - [](const std::pair& lhs, const std::pair& rhs) { - return lhs.second < rhs.second; } - ); - if(it == m_pdev_avalable_blk_map.end()) { + auto&& it = + std::max_element(m_pdev_avalable_blk_map.begin(), m_pdev_avalable_blk_map.end(), + [](const std::pair< uint32_t, uint32_t >& lhs, + const std::pair< uint32_t, uint32_t >& rhs) { return lhs.second < rhs.second; }); + if (it == m_pdev_avalable_blk_map.end()) { LOGINFO("No pdev found for new pg"); return nullptr; } pdevID = it->first; - }else{ + } else { pdevID = pdevIdHint.value(); - } + } auto it = m_pdev_heap_map.find(pdevID); if (it == m_pdev_heap_map.end()) { @@ -80,14 +80,14 @@ csharedChunk HeapChunkSelector::select_chunk(homestore::blk_count_t count, const const auto& vchunk = [it = std::move(it), pdevID]() { auto& heapLock = it->second->first; auto& heap = it->second->second; - std::lock_guard l(heapLock); - if(heap.empty()) return VChunk(nullptr); - const auto& vchunk = heap.top(); + std::lock_guard< std::mutex > l(heapLock); + if (heap.empty()) return VChunk(nullptr); + VChunk vchunk(heap.top().get_internal_chunk()); heap.pop(); return vchunk; }(); - if(vchunk.get_internal_chunk()){ + if (vchunk.get_internal_chunk()) { auto&& avalableBlkCounter = m_pdev_avalable_blk_map.find(pdevID)->second; avalableBlkCounter.fetch_sub(vchunk.available_blks()); } else { @@ -98,19 +98,17 @@ csharedChunk HeapChunkSelector::select_chunk(homestore::blk_count_t count, const } void HeapChunkSelector::foreach_chunks(std::function< void(csharedChunk&) >&& cb) { - //we should call `cb` on all the chunks, selected or not - //actually, after vdev is initialized, no more new chunks will be added again, - //so we can make this assumption that when this is called - std::for_each(std::execution::par_unseq, m_chunks.begin(), m_chunks.end(), - [cb = std::move(cb)](auto& p){ cb(p.second); }); + // we should call `cb` on all the chunks, selected or not + std::for_each(std::execution::par_unseq, m_chunks.begin(), m_chunks.end(), + [cb = std::move(cb)](auto& p) { cb(p.second); }); } -void HeapChunkSelector::release_chunk(const uint32_t& chunkID) { +void HeapChunkSelector::release_chunk(const uint32_t chunkID) { const auto& it = m_chunks.find(chunkID); - if(it == m_chunks.end()) { + if (it == m_chunks.end()) { LOGWARN("No chunk found for ChunkID {}", chunkID); } else { add_chunk(it->second); } } -} // namespace homeobject \ No newline at end of file +} // namespace homeobject \ No newline at end of file diff --git a/src/lib/homestore/heap_chunk_selector.h b/src/lib/homestore/heap_chunk_selector.h index 768f4e1c..b0c81a3b 100644 --- a/src/lib/homestore/heap_chunk_selector.h +++ b/src/lib/homestore/heap_chunk_selector.h @@ -16,7 +16,7 @@ namespace homeobject { -using csharedChunk = homestore::cshared; +using csharedChunk = homestore::cshared< homestore::Chunk >; class HeapChunkSelector : public homestore::ChunkSelector { public: @@ -27,39 +27,36 @@ class HeapChunkSelector : public homestore::ChunkSelector { using VChunk = homestore::VChunk; class VChunkComparator { public: - bool operator()(VChunk& lhs, VChunk& rhs) { - return lhs.available_blks() < rhs.available_blks(); - } + bool operator()(VChunk& lhs, VChunk& rhs) { return lhs.available_blks() < rhs.available_blks(); } }; - using VChunkHeap = std::priority_queue, VChunkComparator>; + using VChunkHeap = std::priority_queue< VChunk, std::vector< VChunk >, VChunkComparator >; void add_chunk(csharedChunk&) override; void foreach_chunks(std::function< void(csharedChunk&) >&& cb) override; - csharedChunk select_chunk([[maybe_unused]]homestore::blk_count_t nblks, const homestore::blk_alloc_hints& hints); + csharedChunk select_chunk([[maybe_unused]] homestore::blk_count_t nblks, const homestore::blk_alloc_hints& hints); - // this function is used to return a chunk back to ChunkSelector when sealing a shard, and will only be used by Homeobject. - void release_chunk(const uint32_t&); + // this function is used to return a chunk back to ChunkSelector when sealing a shard, and will only be used by + // Homeobject. + void release_chunk(const uint32_t); - // we use 64K for the initial size - static constexpr uint32_t chunk_atomicmap_init_size = 1<<16; + // we use 64K for the initial size + static constexpr uint32_t chunk_atomicmap_init_size = 1 << 16; // we suppose our max pdev number is 64(64 physical disks) - static constexpr uint32_t pdev_atomicmap_init_size = 1<<6; + static constexpr uint32_t pdev_atomicmap_init_size = 1 << 6; using PdevHeapMap = folly::AtomicHashMap< uint32_t, std::shared_ptr< std::pair< std::mutex, VChunkHeap > > >; - using PdevAvalableBlkMap = folly::AtomicHashMap< uint32_t, std::atomic_uint32_t >; + using PdevAvalableBlkMap = folly::AtomicHashMap< uint32_t, std::atomic_size_t >; using ChunkMap = folly::AtomicHashMap< uint32_t, csharedChunk >; private: - //this holds all the unselected chunks for each pdev. + // this holds all the unselected chunks for each pdev. PdevHeapMap m_pdev_heap_map; - //for now, uint32_t is enough for the sum of all the available blocks of a pdev. - //if necessary , we can change this to uint64_t to hold a larger sum. PdevAvalableBlkMap m_pdev_avalable_blk_map; - //hold all the chunks , selected or not + // hold all the chunks , selected or not ChunkMap m_chunks; }; } // namespace homeobject \ No newline at end of file diff --git a/src/lib/homestore/tests/CMakeLists.txt b/src/lib/homestore/tests/CMakeLists.txt index e6134b86..114b8afc 100644 --- a/src/lib/homestore/tests/CMakeLists.txt +++ b/src/lib/homestore/tests/CMakeLists.txt @@ -19,3 +19,8 @@ target_link_libraries(test_shard_manager ) add_test(NAME ShardManagerTest COMMAND ${CMAKE_BINARY_DIR}/bin/test_shard_manager) +add_executable (test_heap_chunk_selector) +target_sources(test_heap_chunk_selector PRIVATE test_heap_chunk_selector.cpp ../heap_chunk_selector.cpp) +target_link_libraries(test_heap_chunk_selector ${COMMON_TEST_DEPS}) +add_test(NAME HeapChunkSelectorTest COMMAND ${CMAKE_BINARY_DIR}/bin/test_heap_chunk_selector) + diff --git a/src/lib/homestore/tests/test_heap_chunk_selector.cpp b/src/lib/homestore/tests/test_heap_chunk_selector.cpp new file mode 100644 index 00000000..0d2d13fa --- /dev/null +++ b/src/lib/homestore/tests/test_heap_chunk_selector.cpp @@ -0,0 +1,132 @@ +#include "lib/homestore/heap_chunk_selector.h" + +#include + +#include +#include +#include + +#include + +SISL_LOGGING_INIT(logging, HOMEOBJECT_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) + +namespace homestore { +class Chunk : public std::enable_shared_from_this< Chunk > { +public: + uint32_t available_blks() const { return m_available_blks; } + + void set_available_blks(uint32_t available_blks) { m_available_blks = available_blks; } + + uint32_t get_pdev_id() const { return m_pdev_id; } + + void set_pdev_id(uint32_t pdev_id) { m_pdev_id = pdev_id; } + + uint16_t get_chunk_id() const { return m_chunk_id; } + + void set_chunk_id(uint16_t chunk_id) { m_chunk_id = chunk_id; } + const std::shared_ptr< Chunk > get_internal_chunk() { return shared_from_this(); } + + Chunk(uint32_t pdev_id, uint16_t chunk_id, uint32_t available_blks) { + m_available_blks = available_blks; + m_pdev_id = pdev_id; + m_chunk_id = chunk_id; + } + +private: + uint32_t m_available_blks; + uint32_t m_pdev_id; + uint16_t m_chunk_id; +}; + +VChunk::VChunk(cshared< Chunk >& chunk) : m_internal_chunk(chunk) {} + +void VChunk::set_user_private(const sisl::blob& data) {} + +const uint8_t* VChunk::get_user_private() const { return nullptr; }; + +blk_num_t VChunk::available_blks() const { return m_internal_chunk->available_blks(); } + +uint32_t VChunk::get_pdev_id() const { return m_internal_chunk->get_pdev_id(); } + +uint16_t VChunk::get_chunk_id() const { return m_internal_chunk->get_chunk_id(); } + +cshared< Chunk > VChunk::get_internal_chunk() const { return m_internal_chunk->get_internal_chunk(); } + +} // namespace homestore + +using homeobject::csharedChunk; +using homeobject::HeapChunkSelector; +using homestore::Chunk; + +class HeapChunkSelectorTest : public ::testing::Test { +protected: + void SetUp() override { + HCS.add_chunk(std::make_shared< Chunk >(1, 1, 1)); + HCS.add_chunk(std::make_shared< Chunk >(1, 2, 2)); + HCS.add_chunk(std::make_shared< Chunk >(1, 3, 3)); + HCS.add_chunk(std::make_shared< Chunk >(2, 4, 1)); + HCS.add_chunk(std::make_shared< Chunk >(2, 5, 2)); + HCS.add_chunk(std::make_shared< Chunk >(2, 6, 3)); + HCS.add_chunk(std::make_shared< Chunk >(3, 7, 1)); + HCS.add_chunk(std::make_shared< Chunk >(3, 8, 2)); + HCS.add_chunk(std::make_shared< Chunk >(3, 9, 3)); + }; + +public: + HeapChunkSelector HCS; +}; + +TEST_F(HeapChunkSelectorTest, test_for_each_chunk) { + std::atomic_uint32_t size; + HCS.foreach_chunks([&size](csharedChunk& chunk) { size.fetch_add(chunk->available_blks()); }); + ASSERT_EQ(size.load(), 18); +} + +TEST_F(HeapChunkSelectorTest, test_select_chunk) { + homestore::blk_count_t count = 1; + homestore::blk_alloc_hints hints; + for (uint32_t i = 1; i < 4; i++) { + hints.pdev_id_hint = i; + for (int j = 3; j > 0; j--) { + auto chunk = HCS.select_chunk(count, hints); + ASSERT_EQ(chunk->get_pdev_id(), i); + EXPECT_EQ(chunk->available_blks(), j); + } + } +} + +TEST_F(HeapChunkSelectorTest, test_release_chunk) { + homestore::blk_count_t count = 1; + homestore::blk_alloc_hints hints; + hints.pdev_id_hint = 1; + auto chunk1 = HCS.select_chunk(count, hints); + ASSERT_EQ(chunk1->get_pdev_id(), 1); + EXPECT_EQ(chunk1->available_blks(), 3); + + auto chunk2 = HCS.select_chunk(count, hints); + ASSERT_EQ(chunk2->get_pdev_id(), 1); + EXPECT_EQ(chunk2->available_blks(), 2); + + HCS.release_chunk(chunk1->get_chunk_id()); + HCS.release_chunk(chunk2->get_chunk_id()); + + chunk1 = HCS.select_chunk(count, hints); + ASSERT_EQ(chunk1->get_pdev_id(), 1); + EXPECT_EQ(chunk1->available_blks(), 3); + + chunk2 = HCS.select_chunk(count, hints); + ASSERT_EQ(chunk2->get_pdev_id(), 1); + EXPECT_EQ(chunk2->available_blks(), 2); +} + +int main(int argc, char* argv[]) { + int parsed_argc = argc; + ::testing::InitGoogleTest(&parsed_argc, argv); + SISL_OPTIONS_LOAD(parsed_argc, argv, logging); + sisl::logging::SetLogger(std::string(argv[0])); + spdlog::set_pattern("[%D %T.%e] [%n] [%^%l%$] [%t] %v"); + parsed_argc = 1; + auto f = ::folly::Init(&parsed_argc, &argv, true); + return RUN_ALL_TESTS(); +} \ No newline at end of file