format code and fix comments

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<std::pair<std::mutex, VChunkHeap>>());
+    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<std::mutex> 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<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()) {
+        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<std::mutex> 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
+} // namespace homeobject

src/lib/homestore/heap_chunk_selector.h

@@ -16,7 +16,7 @@
 
 namespace homeobject {
 
-using csharedChunk = homestore::cshared<homestore::Chunk>;  
+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<VChunk, std::vector<VChunk>, 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

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)
+

src/lib/homestore/tests/test_heap_chunk_selector.cpp

@@ -0,0 +1,132 @@
+#include "lib/homestore/heap_chunk_selector.h"
+
+#include <gtest/gtest.h>
+
+#include <sisl/options/options.h>
+#include <sisl/logging/logging.h>
+#include <folly/init/Init.h>
+
+#include <memory>
+
+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();
+}