From f013699613f39acd1cbe5bb2b26ad7674eebf456 Mon Sep 17 00:00:00 2001 From: Chaka Date: Wed, 12 Apr 2023 22:19:06 +0300 Subject: [PATCH] Revert to use CachePrevValueIfNeeded(), but let it call GetString() if needed. (#1080) I believe this is a better API as it's harder to mistakenly use. Signed-off-by: chakaz Co-authored-by: ashotland --- src/server/string_family.cc | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/server/string_family.cc b/src/server/string_family.cc index dd7927e40..53e1c25dd 100644 --- a/src/server/string_family.cc +++ b/src/server/string_family.cc @@ -482,14 +482,11 @@ class SetResultBuilder { explicit SetResultBuilder(bool return_prev_value) : return_prev_value_(return_prev_value) { } - // Calling GetString() copies `it`'s value, which is unnecessary in most case - bool ShouldReturnPrevValue() const { - return return_prev_value_; - } - - void CachePrevValue(string_view value) { - DCHECK(return_prev_value_); - prev_value_ = value; + void CachePrevValueIfNeeded(EngineShard* shard, const PrimeValue& pv) { + if (return_prev_value_) { + // We call lazily call GetString() here to save string copying when not needed. + prev_value_ = GetString(shard, pv); + } } // Returns either the previous value or `status`, depending on return_prev_value_. @@ -521,8 +518,8 @@ OpResult> SetCmd::Set(const SetParams& params, string_view key, if (params.IsConditionalSet()) { const auto [it, expire_it] = db_slice.FindExt(op_args_.db_cntx, key); - if (result_builder.ShouldReturnPrevValue() && IsValid(it)) { - result_builder.CachePrevValue(GetString(shard, it->second)); + if (IsValid(it)) { + result_builder.CachePrevValueIfNeeded(shard, it->second); } // Make sure that we have this key, and only add it if it does exists @@ -550,9 +547,7 @@ OpResult> SetCmd::Set(const SetParams& params, string_view key, PrimeIterator it = get<0>(add_res); if (!get<2>(add_res)) { // Existing. - if (result_builder.ShouldReturnPrevValue()) { - result_builder.CachePrevValue(GetString(shard, it->second)); - } + result_builder.CachePrevValueIfNeeded(shard, it->second); return std::move(result_builder).Return(SetExisting(params, it, get<1>(add_res), key, value)); }