Fix bugs in Simple Cache (eBay#263)

raakella1 · Ravi Nagarjun Akella · hkadayam · commit 1d869eda5e5d · 2025-05-09T13:17:59.000-07:00
* Fix bugs in Simple Cache

* use try_lock to delete the evicted entry from hashmap on a best effort basis

* fix bugs in the do_evict logic

* add commenst to explain the LRU cache functionality better

---------

Co-authored-by: Ravi Nagarjun Akella &lt;raakella1@$HOSTNAME&gt;
diff --git a/include/sisl/cache/evictor.hpp b/include/sisl/cache/evictor.hpp
@@ -28,6 +28,17 @@ typedef ValueEntryBase CacheRecord;
 
 class Evictor {
 public:
+    typedef std::function< bool(const CacheRecord&) > eviction_cb_t;
+    using can_evict_cb_t = eviction_cb_t;
+
+    // struct to hold the eviction callbacks for each record family
+    // can_evict_cb: called before eviction to check if the record can be evicted.
+    // post_eviction_cb: called after eviction to do any cleanup. If this returns false, the record is reinserted.
+    // and we try to evict the next record.
+    struct RecordFamily {
+        Evictor::eviction_cb_t can_evict_cb{nullptr};
+        Evictor::eviction_cb_t post_eviction_cb{nullptr};
+    };
     using do_evict_cb_t = std::function< bool(const CacheRecord&) >;
 
     struct RecordFamily {
@@ -43,9 +54,13 @@ class Evictor {
     Evictor& operator=(Evictor&&) noexcept = delete;
     virtual ~Evictor() = default;
 
+    uint32_t register_record_family(RecordFamily record_family) {
     uint32_t register_record_family(RecordFamily family) {
         uint32_t id{0};
         std::unique_lock lk(m_reg_mtx);
+        while (id < m_eviction_cbs.size()) {
+            if (m_eviction_cbs[id].first == false) {
+                m_eviction_cbs[id] = std::make_pair(true, record_family);
         while (id < m_record_families.size()) {
             if (m_record_families[id].registered == false) {
                 m_record_families[id] = std::move(family);
@@ -58,26 +73,30 @@ class Evictor {
         return 0;
     }
 
-    void unregister_record_family(uint32_t record_type_id) {
+    void unregister_record_family(const uint32_t record_type_id) {
         std::unique_lock lk(m_reg_mtx);
+        m_eviction_cbs[record_type_id] = std::make_pair(false, RecordFamily{});
         m_record_families[record_type_id].registered = false;
         m_record_families[record_type_id].do_evict_cb = nullptr;
     }
 
     virtual bool add_record(uint64_t hash_code, CacheRecord& record) = 0;
     virtual void remove_record(uint64_t hash_code, CacheRecord& record) = 0;
-    virtual bool record_accessed(uint64_t hash_code, CacheRecord& record) = 0;
+    virtual void record_accessed(uint64_t hash_code, CacheRecord& record) = 0;
     virtual void record_resized(uint64_t hash_code, const CacheRecord& record, uint32_t old_size) = 0;
 
     int64_t max_size() const { return m_max_size; }
     uint32_t num_partitions() const { return m_num_partitions; }
+    const eviction_cb_t& can_evict_cb(const uint32_t record_id) const { return m_eviction_cbs[record_id].second.can_evict_cb; }
+    const eviction_cb_t& post_eviction_cb(const uint32_t record_id) const { return m_eviction_cbs[record_id].second.post_eviction_cb; }
     const do_evict_cb_t& do_evict_cb(uint32_t record_id) const { return m_record_families[record_id].do_evict_cb; }
 
 private:
     int64_t m_max_size;
     uint32_t m_num_partitions;
 
     std::mutex m_reg_mtx;
+    std::array< std::pair< bool /*registered*/, RecordFamily >, CacheRecord::max_record_families() > m_eviction_cbs;
     std::array< RecordFamily, CacheRecord::max_record_families() > m_record_families;
 };
 } // namespace sisl
diff --git a/include/sisl/cache/lru_evictor.hpp b/include/sisl/cache/lru_evictor.hpp
@@ -49,6 +49,15 @@ class LRUEvictor : public Evictor {
 
     void record_resized(uint64_t hash_code, const CacheRecord& record, uint32_t old_size) override;
 
+    // for testing purpose
+    int64_t filled_size() {
+        int64_t filled_size{0};
+        for (uint32_t i{0}; i < num_partitions(); ++i) {
+            filled_size += m_partitions[i].filled_size();
+        }
+        return filled_size;
+    }
+
 private:
     typedef list<
         ValueEntryBase,
@@ -82,6 +91,12 @@ class LRUEvictor : public Evictor {
         bool record_accessed(CacheRecord& record);
         void record_resized(const CacheRecord& record, uint32_t old_size);
 
+        // for testing purpose
+        int64_t filled_size() {
+            std::unique_lock guard{m_list_guard}; 
+            return m_filled_size; 
+        }
+
     private:
         bool find_evict_candidates(uint32_t record_fid, uint32_t needed_size);
         bool will_fill(uint32_t new_size) const { return ((m_filled_size + new_size) > m_max_size); }
diff --git a/include/sisl/cache/range_cache.hpp b/include/sisl/cache/range_cache.hpp
@@ -43,7 +43,7 @@ class RangeCache {
                                     bind_this(RangeCache< K >::on_hash_operation, 4))},
             m_can_evict_cb{std::move(evict_cb)},
             m_per_value_size{per_val_size} {
-        m_evictor->register_record_family(std::move(evict_cb));
+        m_evictor->register_record_family(Evictor::RecordFamily{.can_evict_cb = evict_cb});
     }
 
     ~RangeCache() { m_evictor->unregister_record_family(m_record_family_id); }
diff --git a/include/sisl/cache/simple_hashmap.hpp b/include/sisl/cache/simple_hashmap.hpp
@@ -73,6 +73,7 @@ class SimpleHashMap {
     bool upsert(const K& key, const V& value);
     bool get(const K& input_key, V& out_val);
     bool erase(const K& key, V& out_val);
+    bool try_erase(const K& key);
     bool update(const K& key, auto&& update_cb);
     bool upsert_or_delete(const K& key, auto&& update_or_delete_cb);
     K record_to_key(const ValueEntryBase& record);
@@ -269,9 +270,33 @@ class SimpleHashBucket {
     static void access_cb(const SingleEntryHashNode< V >& node, const K& key, const V& value, hash_op_t op) {
         SimpleHashMap< K, V >::call_access_cb((const ValueEntryBase&)node, key, value, op);
     }
+
+    bool erase_unsafe(const K& input_key, V& out_val, bool call_access_cb) {
+        SingleEntryHashNode< V >* n = nullptr;
+
+        auto it = m_list.begin();
+        for (auto itend{m_list.end()}; it != itend; ++it) {
+            const K k = SimpleHashMap< K, V >::extractor_cb()(it->m_value);
+            if (input_key > k) {
+                break;
+            } else if (input_key == k) {
+                n = &*it;
+                break;
+            }
+        }
+
+        if (n) {
+            if (call_access_cb) { access_cb(*n, input_key, hash_op_t::DELETE); }
+            out_val = n->m_value;
+            m_list.erase(it);
+            delete n;
+            return true;
+        }
+        return false;
+    }
 };
 
-///////////////////////////////////////////// RangeHashMap Definitions ///////////////////////////////////
+///////////////////////////////////////////// SimpleHashMap Definitions ///////////////////////////////////
 template < typename K, typename V >
 SimpleHashMap< K, V >::SimpleHashMap(uint32_t nBuckets, const key_extractor_cb_t< K, V >& extract_cb,
                                      kv_access_cb_t< K, V > access_cb) :
diff --git a/src/cache/tests/test_simple_cache.cpp b/src/cache/tests/test_simple_cache.cpp
@@ -187,6 +187,96 @@ TEST_F(SimpleCacheTest, RandomData) {
             m_cache_misses, (100 * (double)m_cache_misses) / cache_ops);
 }
 
+TEST_F(SimpleCacheTest, MultiThreaded) {
+    const auto num_threads = 20;
+    LOGINFO("INFO: Do random read/write operations on all chunks for {} threads", num_threads);
+    std::vector< std::thread > threads;
+    for (uint32_t i{0}; i < num_threads; ++i) {
+        threads.emplace_back([this]() {
+            for (uint32_t j{0}; j < 20000; ++j) {
+                const uint32_t id = g_re() % m_total_keys;
+                const op_t op = s_cast< op_t >(g_re() % 3);
+                std::shared_ptr< Entry > e = std::make_shared< Entry >(0);
+                switch (op) {
+                case op_t::READ:
+                    m_cache->get(id, e);
+                    break;
+                case op_t::WRITE:
+                    m_cache->insert(std::make_shared< Entry >(id, fmt::format("test{}", j)));
+                    break;
+                case op_t::REMOVE:
+                    m_cache->remove(id, e);
+                    break;
+                }
+            }
+        });
+    }
+    for (auto& t : threads) { t.join(); }
+}
+
+TEST(SimpleCacheSize, TriggerEvict) {
+    uint32_t num_partitions = 10;
+    uint32_t max_nodes_per_partition = 3;
+    uint32_t cache_size = g_val_size * num_partitions * max_nodes_per_partition;
+    std::shared_ptr< Evictor > evictor = std::make_unique< LRUEvictor >(cache_size, num_partitions);
+    auto simple_cache = std::make_unique< SimpleCache< uint32_t, std::shared_ptr< Entry > > >(
+        evictor,                                                             // Evictor to evict used entries
+            10000,                                                     // Total number of buckets
+            g_val_size,                                                            // Value size
+            [](const std::shared_ptr< Entry >& e) -> uint32_t { return e->m_id; }, // Method to extract key
+            nullptr                                                                // Method to prevent eviction
+        );
+        auto* evictor_ptr = dynamic_cast<LRUEvictor*>(evictor.get());
+    uint32_t num_iters = num_partitions * max_nodes_per_partition * 1000;
+    for(uint32_t i = 0; i < num_iters; i++) {
+        ASSERT_TRUE(simple_cache->insert(std::make_shared< Entry >(i, fmt::format("test{}", i))));
+        ASSERT_LE(evictor_ptr->filled_size(), cache_size);
+    }
+    uint32_t cache_hits{0};
+    for(uint32_t i = 0; i < num_iters; i++) {
+        std::shared_ptr< Entry > e = std::make_shared< Entry >(0);
+        if(simple_cache->get(i, e)) {
+            ++cache_hits;
+        }
+    }
+}
+
+TEST(SimpleCacheSize, MultithreadedEviction) {
+    uint32_t num_partitions = 10;
+    uint32_t max_nodes_per_partition = 3;
+    uint32_t cache_size = g_val_size * num_partitions * max_nodes_per_partition;
+    std::shared_ptr< Evictor > evictor = std::make_unique< LRUEvictor >(cache_size, num_partitions);
+    auto simple_cache = std::make_unique< SimpleCache< uint32_t, std::shared_ptr< Entry > > >(
+            evictor,                                                             // Evictor to evict used entries
+            10000,                                                     // Total number of buckets
+            g_val_size,                                                            // Value size
+            [](const std::shared_ptr< Entry >& e) -> uint32_t { return e->m_id; }, // Method to extract key
+            nullptr                                                                // Method to prevent eviction
+        );
+    auto* evictor_ptr = dynamic_cast<LRUEvictor*>(evictor.get());
+    uint32_t num_iters = num_partitions * max_nodes_per_partition * 1000;
+    std::vector< std::thread > threads;
+    const auto num_writers = 10;
+    for (uint32_t i{0}; i < num_writers; ++i) {
+        threads.emplace_back([&simple_cache, &evictor_ptr, num_iters]() {
+            for (uint32_t j{0}; j < num_iters; ++j) {
+                simple_cache->insert(std::make_shared< Entry >(j, fmt::format("test{}", j)));
+                ASSERT_LE(evictor_ptr->filled_size(), evictor_ptr->max_size());
+            }
+        });
+    }
+    const auto num_readers = 10;
+    for (uint32_t i{0}; i < num_readers; ++i) {
+        threads.emplace_back([&simple_cache, num_iters]() {
+            for (uint32_t j{0}; j < num_iters; ++j) {
+                std::shared_ptr< Entry > e = std::make_shared< Entry >(0);
+                simple_cache->get(j, e);
+            }
+        });
+    }
+    for (auto& t : threads) { t.join(); }
+}
+
 SISL_OPTIONS_ENABLE(logging, test_simplecache)
 SISL_OPTION_GROUP(test_simplecache,
                   (cache_size_mb, "", "cache_size_mb", "cache size in mb",

Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ class RangeCache {`
`43`	`43`	`bind_this(RangeCache< K >::on_hash_operation, 4))},`
`44`	`44`	`m_can_evict_cb{std::move(evict_cb)},`
`45`	`45`	`m_per_value_size{per_val_size} {`
`46`		`- m_evictor->register_record_family(std::move(evict_cb));`
	`46`	`+ m_evictor->register_record_family(Evictor::RecordFamily{.can_evict_cb = evict_cb});`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`~RangeCache() { m_evictor->unregister_record_family(m_record_family_id); }`