Fix bugs in Simple Cache (#263)

* 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 <raakella1@$HOSTNAME>

Author: raakella1

conanfile.py

@@ -9,7 +9,7 @@
 
 class SISLConan(ConanFile):
     name = "sisl"
-    version = "12.3.3"
+    version = "12.4.1"
 
     homepage = "https://github.com/eBay/sisl"
     description = "Library for fast data structures, utilities"

include/sisl/cache/evictor.hpp

@@ -28,7 +28,17 @@ typedef ValueEntryBase CacheRecord;
 
 class Evictor {
 public:
-    typedef std::function< bool(const CacheRecord&) > can_evict_cb_t;
+    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};
+    };
 
     Evictor(const int64_t max_size, const uint32_t num_partitions) :
             m_max_size{max_size}, m_num_partitions{num_partitions} {}
@@ -38,12 +48,12 @@ class Evictor {
     Evictor& operator=(Evictor&&) noexcept = delete;
     virtual ~Evictor() = default;
 
-    uint32_t register_record_family(can_evict_cb_t can_evict_cb = nullptr) {
+    uint32_t register_record_family(RecordFamily record_family) {
         uint32_t id{0};
         std::unique_lock lk(m_reg_mtx);
-        while (id < m_can_evict_cbs.size()) {
-            if (m_can_evict_cbs[id].first == false) {
-                m_can_evict_cbs[id] = std::make_pair(true, can_evict_cb);
+        while (id < m_eviction_cbs.size()) {
+            if (m_eviction_cbs[id].first == false) {
+                m_eviction_cbs[id] = std::make_pair(true, record_family);
                 return id;
             }
             ++id;
@@ -54,7 +64,7 @@ class Evictor {
 
     void unregister_record_family(const uint32_t record_type_id) {
         std::unique_lock lk(m_reg_mtx);
-        m_can_evict_cbs[record_type_id] = std::make_pair(false, nullptr);
+        m_eviction_cbs[record_type_id] = std::make_pair(false, RecordFamily{});
     }
 
     virtual bool add_record(uint64_t hash_code, CacheRecord& record) = 0;
@@ -64,13 +74,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 m_can_evict_cbs[record_id].second; }
+    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::pair< bool, can_evict_cb_t >, CacheRecord::max_record_families() > m_can_evict_cbs;
+    std::array< std::pair< bool /*registered*/, RecordFamily >, CacheRecord::max_record_families() > m_eviction_cbs;
 };
 } // namespace sisl

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,10 +91,16 @@ class LRUEvictor : public Evictor {
         void 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 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,19 +37,40 @@ 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));
+                // Register the record family callbacks with the evictor:
+                // - `can_evict_cb`: Provided by the user of the `SimpleCache`. This callback determines whether a record can be evicted.
+                // - `post_eviction_cb`: Owned by the `SimpleCache`. This callback is used to remove the evicted record from the hashmap.
+                //   Note: We cannot directly call `erase` on the hashmap as it might result in deadlocks. Instead, we use `try_erase`,
+                //   which attempts to acquire the bucket lock using `try_lock`. If the lock cannot be acquired, the method returns `false`.
+                //   In such cases, we notify the evictor to skip evicting this record and try the next one.
+        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);
-        return m_map.insert(k, 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;
     }
 
     bool upsert(const V& value) {
@@ -101,4 +122,5 @@ class SimpleCache {
 
 template < typename K, typename V >
 thread_local std::set< K > SimpleCache< K, V >::t_failed_keys;
+
 } // namespace sisl

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);
 
@@ -184,27 +185,24 @@ class SimpleHashBucket {
 #ifndef GLOBAL_HASHSET_LOCK
         folly::SharedMutexWritePriority::WriteHolder holder(m_lock);
 #endif
-        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;
-            }
-        }
+        return erase_unsafe(input_key, out_val, true /* call_access_cb */);
+    }
 
-        if (n) {
-            access_cb(*n, input_key, hash_op_t::DELETE);
-            out_val = n->m_value;
-            m_list.erase(it);
-            delete n;
-            return true;
+    bool try_erase(const K& input_key) {
+        V dummy_val;
+#ifndef GLOBAL_HASHSET_LOCK
+        if(m_lock.try_lock()) {
+            bool ret = erase_unsafe(input_key, dummy_val, false /* call_access_cb */);
+            m_lock.unlock();
+            return ret;
+        } else {
+            return false;
         }
-        return false;
+#else 
+        // We are in global hashset lock 
+        return erase_unsafe(input_key, dummy_val, false); #endif
+#endif
+        return erase_unsafe(input_key, dummy_val, false);
     }
 
     bool upsert_or_delete(const K& input_key, auto&& update_or_delete_cb) {
@@ -266,9 +264,33 @@ class SimpleHashBucket {
     static void access_cb(const SingleEntryHashNode< V >& node, const K& key, hash_op_t op) {
         SimpleHashMap< K, V >::call_access_cb((const ValueEntryBase&)node, key, 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,
                                      key_access_cb_t< K > access_cb) :
@@ -317,6 +339,12 @@ bool SimpleHashMap< K, V >::erase(const K& key, V& out_val) {
     return get_bucket(key).erase(key, out_val);
 }
 
+template < typename K, typename V >
+bool SimpleHashMap< K, V >::try_erase(const K& key) {
+    set_current_instance(this);
+    return get_bucket(key).try_erase(key);
+} 
+
 /// This is a special atomic operation where user can insert_or_update_or_erase based on condition atomically. It
 /// performs differently based on certain conditions.
 ///

src/cache/lru_evictor.cpp

@@ -46,7 +46,7 @@ void LRUEvictor::record_resized(uint64_t hash_code, const CacheRecord &record,
 
 bool LRUEvictor::LRUPartition::add_record(CacheRecord &record) {
   std::unique_lock guard{m_list_guard};
-  if (will_fill(record.size()) > m_max_size) {
+  if (will_fill(record.size())) {
     if (!do_evict(record.record_family_id(), record.size())) {
       return false;
     }
@@ -58,13 +58,27 @@ bool LRUEvictor::LRUPartition::add_record(CacheRecord &record) {
 
 void LRUEvictor::LRUPartition::remove_record(CacheRecord &record) {
   std::unique_lock guard{m_list_guard};
+
+  // accessing the iterator to the record crashes if the record is not present in the list.
+  if (!record.m_member_hook.is_linked()) {
+    LOGERROR("Not expected! Record not found in partition {}", m_partition_num);
+    DEBUG_ASSERT(false, "Not expected! Record not found");
+    return;
+  }
   auto it = m_list.iterator_to(record);
   m_filled_size -= record.size();
   m_list.erase(it);
 }
 
 void LRUEvictor::LRUPartition::record_accessed(CacheRecord &record) {
   std::unique_lock guard{m_list_guard};
+
+  // accessing the iterator to the record crashes if the record is not present in the list.
+  if (!record.m_member_hook.is_linked()) {
+    LOGERROR("Not Expected! Record not found in partition {}", m_partition_num);
+    DEBUG_ASSERT(false, "Not expected! Record not found");
+    return;
+  }
   m_list.erase(m_list.iterator_to(record));
   m_list.push_back(record);
 }
@@ -82,14 +96,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)(rec)) {
-      m_filled_size -= rec.size();
+    if (!rec.is_pinned() && (!m_evictor->can_evict_cb(record_fid) || m_evictor->can_evict_cb(record_fid)(rec))) {
+      auto const rec_size = rec.size();
       it = m_list.erase(it);
-    } else {
-      ++count;
-      it = std::next(it);
+      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;
+      }
+    }
+    
+    if (eviction_failed) {
+        ++count;
+        it = std::next(it);
     }
   }