chore: simplify BumpUps deduplication (#4098)

* chore: simplify BumpUps deduplication

This pr #2474 introduced iterator protection by
tracking which keys where bumped up during the transaction operation.
This was done by managing keys view set. However, this can be simplified
using fingerprints. Also, fingerprints do not require that the original keys exist.

In addition, this #3241 PR introduces FetchedItemsRestorer that tracks bumped set and
saves it to protect against fiber context switch. My claim is that it's redundant.
Since we only keep the auto-laundering iterators, when a fiber preempts these iterators recognize it
(see IteratorT::LaunderIfNeeded) and refresh themselves anyway.

To summarize: fetched_items_ protect us from iterator invalidation during atomic scenarios,
and auto-laundering protects us from everything else, so fetched_items_ can be cleared in that case.
---------

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

src/server/db_slice.cc

@@ -118,20 +118,6 @@ class PrimeEvictionPolicy {
   const bool apply_memory_limit_;
 };
 
-class PrimeBumpPolicy {
- public:
-  PrimeBumpPolicy(const absl::flat_hash_set<CompactObjectView>& fetched_items)
-      : fetched_items_(fetched_items) {
-  }
-  // returns true if we can change the object location in dash table.
-  bool CanBump(const CompactObj& obj) const {
-    return !obj.IsSticky() && !fetched_items_.contains(obj);
-  }
-
- private:
-  const absl::flat_hash_set<CompactObjectView>& fetched_items_;
-};
-
 bool PrimeEvictionPolicy::CanGrow(const PrimeTable& tbl) const {
   ssize_t mem_available = db_slice_->memory_budget() + mem_offset_;
   if (!apply_memory_limit_ || mem_available > soft_limit_)
@@ -217,22 +203,14 @@ unsigned PrimeEvictionPolicy::Evict(const PrimeTable::HotspotBuckets& eb, PrimeT
   return 1;
 }
 
-// Helper class to cache and restore fetched_items_ of DbSlice for flows that preempt
-// because some other transaction might conclude and clear the fetched_items_ with OnCbFinish()
+// Deprecated and should be removed.
 class FetchedItemsRestorer {
  public:
-  using RestoreType = absl::flat_hash_set<CompactObjectView>;
-  explicit FetchedItemsRestorer(RestoreType* dst) : dst_to_restore_(dst) {
-    cached_ = std::move(*dst_to_restore_);
+  template <typename U> explicit FetchedItemsRestorer(U&& u) {
   }
 
   ~FetchedItemsRestorer() {
-    *dst_to_restore_ = std::move(cached_);
   }
-
- private:
-  RestoreType cached_;
-  RestoreType* dst_to_restore_;
 };
 
 }  // namespace
@@ -487,12 +465,12 @@ OpResult<DbSlice::PrimeItAndExp> DbSlice::FindInternal(const Context& cntx, std:
       };
       db.prime.CVCUponBump(change_cb_.back().first, res.it, bump_cb);
     }
-    auto bump_it = db.prime.BumpUp(res.it, PrimeBumpPolicy{fetched_items_});
+
+    auto bump_it = db.prime.BumpUp(res.it, PrimeBumpPolicy{&fetched_items_});
     if (bump_it != res.it) {  // the item was bumped
       res.it = bump_it;
       ++events_.bumpups;
     }
-    fetched_items_.insert(res.it->first.AsRef());
   }
 
   std::move(update_stats_on_miss).Cancel();
@@ -704,7 +682,6 @@ bool DbSlice::Del(Context cntx, Iterator it) {
     string_view key = it->first.GetSlice(&tmp);
     doc_del_cb_(key, cntx, it->second);
   }
-  fetched_items_.erase(it->first.AsRef());
   PerformDeletion(it, db.get());
 
   return true;

src/server/db_slice.h

@@ -592,14 +592,27 @@ class DbSlice {
 
   DbTableArray db_arr_;
 
+  struct FpHasher {
+    size_t operator()(uint64_t val) const {
+      return val;
+    }
+  };
+
   // Used in temporary computations in Acquire/Release.
-  mutable absl::flat_hash_set<uint64_t> uniq_fps_;
+  mutable absl::flat_hash_set<uint64_t, FpHasher> uniq_fps_;
 
   // ordered from the smallest to largest version.
   std::list<std::pair<uint64_t, ChangeCallback>> change_cb_;
 
   // Used in temporary computations in Find item and CbFinish
-  mutable absl::flat_hash_set<CompactObjectView> fetched_items_;
+  // This set is used to hold fingerprints of key accessed during the run of
+  // a transaction callback (not the whole transaction).
+  // We track them to avoid bumping them again (in any direction) so that the iterators to
+  // the fetched keys will not be invalidated. We must do it for atomic operations,
+  // for operations that preempt in the middle we have another mechanism -
+  // auto laundering iterators, so in case of preemption we do not mind that fetched_items are
+  // cleared or changed.
+  mutable absl::flat_hash_set<uint64_t, FpHasher> fetched_items_;
 
   // Registered by shard indices on when first document index is created.
   DocDeletionCallback doc_del_cb_;
@@ -632,6 +645,24 @@ class DbSlice {
                       absl::container_internal::hash_default_hash<std::string>,
                       absl::container_internal::hash_default_eq<std::string>, AllocatorType>
       client_tracking_map_;
+
+  class PrimeBumpPolicy {
+   public:
+    PrimeBumpPolicy(absl::flat_hash_set<uint64_t, FpHasher>* items) : fetched_items_(items) {
+    }
+
+    // returns true if we can change the object location in dash table.
+    bool CanBump(const CompactObj& obj) const {
+      if (obj.IsSticky()) {
+        return false;
+      }
+      auto hc = obj.HashCode();
+      return fetched_items_->insert(hc).second;
+    }
+
+   private:
+    mutable absl::flat_hash_set<uint64_t, FpHasher>* fetched_items_;
+  };
 };
 
 inline bool IsValid(const DbSlice::Iterator& it) {