From a86fcf80be0aaeea6aa0b4540d1be06b419d3653 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Fri, 4 Oct 2024 17:14:49 +0300 Subject: [PATCH] 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 --- src/core/dense_set.cc | 55 +++++++++++-------------------------- src/core/dense_set.h | 11 -------- src/core/string_set.cc | 15 ++++------ src/core/string_set.h | 3 -- src/core/string_set_test.cc | 21 ++++++++++++++ src/server/rdb_load.cc | 13 ++++----- 6 files changed, 49 insertions(+), 69 deletions(-) diff --git a/src/core/dense_set.cc b/src/core/dense_set.cc index 4c3a890c7..796f70a63 100644 --- a/src/core/dense_set.cc +++ b/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 = dptr->Raw(); + obj_malloc_used_ -= ObjectAllocSize(res); + obj_malloc_used_ += ObjectAllocSize(obj); + + dptr->SetObject(obj); + dptr->SetTtl(has_ttl); + + return res; } - void* res = ptr->Raw(); - obj_malloc_used_ -= ObjectAllocSize(res); - obj_malloc_used_ += ObjectAllocSize(obj); - - ptr->SetObject(obj); - ptr->SetTtl(has_ttl); - - return res; + AddUnique(obj, has_ttl, hc); + return nullptr; } /** diff --git a/src/core/dense_set.h b/src/core/dense_set.h index 0aade41ff..866a19914 100644 --- a/src/core/dense_set.h +++ b/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. diff --git a/src/core/string_set.cc b/src/core/string_set.cc index 75d670a3d..502a12a34 100644 --- a/src/core/string_set.cc +++ b/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; } diff --git a/src/core/string_set.h b/src/core/string_set.h index 544d4b3f7..2e8d73c96 100644 --- a/src/core/string_set.h +++ b/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); } diff --git a/src/core/string_set_test.cc b/src/core/string_set_test.cc index 8a7db6ffa..187dcbd6c 100644 --- a/src/core/string_set_test.cc +++ b/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 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 diff --git a/src/server/rdb_load.cc b/src/server/rdb_load.cc index 65c913a70..35ecb7fec 100644 --- a/src/server/rdb_load.cc +++ b/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(); 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; }