From d2ab1a6c5f54f271a4262997ce1786bf114a64b4 Mon Sep 17 00:00:00 2001 From: leipeng Date: Mon, 24 Apr 2023 19:41:11 +0800 Subject: [PATCH] MemTableRep::KeyValuePair: Remove virtual functions and related changes 1. Now KeyValuePair is a struct{ikey,value} 2. MemTableRep::Iterator does not derive from KeyValuePair 3. MemTableRep::Get: arg `callback` proto change 4. All related changes --- db/db_memtable_test.cc | 2 +- db/memtable.cc | 22 ++++++-------------- include/rocksdb/memtablerep.h | 38 +++++++++++------------------------ memtable/hash_linklist_rep.cc | 9 ++++----- memtable/hash_skiplist_rep.cc | 7 +++---- memtable/memtablerep_bench.cc | 4 ++-- memtable/skiplistrep.cc | 5 ++--- memtable/vectorrep.cc | 6 +++--- test_util/testutil.cc | 2 +- 9 files changed, 34 insertions(+), 61 deletions(-) diff --git a/db/db_memtable_test.cc b/db/db_memtable_test.cc index ec09cde7e7..9c51cfa70b 100644 --- a/db/db_memtable_test.cc +++ b/db/db_memtable_test.cc @@ -56,7 +56,7 @@ class MockMemTableRep : public MemTableRep { bool Contains(const Slice& key) const override { return rep_->Contains(key); } void Get(const ReadOptions& ro, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) override { + bool (*callback_func)(void* arg, const KeyValuePair&)) override { rep_->Get(ro, k, callback_args, callback_func); } diff --git a/db/memtable.cc b/db/memtable.cc index 48ea905912..4f327c591e 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -801,7 +801,7 @@ struct Saver { }; } // anonymous namespace -static bool SaveValue(void* arg, const MemTableRep::KeyValuePair* pair) { +static bool SaveValue(void* arg, const MemTableRep::KeyValuePair& pair) { Saver* s = reinterpret_cast(arg); assert(s != nullptr); assert(!s->value || !s->columns); @@ -815,7 +815,8 @@ static bool SaveValue(void* arg, const MemTableRep::KeyValuePair* pair) { // Check that it belongs to same user key. We do not check the // sequence number since the Seek() call above should have skipped // all entries with overly large sequence numbers. - auto [ikey, v] = pair->GetKeyValue(); + const Slice ikey = pair.ikey; + Slice v = pair.value; const size_t key_length = ikey.size(); const char* key_ptr = ikey.data(); assert(key_length >= 8); @@ -1530,20 +1531,9 @@ size_t MemTable::CountSuccessiveMergeEntries(const LookupKey& key) { return num_successive_merges; } -Slice MemTableRep::EncodedKeyValuePair::GetKey() const { - return GetLengthPrefixedSlice(key_); -} - -Slice MemTableRep::EncodedKeyValuePair::GetValue() const { - Slice k = GetLengthPrefixedSlice(key_); - return GetLengthPrefixedSlice(k.data() + k.size()); -} - -std::pair MemTableRep::EncodedKeyValuePair::GetKeyValue() const { - Slice k = GetLengthPrefixedSlice(key_); - Slice v = GetLengthPrefixedSlice(k.data() + k.size()); - return {k, v}; -} +MemTableRep::KeyValuePair::KeyValuePair(const char* key) + : ikey(GetLengthPrefixedSlice(key)), + value(GetLengthPrefixedSlice(ikey.end())) {} Slice MemTableRep::Iterator::GetKey() const { assert(Valid()); diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index 230bff0544..6fd2bfa0bc 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -201,27 +201,13 @@ class MemTableRep { // of time. Otherwise, RocksDB may be blocked. virtual void MarkFlushed() {} - class KeyValuePair { - public: - virtual Slice GetKey() const = 0; - virtual Slice GetValue() const = 0; - virtual std::pair GetKeyValue() const = 0; - virtual ~KeyValuePair() {} - }; - - class EncodedKeyValuePair : public KeyValuePair { - public: - virtual Slice GetKey() const override; - virtual Slice GetValue() const override; - virtual std::pair GetKeyValue() const override; - - KeyValuePair* SetKey(const char* key) { - key_ = key; - return this; - } - - private: - const char* key_ = nullptr; + struct KeyValuePair { + Slice ikey; + Slice value; + explicit KeyValuePair(const char* key); ///< cons from varlen prefixed kv + KeyValuePair(Slice ik, Slice v) : ikey(ik), value(v) {} + KeyValuePair(const std::pair& kv) // implicit cons + : ikey(kv.first), value(kv.second) {} }; template @@ -252,7 +238,7 @@ class MemTableRep { // seek and call the call back function. virtual void Get(const struct ReadOptions&, const LookupKey&, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) = 0; + bool (*callback_func)(void* arg, const KeyValuePair&)) = 0; virtual uint64_t ApproximateNumEntries(const Slice& /*start_ikey*/, const Slice& /*end_key*/) { @@ -277,7 +263,7 @@ class MemTableRep { virtual ~MemTableRep() {} // Iteration over the contents of a skip collection - class Iterator : public KeyValuePair { + class Iterator { public: // Initialize an iterator over the specified collection. // The returned iterator is not valid. @@ -293,15 +279,15 @@ class MemTableRep { // Returns the key at the current position. // REQUIRES: Valid() - virtual Slice GetKey() const override; + virtual Slice GetKey() const; // Returns the value at the current position. // REQUIRES: Valid() - virtual Slice GetValue() const override; + virtual Slice GetValue() const; // Returns the key & value at the current position. // REQUIRES: Valid() - virtual std::pair GetKeyValue() const override; + virtual std::pair GetKeyValue() const; // Advances to the next position. // REQUIRES: Valid() diff --git a/memtable/hash_linklist_rep.cc b/memtable/hash_linklist_rep.cc index ebc8ecb583..b1e2d2f0ba 100644 --- a/memtable/hash_linklist_rep.cc +++ b/memtable/hash_linklist_rep.cc @@ -176,7 +176,7 @@ class HashLinkListRep : public MemTableRep { size_t ApproximateMemoryUsage() override; void Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) override; + bool (*callback_func)(void* arg, const KeyValuePair&)) override; ~HashLinkListRep() override; @@ -729,11 +729,10 @@ size_t HashLinkListRep::ApproximateMemoryUsage() { void HashLinkListRep::Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void*, const KeyValuePair*)) { + bool (*callback_func)(void*, const KeyValuePair&)) { auto transformed = transform_->Transform(k.user_key()); Pointer& bucket = GetBucket(transformed); - EncodedKeyValuePair kv; if (IsEmptyBucket(bucket)) { return; } @@ -742,7 +741,7 @@ void HashLinkListRep::Get(const ReadOptions&, if (link_list_head != nullptr) { LinkListIterator iter(this, link_list_head); for (iter.Seek(k.internal_key(), nullptr); - iter.Valid() && callback_func(callback_args, kv.SetKey(iter.key())); + iter.Valid() && callback_func(callback_args, KeyValuePair(iter.key())); iter.Next()) { } } else { @@ -751,7 +750,7 @@ void HashLinkListRep::Get(const ReadOptions&, // Is a skip list MemtableSkipList::Iterator iter(&skip_list_header->skip_list); for (iter.Seek(k.memtable_key_data()); - iter.Valid() && callback_func(callback_args, kv.SetKey(iter.key())); + iter.Valid() && callback_func(callback_args, KeyValuePair(iter.key())); iter.Next()) { } } diff --git a/memtable/hash_skiplist_rep.cc b/memtable/hash_skiplist_rep.cc index fc31f7a522..6cbd64691a 100644 --- a/memtable/hash_skiplist_rep.cc +++ b/memtable/hash_skiplist_rep.cc @@ -34,7 +34,7 @@ class HashSkipListRep : public MemTableRep { size_t ApproximateMemoryUsage() override; void Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) override; + bool (*callback_func)(void* arg, const KeyValuePair&)) override; ~HashSkipListRep() override; @@ -286,14 +286,13 @@ size_t HashSkipListRep::ApproximateMemoryUsage() { return 0; } void HashSkipListRep::Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void*, const KeyValuePair*)) { + bool (*callback_func)(void*, const KeyValuePair&)) { auto transformed = transform_->Transform(k.user_key()); auto bucket = GetBucket(transformed); if (bucket != nullptr) { - EncodedKeyValuePair kv; Bucket::Iterator iter(bucket); for (iter.Seek(k.memtable_key_data()); - iter.Valid() && callback_func(callback_args, kv.SetKey(iter.key())); + iter.Valid() && callback_func(callback_args, KeyValuePair(iter.key())); iter.Next()) { } } diff --git a/memtable/memtablerep_bench.cc b/memtable/memtablerep_bench.cc index df96b0752c..560b724ac6 100644 --- a/memtable/memtablerep_bench.cc +++ b/memtable/memtablerep_bench.cc @@ -314,14 +314,14 @@ class ReadBenchmarkThread : public BenchmarkThread { : BenchmarkThread(table, key_gen, bytes_written, bytes_read, sequence, num_ops, read_hits) {} - static bool callback(void* arg, const MemTableRep::KeyValuePair* kv) { + static bool callback(void* arg, const MemTableRep::KeyValuePair& kv) { CallbackVerifyArgs* callback_args = static_cast(arg); assert(callback_args != nullptr); if (FLAGS_skip_read_cmp) { callback_args->found = true; return true; } - Slice internal_key = kv->GetKey(); + Slice internal_key = kv.ikey; size_t key_length = internal_key.size(); const char* key_ptr = internal_key.data(); if (FLAGS_strict_verify) { diff --git a/memtable/skiplistrep.cc b/memtable/skiplistrep.cc index 3484029c3d..73237f4671 100644 --- a/memtable/skiplistrep.cc +++ b/memtable/skiplistrep.cc @@ -83,12 +83,11 @@ class SkipListRep : public MemTableRep { } void Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) override { + bool (*callback_func)(void* arg, const KeyValuePair&)) override { SkipListRep::Iterator iter(&skip_list_); - EncodedKeyValuePair kv; Slice dummy_slice; for (iter.Seek(dummy_slice, k.memtable_key_data()); - iter.Valid() && callback_func(callback_args, kv.SetKey(iter.key())); + iter.Valid() && callback_func(callback_args, KeyValuePair(iter.key())); iter.Next()) { } } diff --git a/memtable/vectorrep.cc b/memtable/vectorrep.cc index d7af7fba26..44b4482ab0 100644 --- a/memtable/vectorrep.cc +++ b/memtable/vectorrep.cc @@ -39,7 +39,7 @@ class VectorRep : public MemTableRep { size_t ApproximateMemoryUsage() override; void Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) override; + bool (*callback_func)(void* arg, const KeyValuePair&)) override; ~VectorRep() override {} @@ -253,7 +253,7 @@ void VectorRep::Iterator::SeekToLast() { void VectorRep::Get(const ReadOptions&, const LookupKey& k, void* callback_args, - bool (*callback_func)(void* arg, const KeyValuePair*)) { + bool (*callback_func)(void* arg, const KeyValuePair&)) { rwlock_.ReadLock(); VectorRep* vector_rep; std::shared_ptr bucket; @@ -267,7 +267,7 @@ void VectorRep::Get(const ReadOptions&, rwlock_.ReadUnlock(); for (iter.Seek(k.user_key(), k.memtable_key_data()); - iter.Valid() && callback_func(callback_args, &iter); iter.Next()) { + iter.Valid() && callback_func(callback_args, iter.GetKeyValue()); iter.Next()) { } } diff --git a/test_util/testutil.cc b/test_util/testutil.cc index afaf4c25d4..cd04d35ec5 100644 --- a/test_util/testutil.cc +++ b/test_util/testutil.cc @@ -598,7 +598,7 @@ class SpecialMemTableRep : public MemTableRep { virtual void Get(const ReadOptions& ro, const LookupKey& k, void* callback_args, bool (*callback_func)(void* arg, - const KeyValuePair*)) override { + const KeyValuePair&)) override { memtable_->Get(ro, k, callback_args, callback_func); }