chore: make add_ttl a param of ObjectClone

I just implemented this to see how it would look. Can revert if this is not the way we want to go after all.
dragonflydb · Sep 27, 2024 · 38ec7aa · 38ec7aa
1 parent d95b074
commit 38ec7aa
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 69 deletions.
diff --git a/src/core/dense_set.cc b/src/core/dense_set.cc
@@ -211,7 +211,7 @@ void DenseSet::CloneBatch(unsigned len, CloneItem* items, DenseSet* other) const
       auto& src = items[i];
       if (src.obj) {
         // The majority of the CPU is spent in this block.
-        void* new_obj = other->ObjectClone(src.obj, src.has_ttl);
+        void* new_obj = other->ObjectClone(src.obj, src.has_ttl, false);
         uint64_t hash = Hash(src.obj, 0);
         other->AddUnique(new_obj, src.has_ttl, hash);
         src.obj = nullptr;

diff --git a/src/core/dense_set.h b/src/core/dense_set.h
@@ -188,11 +188,14 @@ class DenseSet {
     }
 
     void SetExpiryTime(uint32_t ttl_sec) {
-      if (HasExpiry()) {
-        owner_->ObjUpdateExpireTime(curr_entry_->GetObject(), ttl_sec);
-      } else {
-        owner_->ObjReplace(curr_entry_->GetObject(), ttl_sec);
+      if (!HasExpiry()) {
+        auto src = curr_entry_->GetObject();
+        void* new_obj = owner_->ObjectClone(src, false, true);
+        curr_entry_->SetObject(new_obj);
+        curr_entry_->SetTtl(true);
+        owner_->ObjDelete(src, false);
       }
+      owner_->ObjUpdateExpireTime(curr_entry_->GetObject(), ttl_sec);
     }
 
     bool HasExpiry() const {
@@ -274,9 +277,8 @@ class DenseSet {
   virtual size_t ObjectAllocSize(const void* obj) const = 0;
   virtual uint32_t ObjExpireTime(const void* obj) const = 0;
   virtual void ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) = 0;
-  virtual void ObjReplace(const void* obj, uint32_t ttl_sec) = 0;
   virtual void ObjDelete(void* obj, bool has_ttl) const = 0;
-  virtual void* ObjectClone(const void* obj, bool has_ttl) const = 0;
+  virtual void* ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const = 0;
 
   void CollectExpired();
 

diff --git a/src/core/score_map.cc b/src/core/score_map.cc
@@ -128,10 +128,6 @@ uint32_t ScoreMap::ObjExpireTime(const void* obj) const {
   return UINT32_MAX;
 }
 
-void ScoreMap::ObjReplace(const void* obj, uint32_t ttl_sec) {
-  // Should not reach.
-}
-
 void ScoreMap::ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) {
   // Should not reach.
 }
@@ -141,7 +137,7 @@ void ScoreMap::ObjDelete(void* obj, bool has_ttl) const {
   sdsfree(s1);
 }
 
-void* ScoreMap::ObjectClone(const void* obj, bool has_ttl) const {
+void* ScoreMap::ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const {
   return nullptr;
 }
 

diff --git a/src/core/score_map.h b/src/core/score_map.h
@@ -123,9 +123,8 @@ class ScoreMap : public DenseSet {
   size_t ObjectAllocSize(const void* obj) const final;
   uint32_t ObjExpireTime(const void* obj) const final;
   void ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) override;
-  void ObjReplace(const void* obj, uint32_t ttl_sec) override;
-  void ObjDelete(void* obj, bool has_ttl) const final;
-  void* ObjectClone(const void* obj, bool has_ttl) const final;
+  void ObjDelete(void* obj, bool has_ttl) const override;
+  void* ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const final;
 };
 
 }  // namespace dfly
diff --git a/src/core/string_map.cc b/src/core/string_map.cc
@@ -280,26 +280,20 @@ void StringMap::ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) {
   return SdsUpdateExpireTime(obj, time_now() + ttl_sec, 8);
 }
 
-void StringMap::ObjReplace(const void* obj, uint32_t ttl_sec) {
-  sds str = (sds)obj;
-  auto pair = detail::SdsPair(str, GetValue(str));
-  auto [newkey, sdsval_tag] = CreateEntry(pair->first, pair->second, time_now(), ttl_sec);
-
-  sds prev_entry = (sds)AddOrReplaceObj(newkey, sdsval_tag & kValTtlBit);
-  if (prev_entry) {
-    ObjDelete(prev_entry, false);
-  }
-}
-
 void StringMap::ObjDelete(void* obj, bool has_ttl) const {
   sds s1 = (sds)obj;
   sds value = GetValue(s1);
   sdsfree(value);
   sdsfree(s1);
 }
 
-void* StringMap::ObjectClone(const void* obj, bool has_ttl) const {
-  return nullptr;
+void* StringMap::ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const {
+  uint32_t ttl_sec = add_ttl ? 0 : (has_ttl ? ObjExpireTime(obj) : UINT32_MAX);
+  sds str = (sds)obj;
+  auto pair = detail::SdsPair(str, GetValue(str));
+  auto [newkey, sdsval_tag] = CreateEntry(pair->first, pair->second, time_now(), ttl_sec);
+
+  return (void*)newkey;
 }
 
 detail::SdsPair StringMap::iterator::BreakToPair(void* obj) {

diff --git a/src/core/string_map.h b/src/core/string_map.h
@@ -103,7 +103,6 @@ class StringMap : public DenseSet {
     using IteratorBase::SetExpiryTime;
   };
 
-  // Returns true if field was added
   // otherwise updates its value and returns false.
   bool AddOrUpdate(std::string_view field, std::string_view value, uint32_t ttl_sec = UINT32_MAX);
 
@@ -159,9 +158,8 @@ class StringMap : public DenseSet {
   size_t ObjectAllocSize(const void* obj) const final;
   uint32_t ObjExpireTime(const void* obj) const final;
   void ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) override;
-  void ObjReplace(const void* obj, uint32_t ttl_sec) override;
-  void ObjDelete(void* obj, bool has_ttl) const final;
-  void* ObjectClone(const void* obj, bool has_ttl) const final;
+  void ObjDelete(void* obj, bool has_ttl) const override;
+  void* ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const final;
 };
 
 }  // namespace dfly
diff --git a/src/core/string_set.cc b/src/core/string_set.cc
@@ -43,7 +43,7 @@ bool StringSet::AddSds(sds s1) {
 }
 
 bool StringSet::Add(string_view src, uint32_t ttl_sec) {
-  sds newsds = MakeSdsWithTtl(src, ttl_sec);
+  sds newsds = MakeSetSds(src, ttl_sec);
   bool has_ttl = ttl_sec != UINT32_MAX;
 
   if (AddOrFindObj(newsds, has_ttl) != nullptr) {
@@ -119,54 +119,30 @@ void StringSet::ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) {
   return SdsUpdateExpireTime(obj, time_now() + ttl_sec, 0);
 }
 
-void StringSet::ObjReplace(const void* obj, uint32_t ttl_sec) {
-  sds str = (sds)obj;
-  sds newsds = MakeSdsWithTtl(str, ttl_sec);
-  bool has_ttl = ttl_sec == UINT32_MAX ? false : true;
-
-  sds prev_entry = (sds)AddOrReplaceObj(newsds, has_ttl);
-  if (prev_entry) {
-    sdsfree(prev_entry);
-  } else {
-    // No previous entry found, so we need to free the new entry.
-    sdsfree(newsds);
-  }
-}
-
 void StringSet::ObjDelete(void* obj, bool has_ttl) const {
   sdsfree((sds)obj);
 }
 
-void* StringSet::ObjectClone(const void* obj, bool has_ttl) const {
+void* StringSet::ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const {
   sds src = (sds)obj;
-  if (has_ttl) {
-    size_t slen = sdslen(src);
-    char* ttlptr = src + slen + 1;
-    uint32_t at = absl::little_endian::Load32(ttlptr);
-    sds newsds = AllocImmutableWithTtl(slen, at);
-    if (slen)
-      memcpy(newsds, src, slen);
-    return newsds;
-  }
-  return sdsnewlen(src, sdslen(src));
+  string_view sv{src, sdslen(src)};
+  uint32_t ttl_sec = add_ttl ? 0 : (has_ttl ? ObjExpireTime(obj) : UINT32_MAX);
+  return (void*)MakeSetSds(sv, ttl_sec);
 }
 
-sds StringSet::MakeSdsWithTtl(string_view src, uint32_t ttl_sec) {
+sds StringSet::MakeSetSds(string_view src, uint32_t ttl_sec) const {
   DCHECK_GT(ttl_sec, 0u);  // ttl_sec == 0 would mean find and delete immediately
-  sds newsds = nullptr;
-
-  if (ttl_sec == UINT32_MAX) {
-    newsds = sdsnewlen(src.data(), src.size());
-  } else {
+  if (ttl_sec != UINT32_MAX) {
     uint32_t at = time_now() + ttl_sec;
     DCHECK_LT(time_now(), at);
 
-    newsds = AllocImmutableWithTtl(src.size(), at);
+    sds newsds = AllocImmutableWithTtl(src.size(), at);
     if (!src.empty())
       memcpy(newsds, src.data(), src.size());
+    return newsds;
   }
 
-  return newsds;
+  return sdsnewlen(src.data(), src.size());
 }
 
 }  // namespace dfly
diff --git a/src/core/string_set.h b/src/core/string_set.h
@@ -113,10 +113,9 @@ class StringSet : public DenseSet {
   size_t ObjectAllocSize(const void* s1) const override;
   uint32_t ObjExpireTime(const void* obj) const override;
   void ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) override;
-  void ObjReplace(const void* obj, uint32_t ttl_sec) override;
   void ObjDelete(void* obj, bool has_ttl) const override;
-  void* ObjectClone(const void* obj, bool has_ttl) const override;
-  sds MakeSdsWithTtl(std::string_view src, uint32_t ttl_sec);
+  void* ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const override;
+  sds MakeSetSds(std::string_view src, uint32_t ttl_sec) const;
 };
 
 }  // end namespace dfly