CapacityTree bugfix: composite key proper handling

python_tests/test_issues_11.py

@@ -11,7 +11,7 @@
 def test_wide_lock_badaddr_issue(db0_slab_size):
     db0.set_cache_size(8 << 30)
     all_blobs = db0.list()
-    append_count = 300289
+    append_count = 500000
     # append large blobs to force wide locks
     for count in range(append_count):
         all_blobs.append(MemoBlob(24 << 10))
@@ -20,6 +20,22 @@ def test_wide_lock_badaddr_issue(db0_slab_size):
             db0.commit()
             print(f"Appended {count} blobs")
 
-    # faulty operation
-    all_blobs.append(MemoBlob(24 << 10))
-
+
+@pytest.mark.stress_test
+@pytest.mark.parametrize("db0_slab_size", [{"slab_size": 64 << 20, "autocommit": False}], indirect=True)
+def test_wide_lock_badaddr_issue2(db0_slab_size):
+    """
+    Issue: test segfaults with low cache size (on close)
+    Resolution:
+    """
+    db0.set_cache_size(128 << 20)
+    all_blobs = db0.list()
+    append_count = 200000
+    # append large blobs to force wide locks
+    for count in range(append_count):
+        all_blobs.append(MemoBlob(24 << 10))
+        if count % 10000 == 0:
+            print("Commit...")
+            db0.commit()
+            print(f"Appended {count} blobs")
+            print(f"Prefix size: {db0.get_storage_stats()['prefix_size']}")

src/dbzero/bindings/python/PyHash.cpp

@@ -8,7 +8,7 @@
 #include <dbzero/object_model/object/Object.hpp>
 #include <dbzero/object_model/class/ClassFactory.hpp>
 #include <dbzero/object_model/class/Class.hpp>
-#include <dbzero/core/utils/hashes.hpp>
+#include <dbzero/core/utils/hash_func.hpp>
 
 namespace db0::python
 

src/dbzero/core/collections/SGB_Tree/SGB_Key.hpp

@@ -0,0 +1,68 @@
+#pragma once
+
+#include <functional>
+#include "sgb_tree_node.hpp"
+#include "sgb_types.hpp"
+
+namespace db0
+
+{
+
+    // This is a generic wrapper over a simple type key
+    // it demonstrates the SGB_Tree key's requirements and is mostly used for testing purposes
+DB0_PACKED_BEGIN    
+    template <typename TypeT = std::uint64_t>
+    struct DB0_PACKED_ATTR SGB_KeyT
+    {
+        TypeT m_value;
+
+        inline SGB_KeyT(TypeT value) : m_value(value) {}
+        inline SGB_KeyT() = default;
+
+        inline operator TypeT() const {
+            return m_value;
+        }
+
+        static inline TypeT getKey(TypeT value) {
+            return value;
+        }
+
+        // assignment operator
+        inline SGB_KeyT &operator=(const SGB_KeyT &other) {
+            m_value = other.m_value;
+            return *this;
+        }
+
+        template <typename T>
+        inline bool operator==(T other) const {
+            return m_value == other;
+        }
+
+        template <typename T>
+        inline bool operator!=(T other) const {
+            return m_value != other;
+        }
+
+        template <typename T>
+        inline bool operator<(T other) const {
+            return m_value < other;
+        }
+
+        template <typename T>
+        inline bool operator<=(T other) const {
+            return m_value <= other;
+        }
+
+        template <typename T>
+        inline bool operator>(T other) const {
+            return m_value > other;
+        }
+
+        template <typename T>
+        inline bool operator>=(T other) const {
+            return m_value >= other;
+        }
+    };
+DB0_PACKED_END
+
+}

src/dbzero/core/collections/SGB_Tree/SGB_Tree.hpp

@@ -3,6 +3,7 @@
 #include <functional>
 #include "sgb_tree_node.hpp"
 #include "sgb_types.hpp"
+#include "SGB_Key.hpp"
 
 namespace db0
 
@@ -65,9 +66,9 @@ namespace db0
                 : parent_t(it.first, it.second)
             {
             }
-
-            inline operator bool() const {
-                return this->first != nullptr;
+            
+            inline bool isEnd() const {
+                return this->first == nullptr;
             }
 
             inline unsigned int getIndex() const {
@@ -133,15 +134,14 @@ namespace db0
             if (super_t::empty()) {
                 return this->emplace_to_empty(std::forward<Args>(args)...);
             }
-
-            super_t::modify().m_sgb_size++;
-            // assume key is the first element in the args pack
-            // finding an existing node which can hold the new item
-            auto node = super_t::lower_equal_bound(std::get<0>(std::make_tuple(args...)));
+
+            ++super_t::modify().m_sgb_size;
+            // NOTE: assume getKey exists to extract key from construction args
+            auto node = super_t::lower_equal_bound(ItemT::getKey(std::forward<Args>(args)...));
             if (node == super_t::end()) {
                 node = super_t::begin();
             }
-
+            
             if (node->isFull()) {
                 // erase the max element and create the new node
                 auto max_item_ptr = node->find_max(m_heap_comp);
@@ -173,7 +173,7 @@ namespace db0
             if (!node.modify().erase(std::forward<Args>(args)..., m_heap_comp)) {
                 return { false, NodeT() };
             }
-            super_t::modify().m_sgb_size--;
+            --super_t::modify().m_sgb_size;
             if (node->empty()) {
                 // delete the entire node
                 super_t::erase(node);

src/dbzero/core/crdt/CRDT_Allocator.cpp

@@ -23,7 +23,7 @@ namespace db0
         {
             auto hint = &m_hints[0];
             for (auto &alloc : m_cache) {
-                if (alloc && alloc.first->m_stride == size) {
+                if (!alloc.isEnd() && alloc.first->m_stride == size) {
                     // the const_cast is safe because we store mutated items
                     auto result = const_cast<Alloc*>(alloc.first)->tryAllocUnit(addr_bound, hint->first, hint->second);
                     if (!result || alloc.first->isFull()) {
@@ -42,7 +42,7 @@ namespace db0
             auto hint = &m_hints[0];
             auto new_hint = new_alloc.first->getHint();
             for (auto &alloc : m_cache) {
-                if (!alloc) {
+                if (alloc.isEnd()) {
                     alloc = new_alloc;
                     *hint =  new_hint;
                     return;
@@ -522,7 +522,7 @@ namespace db0
                     assert(!redZone());
                     // merge with the neighboring blank if such exists
                     std::optional<Blank> b1;
-                    if (alloc_window[2]) {
+                    if (!alloc_window[2].isEnd()) {
                         // right neighbor exists
                         auto &right = *alloc_window[2].first;
                         auto b1_size = right.m_address - alloc.m_address - old_size;
@@ -616,7 +616,7 @@ namespace db0
 
         // we need to remove the alloc entry since it's empty
         std::optional<Blank> b0, b1;
-        if (alloc_window[0]) {
+        if (!alloc_window[0].isEnd()) {
             // left neighbor exists
             auto &left = *alloc_window[0].first;
             auto b0_size = alloc.m_address - left.m_address - left.size();
@@ -629,7 +629,7 @@ namespace db0
             }
         }
 
-        if (alloc_window[2]) {
+        if (!alloc_window[2].isEnd()) {
             // right neighbor exists
             auto &right = *alloc_window[2].first;
             auto b1_size = right.m_address - alloc.m_address - alloc.size();
@@ -677,7 +677,7 @@ namespace db0
     std::size_t CRDT_Allocator::getAllocSize(std::uint64_t address) const
     {
         auto alloc = m_allocs.lower_equal_bound(address);
-        if (!alloc) {
+        if (alloc.isEnd()) {
             THROWF(db0::BadAddressException) << "Invalid address: " << address;            
         }
         return alloc.first->getAllocSize(address);
@@ -686,7 +686,7 @@ namespace db0
     bool CRDT_Allocator::isAllocated(std::uint64_t address, std::size_t *size_of_result) const
     {
         auto alloc = m_allocs.lower_equal_bound(address);
-        if (!alloc) {
+        if (alloc.isEnd()) {
             return false;
         }
         return alloc.first->isAllocated(address, size_of_result);
@@ -761,7 +761,7 @@ namespace db0
         // max_addr must be updated before any updates to allocator's metadata
         m_max_addr = std::max(m_max_addr, blank->m_address + min_size);
         // NOTE: has_stripe flag is set here
-        auto alloc = m_allocs.emplace(blank->m_address, stride, count, true);
+        auto alloc = m_allocs.emplace((std::uint32_t)blank->m_address, stride, count, true);
         auto result = alloc.first->allocUnit();
         assert(alloc.first->endAddr() <= m_max_addr);
         assert(!redZone());
@@ -830,7 +830,7 @@ namespace db0
         last_stripe_units = 0;
         auto stripe_ptr = m_stripes.lower_equal_bound(size);
         assert(stripe_ptr.validate());
-        if (!stripe_ptr || stripe_ptr.first->m_stride != size) {
+        if (stripe_ptr.isEnd() || stripe_ptr.first->m_stride != size) {
             return std::nullopt;
         }
 

src/dbzero/core/crdt/CRDT_Allocator.hpp

@@ -9,7 +9,6 @@
 namespace db0
 
 {
-DB0_PACKED_BEGIN
 
     namespace crdt 
 
@@ -66,6 +65,7 @@ DB0_PACKED_BEGIN
         // higher values allow eliminating "bind spots" in the allocator, but may incur storage & performance overhead
         static constexpr std::uint32_t ALIGNED_INDEX_THRESHOLD = 4;
 
+DB0_PACKED_BEGIN
         struct DB0_PACKED_ATTR FillMap
         {            
             // the low 1 - 56 bits are used to encode unit allocations
@@ -139,20 +139,32 @@ DB0_PACKED_BEGIN
                 return m_data & (crdt::HAS_STRIPE_BIT | crdt::LOST_STRIPE_BIT);
             }
         };
+DB0_PACKED_END
 
-        struct Stripe;
-        struct Blank;
+        struct DB0_PACKED_ATTR Stripe;
+        struct DB0_PACKED_ATTR Blank;
 
+DB0_PACKED_BEGIN
         // 16-byte allocation record
         struct DB0_PACKED_ATTR Alloc
         {
+            // primary key
             std::uint32_t m_address = 0;
             std::uint32_t m_stride = 0;
             FillMap m_fill_map;
 
             Alloc() = default;
             Alloc(std::uint32_t address, std::uint32_t stride, std::uint32_t size, bool has_stripe);
 
+            static inline std::uint32_t getKey(const Alloc &alloc) {
+                return alloc.m_address;
+            }
+
+            // Extract key from construction args
+            static inline std::uint32_t getKey(std::uint32_t address, std::uint32_t, std::uint32_t, bool) {
+                return address;                
+            }
+
             struct CompT
             {
                 inline bool operator()(const Alloc &lhs, const Alloc &rhs) const {
@@ -273,15 +285,28 @@ DB0_PACKED_BEGIN
             // Get total capacity
             std::uint32_t capacity() const;
         };
-
-        struct Blank
+DB0_PACKED_END
+
+DB0_PACKED_BEGIN
+        struct DB0_PACKED_ATTR Blank
         {
+            // primary key
             std::uint32_t m_size;
+            // secondary key
             std::uint32_t m_address;
 
             Blank() = default;
             Blank(std::uint32_t size, std::uint32_t address);
 
+            static inline Blank getKey(const Blank &blank) {
+                return blank;
+            }
+
+            // Extract key from construction args
+            static inline Blank getKey(std::uint32_t size, std::uint32_t address) {
+                return { size, address };
+            }
+
             // Get first aligned address within the blank (must satisfy aligned size > 0)
             std::uint32_t getAlignedAddress(std::uint32_t mask, std::uint32_t page_size) const;
 
@@ -334,14 +359,27 @@ DB0_PACKED_BEGIN
                 }
             };
         };
-
-        struct Stripe
+DB0_PACKED_END
+
+DB0_PACKED_BEGIN
+        struct DB0_PACKED_ATTR Stripe
         {
+            // primary key
             std::uint32_t m_stride;
+            // secondary key
             std::uint32_t m_address;
 
             Stripe(std::uint32_t stride, std::uint32_t address);
 
+            static inline Stripe getKey(const Stripe &stripe) {
+                return stripe;
+            }
+
+            // Extract key from construction args
+            static inline Stripe getKey(std::uint32_t stride, std::uint32_t address) {
+                return { stride, address };
+            }
+
             // Note that the Stripe comparison used both fields: size & address
             // but also by-size only comparison and equality check are allowed
             struct CompT
@@ -380,7 +418,8 @@ DB0_PACKED_BEGIN
                 }
             };
         };
-
+DB0_PACKED_END
+
         using AllocSetT = db0::SGB_Tree<Alloc, Alloc::CompT, Alloc::EqualT, std::uint16_t, Address>;
         using BlankSetT = db0::SGB_Tree<Blank, Blank::CompT, Blank::EqualT, std::uint16_t, Address>;
         using AlignedBlankSetT = db0::SGB_Tree<Blank, Blank::AlignedCompT, Blank::EqualT, std::uint16_t, Address>;
@@ -581,7 +620,7 @@ DB0_PACKED_BEGIN
         // note that for aligned blanks the size will represent the aligned size
         auto blank_ptr = index.upper_equal_bound(Blank(min_size, 0));
         // no blank of sufficient size (or aligned size) exists
-        if (!blank_ptr) {
+        if (blank_ptr.isEnd()) {
             return std::nullopt;
         }
 
@@ -640,7 +679,6 @@ DB0_PACKED_BEGIN
         return blank.m_address;
     }
 
-DB0_PACKED_END
 }
 
 namespace std 

src/dbzero/core/memory/MetaAllocator.cpp

@@ -557,7 +557,8 @@ namespace db0
                 auto slab_id = item.m_cap_item.m_slab_id;
                 if (item.m_final_remaining_capacity != item.m_cap_item.m_remaining_capacity) {
                     auto it = m_capacity_items.find_equal(item.m_cap_item);
-                    // register under a modified key
+                    assert(!it.isEnd());
+                    // register under a modified key                    
                     m_capacity_items.erase(it);
                     m_capacity_items.emplace(
                         item.m_final_remaining_capacity, item.m_final_lost_capacity, slab_id