chore: Remove DenseSet::AddOrFindDense and AddSds (#3864)

Clean up interface a bit. AddOrFindDense does not make much sense as a single function
because it does not provide any performance benefits - we still must perform a lookup
before inserting. AddSds should have been removed a long time ago.

Signed-off-by: Roman Gershman <[email protected]>

src/core/dense_set.cc

@@ -477,32 +477,6 @@ void DenseSet::Grow(size_t prev_size) {
   }
 }
 
-auto DenseSet::AddOrFindDense(void* ptr, bool has_ttl) -> DensePtr* {
-  uint64_t hc = Hash(ptr, 0);
-
-  if (entries_.empty()) {
-    capacity_log_ = kMinSizeShift;
-    entries_.resize(kMinSize);
-    uint32_t bucket_id = BucketId(hc);
-    auto e = entries_.begin() + bucket_id;
-    obj_malloc_used_ += PushFront(e, ptr, has_ttl);
-    ++size_;
-    ++num_used_buckets_;
-
-    return nullptr;
-  }
-
-  // if the value is already in the set exit early
-  uint32_t bucket_id = BucketId(hc);
-  DensePtr* dptr = Find(ptr, bucket_id, 0).second;
-  if (dptr != nullptr) {
-    return dptr;
-  }
-
-  AddUnique(ptr, has_ttl, hc);
-  return nullptr;
-}
-
 // Assumes that the object does not exist in the set.
 void DenseSet::AddUnique(void* obj, bool has_ttl, uint64_t hashcode) {
   if (entries_.empty()) {
@@ -685,22 +659,25 @@ void* DenseSet::PopInternal() {
 }
 
 void* DenseSet::AddOrReplaceObj(void* obj, bool has_ttl) {
-  DensePtr* ptr = AddOrFindDense(obj, has_ttl);
-  if (!ptr)
-    return nullptr;
+  uint64_t hc = Hash(obj, 0);
+  DensePtr* dptr = entries_.empty() ? nullptr : Find(obj, BucketId(hc), 0).second;
 
-  if (ptr->IsLink()) {
-    ptr = ptr->AsLink();
-  }
+  if (dptr) {  // replace
+    if (dptr->IsLink())
+      dptr = dptr->AsLink();
 
-  void* res = ptr->Raw();
-  obj_malloc_used_ -= ObjectAllocSize(res);
-  obj_malloc_used_ += ObjectAllocSize(obj);
+    void* res = dptr->Raw();
+    obj_malloc_used_ -= ObjectAllocSize(res);
+    obj_malloc_used_ += ObjectAllocSize(obj);
+
+    dptr->SetObject(obj);
+    dptr->SetTtl(has_ttl);
 
-  ptr->SetObject(obj);
-  ptr->SetTtl(has_ttl);
+    return res;
+  }
 
-  return res;
+  AddUnique(obj, has_ttl, hc);
+  return nullptr;
 }
 
 /**

src/core/dense_set.h

@@ -310,13 +310,6 @@ class DenseSet {
     obj_malloc_used_ -= delta;
   }
 
-  // Returns previous if the equivalent object already exists,
-  // Returns nullptr if obj was added.
-  void* AddOrFindObj(void* obj, bool has_ttl) {
-    DensePtr* ptr = AddOrFindDense(obj, has_ttl);
-    return ptr ? ptr->GetObject() : nullptr;
-  }
-
   // Returns the previous object if it has been replaced.
   // nullptr, if obj was added.
   void* AddOrReplaceObj(void* obj, bool has_ttl);
@@ -369,10 +362,6 @@ class DenseSet {
   void* PopDataFront(ChainVectorIterator);
   DensePtr PopPtrFront(ChainVectorIterator);
 
-  // Returns DensePtr if the object with such key already exists,
-  // Returns null if obj was added.
-  DensePtr* AddOrFindDense(void* obj, bool has_ttl);
-
   // ============ Pseudo Linked List in DenseSet end ==================
 
   // returns (prev, item) pair. If item is root, then prev is null.

src/core/string_set.cc

@@ -38,19 +38,16 @@ StringSet::~StringSet() {
   Clear();
 }
 
-bool StringSet::AddSds(sds s1) {
-  return AddOrFindObj(s1, false) == nullptr;
-}
-
 bool StringSet::Add(string_view src, uint32_t ttl_sec) {
-  sds newsds = MakeSetSds(src, ttl_sec);
-  bool has_ttl = ttl_sec != UINT32_MAX;
-
-  if (AddOrFindObj(newsds, has_ttl) != nullptr) {
-    sdsfree(newsds);
+  uint64_t hash = Hash(&src, 1);
+  void* prev = FindInternal(&src, hash, 1);
+  if (prev != nullptr) {
     return false;
   }
 
+  sds newsds = MakeSetSds(src, ttl_sec);
+  bool has_ttl = ttl_sec != UINT32_MAX;
+  AddUnique(newsds, has_ttl, hash);
   return true;
 }
 

src/core/string_set.h

@@ -28,9 +28,6 @@ class StringSet : public DenseSet {
   // Returns true if elem was added.
   bool Add(std::string_view s1, uint32_t ttl_sec = UINT32_MAX);
 
-  // Used currently by rdb_load. Returns true if elem was added.
-  bool AddSds(sds elem);
-
   bool Erase(std::string_view str) {
     return EraseInternal(&str, 1);
   }

src/core/string_set_test.cc

@@ -531,4 +531,25 @@ void BM_Clear(benchmark::State& state) {
 }
 BENCHMARK(BM_Clear)->ArgName("elements")->Arg(32000);
 
+void BM_Add(benchmark::State& state) {
+  vector<string> strs;
+  mt19937 generator(0);
+  StringSet ss;
+  unsigned elems = 100000;
+  for (size_t i = 0; i < elems; ++i) {
+    string str = random_string(generator, 16);
+    strs.push_back(str);
+  }
+  ss.Reserve(elems);
+  while (state.KeepRunning()) {
+    for (auto& str : strs)
+      ss.Add(str);
+    state.PauseTiming();
+    ss.Clear();
+    ss.Reserve(elems);
+    state.ResumeTiming();
+  }
+}
+BENCHMARK(BM_Add);
+
 }  // namespace dfly

src/server/rdb_load.cc

@@ -37,6 +37,7 @@ extern "C" {
 #include "core/string_set.h"
 #include "server/cluster/cluster_defs.h"
 #include "server/cluster/cluster_family.h"
+#include "server/container_utils.h"
 #include "server/engine_shard_set.h"
 #include "server/error.h"
 #include "server/hset_family.h"
@@ -1029,16 +1030,14 @@ void RdbLoaderBase::OpaqueObjLoader::HandleBlob(string_view blob) {
       ec_ = RdbError(errc::rdb_file_corrupted);
       return;
     }
+
     unsigned char* lp = (unsigned char*)blob.data();
     StringSet* set = CompactObj::AllocateMR<StringSet>();
     for (unsigned char* cur = lpFirst(lp); cur != nullptr; cur = lpNext(lp, cur)) {
-      unsigned int slen = 0;
-      long long lval = 0;
-      unsigned char* res = lpGetValue(cur, &slen, &lval);
-      sds sdsele = res ? sdsnewlen(res, slen) : sdsfromlonglong(lval);
-      if (!set->AddSds(sdsele)) {
-        LOG(ERROR) << "Duplicate member " << sdsele;
-        sdsfree(sdsele);
+      unsigned char field_buf[LP_INTBUF_SIZE];
+      string_view elem = container_utils::LpGetView(cur, field_buf);
+      if (!set->Add(elem)) {
+        LOG(ERROR) << "Duplicate member " << elem;
         ec_ = RdbError(errc::duplicate_key);
         break;
       }