chore: simplify BumpUps deduplication

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) {