fix bugs in the do_evict logic

Author: Ravi Nagarjun Akella

include/sisl/cache/evictor.hpp

@@ -28,8 +28,13 @@ typedef ValueEntryBase CacheRecord;
 
 class Evictor {
 public:
-    typedef std::function< bool(const CacheRecord&) > can_evict_cb_t;
-    typedef std::function< void(const CacheRecord&) > post_eviction_cb_t;
+    typedef std::function< bool(const CacheRecord&) > eviction_cb_t;
+    using can_evict_cb_t = eviction_cb_t;
+
+    struct RecordFamily {
+        Evictor::eviction_cb_t can_evict_cb{nullptr};
+        Evictor::eviction_cb_t post_eviction_cb{nullptr};
+    };
 
     Evictor(const int64_t max_size, const uint32_t num_partitions) :
             m_max_size{max_size}, m_num_partitions{num_partitions} {}
@@ -39,12 +44,12 @@ class Evictor {
     Evictor& operator=(Evictor&&) noexcept = delete;
     virtual ~Evictor() = default;
 
-    uint32_t register_record_family(can_evict_cb_t can_evict_cb = nullptr, post_eviction_cb_t post_eviction_cb = nullptr) {
+    uint32_t register_record_family(RecordFamily record_family) {
         uint32_t id{0};
         std::unique_lock lk(m_reg_mtx);
         while (id < m_eviction_cbs.size()) {
-            if (std::get<bool>(m_eviction_cbs[id]) == false) {
-                m_eviction_cbs[id] = std::make_tuple(true, can_evict_cb, post_eviction_cb);
+            if (m_eviction_cbs[id].first == false) {
+                m_eviction_cbs[id] = std::make_pair(true, record_family);
                 return id;
             }
             ++id;
@@ -55,7 +60,7 @@ class Evictor {
 
     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_tuple(false, nullptr, nullptr);
+        m_eviction_cbs[record_type_id] = std::make_pair(false, RecordFamily{});
     }
 
     virtual bool add_record(uint64_t hash_code, CacheRecord& record) = 0;
@@ -65,14 +70,14 @@ class Evictor {
 
     int64_t max_size() const { return m_max_size; }
     uint32_t num_partitions() const { return m_num_partitions; }
-    const can_evict_cb_t& can_evict_cb(const uint32_t record_id) const { return std::get<can_evict_cb_t>(m_eviction_cbs[record_id]); }
-    const post_eviction_cb_t& post_eviction_cb(const uint32_t record_id) const { return std::get<post_eviction_cb_t>(m_eviction_cbs[record_id]); }
+    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; }
 
 private:
     int64_t m_max_size;
     uint32_t m_num_partitions;
 
     std::mutex m_reg_mtx;
-    std::array< std::tuple< bool, can_evict_cb_t, post_eviction_cb_t >, CacheRecord::max_record_families() > m_eviction_cbs;
+    std::array< std::pair< bool, RecordFamily >, CacheRecord::max_record_families() > m_eviction_cbs;
 };
 } // namespace sisl

include/sisl/cache/lru_evictor.hpp

@@ -50,7 +50,7 @@ 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() const {
+    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();
@@ -92,12 +92,15 @@ class LRUEvictor : public Evictor {
         void record_resized(const CacheRecord& record, uint32_t old_size);
 
         // for testing purpose
-        int64_t filled_size() const { return m_filled_size; }
+        int64_t filled_size() {
+            std::unique_lock guard{m_list_guard}; 
+            return m_filled_size; 
+        }
 
     private:
         bool do_evict(const uint32_t record_fid, const uint32_t needed_size);
         bool will_fill(const uint32_t new_size) const { return ((m_filled_size + new_size) > m_max_size); }
-        bool is_full() const { return will_fill(0); }
+        bool is_full() const { return (m_filled_size >= m_max_size); }
     };
 
 private:

include/sisl/cache/range_cache.hpp

@@ -39,7 +39,7 @@ class RangeCache {
             m_map{RangeHashMap< K >(num_buckets, bind_this(RangeCache< K >::extract_value, 3),
                                     bind_this(RangeCache< K >::on_hash_operation, 4))},
             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); }

include/sisl/cache/simple_cache.hpp

@@ -37,23 +37,33 @@ class SimpleCache {
 
 public:
     SimpleCache(const std::shared_ptr< Evictor >& evictor, uint32_t num_buckets, uint32_t per_val_size,
-                key_extractor_cb_t< K, V >&& extract_cb, Evictor::can_evict_cb_t evict_cb = nullptr) :
+                key_extractor_cb_t< K, V >&& extract_cb, Evictor::eviction_cb_t evict_cb = nullptr) :
             m_evictor{evictor},
             m_key_extract_cb{std::move(extract_cb)},
             m_map{num_buckets, m_key_extract_cb, std::bind(&SimpleCache< K, V >::on_hash_operation, this, _1, _2, _3)},
             m_per_value_size{per_val_size} {
-        m_record_family_id = m_evictor->register_record_family(std::move(evict_cb), [this](const CacheRecord& record) {
-            V const value = reinterpret_cast<SingleEntryHashNode< V >*>(const_cast< CacheRecord* >(&record))->m_value;
-            K key = m_key_extract_cb(value);
-            return m_map.try_erase(key);
-        });
+        m_record_family_id = m_evictor->register_record_family(Evictor::RecordFamily{.can_evict_cb = evict_cb
+            , .post_eviction_cb = [this](const CacheRecord& record) {
+                V const value = reinterpret_cast<SingleEntryHashNode< V >*>(const_cast< CacheRecord* >(&record))->m_value;
+                K key = m_key_extract_cb(value);
+                return m_map.try_erase(key);
+            }});
     }
 
     ~SimpleCache() { m_evictor->unregister_record_family(m_record_family_id); }
 
     bool insert(const V& value) {
         K k = m_key_extract_cb(value);
         bool ret = m_map.insert(k, value);
+        if (t_failed_keys.size()) {
+            // There are some failures to add for some keys
+            for (auto& key : t_failed_keys) {
+                V dummy_v;
+                m_map.erase(key, dummy_v);
+                ret = false;
+            }
+            t_failed_keys.clear();
+        }
         return ret;
     }
 

src/cache/lru_evictor.cpp

@@ -89,17 +89,24 @@ bool LRUEvictor::LRUPartition::do_evict(const uint32_t record_fid,
   auto it = std::begin(m_list);
   while (will_fill(needed_size) && (it != std::end(m_list))) {
     CacheRecord &rec = *it;
-
+    bool eviction_failed{true};
     /* return the next element */
     if (!rec.is_pinned() && (!m_evictor->can_evict_cb(record_fid) || m_evictor->can_evict_cb(record_fid)(rec))) {
-      m_filled_size -= rec.size();
+      auto const rec_size = rec.size();
       it = m_list.erase(it);
-      if(m_evictor->post_eviction_cb(record_fid)) {
-        m_evictor->post_eviction_cb(record_fid)(rec);
+      if (m_evictor->post_eviction_cb(record_fid) && !m_evictor->post_eviction_cb(record_fid)(rec)) {
+          // If the post eviction callback fails, we need to reinsert the record
+          // back into the list.
+          it = m_list.insert(it, rec);
+      } else {
+        eviction_failed = false;
+        m_filled_size -= rec_size;
       }
-    } else {
-      ++count;
-      it = std::next(it);
+    }
+    
+    if (eviction_failed) {
+        ++count;
+        it = std::next(it);
     }
   }
 

src/cache/tests/test_simple_cache.cpp

@@ -213,15 +213,15 @@ TEST(SimpleCacheSize, TriggerEvict) {
     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
-            cache_size / 4096,                                                     // Total number of buckets
+            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++) {
-        simple_cache->insert(std::make_shared< Entry >(i, fmt::format("test{}", 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};
@@ -239,8 +239,8 @@ TEST(SimpleCacheSize, MultithreadedEviction) {
     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
-            cache_size / 4096,                                                     // Total number of buckets
+            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