From 38ec7aa5aed6df338b41aed2138475457b193ea5 Mon Sep 17 00:00:00 2001 From: NegatioN <4037769+NegatioN@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:41:35 +0200 Subject: [PATCH] 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. --- src/core/dense_set.cc | 2 +- src/core/dense_set.h | 14 ++++++++------ src/core/score_map.cc | 6 +----- src/core/score_map.h | 5 ++--- src/core/string_map.cc | 20 +++++++------------ src/core/string_map.h | 6 ++---- src/core/string_set.cc | 44 ++++++++++-------------------------------- src/core/string_set.h | 5 ++--- 8 files changed, 33 insertions(+), 69 deletions(-) diff --git a/src/core/dense_set.cc b/src/core/dense_set.cc index 6231ae9c1bcd..3de47b1af101 100644 --- 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 index e9fe9e8e7447..2e1f9227ae95 100644 --- 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 index 113cdb39331f..276627bb06d7 100644 --- 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 index ab5e08aebe53..847877297387 100644 --- 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 index cbcf7bc394f5..eaad4d6a5a55 100644 --- a/src/core/string_map.cc +++ b/src/core/string_map.cc @@ -280,17 +280,6 @@ 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); @@ -298,8 +287,13 @@ void StringMap::ObjDelete(void* obj, bool has_ttl) const { 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 index de9279e5cecc..7930dd66d838 100644 --- 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 index a38951fcca6e..9c1e28ea326f 100644 --- 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 index 592929ce6f3c..544d4b3f7e52 100644 --- 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