From 8eb1d445c304a62f1919382491ba760434a62462 Mon Sep 17 00:00:00 2001 From: Maysam Yabandeh Date: Thu, 15 Feb 2018 17:12:48 -0800 Subject: [PATCH] Unbreak MemTableRep API change Summary: The MemTableRep API was broken by this commit: 813719e9525f647aaebf19ca3d4bb6f1c63e2648 This patch reverts the changes and instead adds InsertKey (and etc.) overloads to extend the MemTableRep API without breaking the existing classes that inherit from it. Closes https://github.com/facebook/rocksdb/pull/3513 Differential Revision: D7004134 Pulled By: maysamyabandeh fbshipit-source-id: e568d91fe1e17dd76c0c1f6c7dd51a18633b1c4f --- db/db_memtable_test.cc | 9 +++------ db/db_test_util.h | 4 ++-- db/memtable.cc | 8 ++++---- include/rocksdb/memtablerep.h | 32 +++++++++++++++++++++++++------- memtable/hash_cuckoo_rep.cc | 9 ++++----- memtable/hash_linklist_rep.cc | 9 ++++----- memtable/hash_skiplist_rep.cc | 5 ++--- memtable/skiplistrep.cc | 18 +++++++++++++++--- memtable/vectorrep.cc | 5 ++--- 9 files changed, 61 insertions(+), 38 deletions(-) diff --git a/db/db_memtable_test.cc b/db/db_memtable_test.cc index f55bd6bc9..92109c6ca 100644 --- a/db/db_memtable_test.cc +++ b/db/db_memtable_test.cc @@ -28,17 +28,14 @@ class MockMemTableRep : public MemTableRep { return rep_->Allocate(len, buf); } - virtual bool Insert(KeyHandle handle) override { - return rep_->Insert(handle); - } + virtual void Insert(KeyHandle handle) override { rep_->Insert(handle); } - virtual bool InsertWithHint(KeyHandle handle, void** hint) override { + virtual void InsertWithHint(KeyHandle handle, void** hint) override { num_insert_with_hint_++; EXPECT_NE(nullptr, hint); last_hint_in_ = *hint; - bool res = rep_->InsertWithHint(handle, hint); + rep_->InsertWithHint(handle, hint); last_hint_out_ = *hint; - return res; } virtual bool Contains(const char* key) const override { diff --git a/db/db_test_util.h b/db/db_test_util.h index 95f74190c..9f6345835 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -136,9 +136,9 @@ class SpecialMemTableRep : public MemTableRep { // Insert key into the list. // REQUIRES: nothing that compares equal to key is currently in the list. - virtual bool Insert(KeyHandle handle) override { + virtual void Insert(KeyHandle handle) override { num_entries_++; - return memtable_->Insert(handle); + memtable_->Insert(handle); } // Returns true iff an entry that compares equal to key is in the list. diff --git a/db/memtable.cc b/db/memtable.cc index 80671a41b..6cf4c2a03 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -236,7 +236,7 @@ int MemTable::KeyComparator::operator()(const char* prefix_len_key, return comparator.CompareKeySeq(a, key); } -bool MemTableRep::InsertConcurrently(KeyHandle handle) { +void MemTableRep::InsertConcurrently(KeyHandle handle) { #ifndef ROCKSDB_LITE throw std::runtime_error("concurrent insert not supported"); #else @@ -478,12 +478,12 @@ bool MemTable::Add(SequenceNumber s, ValueType type, if (insert_with_hint_prefix_extractor_ != nullptr && insert_with_hint_prefix_extractor_->InDomain(key_slice)) { Slice prefix = insert_with_hint_prefix_extractor_->Transform(key_slice); - bool res = table->InsertWithHint(handle, &insert_hints_[prefix]); + bool res = table->InsertKeyWithHint(handle, &insert_hints_[prefix]); if (!res) { return res; } } else { - bool res = table->Insert(handle); + bool res = table->InsertKey(handle); if (!res) { return res; } @@ -519,7 +519,7 @@ bool MemTable::Add(SequenceNumber s, ValueType type, assert(post_process_info == nullptr); UpdateFlushState(); } else { - bool res = table->InsertConcurrently(handle); + bool res = table->InsertKeyConcurrently(handle); if (!res) { return res; } diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index e7d675d1a..1e8f41a45 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -81,10 +81,15 @@ class MemTableRep { // single buffer and pass that in as the parameter to Insert). // REQUIRES: nothing that compares equal to key is currently in the // collection, and no concurrent modifications to the table in progress - // + virtual void Insert(KeyHandle handle) = 0; + + // Same as ::Insert // Returns false if MemTableRepFactory::CanHandleDuplicatedKey() is true and // the already exists. - virtual bool Insert(KeyHandle handle) = 0; + virtual bool InsertKey(KeyHandle handle) { + Insert(handle); + return true; + } // Same as Insert(), but in additional pass a hint to insert location for // the key. If hint points to nullptr, a new hint will be populated. @@ -92,12 +97,17 @@ class MemTableRep { // // Currently only skip-list based memtable implement the interface. Other // implementations will fallback to Insert() by default. - // + virtual void InsertWithHint(KeyHandle handle, void** hint) { + // Ignore the hint by default. + Insert(handle); + } + + // Same as ::InsertWithHint // Returns false if MemTableRepFactory::CanHandleDuplicatedKey() is true and // the already exists. - virtual bool InsertWithHint(KeyHandle handle, void** hint) { - // Ignore the hint by default. - return Insert(handle); + virtual bool InsertKeyWithHint(KeyHandle handle, void** hint) { + InsertWithHint(handle, hint); + return true; } // Like Insert(handle), but may be called concurrent with other calls @@ -105,7 +115,15 @@ class MemTableRep { // // Returns false if MemTableRepFactory::CanHandleDuplicatedKey() is true and // the already exists. - virtual bool InsertConcurrently(KeyHandle handle); + virtual void InsertConcurrently(KeyHandle handle); + + // Same as ::InsertConcurrently + // Returns false if MemTableRepFactory::CanHandleDuplicatedKey() is true and + // the already exists. + virtual bool InsertKeyConcurrently(KeyHandle handle) { + InsertConcurrently(handle); + return true; + } // Returns true iff an entry that compares equal to key is in the collection. virtual bool Contains(const char* key) const = 0; diff --git a/memtable/hash_cuckoo_rep.cc b/memtable/hash_cuckoo_rep.cc index bfc586bb6..034bf5858 100644 --- a/memtable/hash_cuckoo_rep.cc +++ b/memtable/hash_cuckoo_rep.cc @@ -97,7 +97,7 @@ class HashCuckooRep : public MemTableRep { // Insert the specified key (internal_key) into the mem-table. Assertion // fails if // the current mem-table already contains the specified key. - virtual bool Insert(KeyHandle handle) override; + virtual void Insert(KeyHandle handle) override; // This function returns bucket_count_ * approximate_entry_size_ when any // of the followings happen to disallow further write operations: @@ -319,7 +319,7 @@ void HashCuckooRep::Get(const LookupKey& key, void* callback_args, } } -bool HashCuckooRep::Insert(KeyHandle handle) { +void HashCuckooRep::Insert(KeyHandle handle) { static const float kMaxFullness = 0.90f; auto* key = static_cast(handle); @@ -340,7 +340,7 @@ bool HashCuckooRep::Insert(KeyHandle handle) { is_nearly_full_ = true; } backup_table_->Insert(key); - return true; + return; } // when reaching this point, means the insert can be done successfully. occupied_count_++; @@ -349,7 +349,7 @@ bool HashCuckooRep::Insert(KeyHandle handle) { } // perform kickout process if the length of cuckoo path > 1. - if (cuckoo_path_length == 0) return true; + if (cuckoo_path_length == 0) return; // the cuckoo path stores the kickout path in reverse order. // so the kickout or displacement is actually performed @@ -366,7 +366,6 @@ bool HashCuckooRep::Insert(KeyHandle handle) { } int insert_key_bid = cuckoo_path_[cuckoo_path_length - 1]; cuckoo_array_[insert_key_bid].store(key, std::memory_order_release); - return true; } bool HashCuckooRep::Contains(const char* internal_key) const { diff --git a/memtable/hash_linklist_rep.cc b/memtable/hash_linklist_rep.cc index d0b98e993..932b62a34 100644 --- a/memtable/hash_linklist_rep.cc +++ b/memtable/hash_linklist_rep.cc @@ -170,7 +170,7 @@ class HashLinkListRep : public MemTableRep { virtual KeyHandle Allocate(const size_t len, char** buf) override; - virtual bool Insert(KeyHandle handle) override; + virtual void Insert(KeyHandle handle) override; virtual bool Contains(const char* key) const override; @@ -571,7 +571,7 @@ Node* HashLinkListRep::GetLinkListFirstNode(Pointer* first_next_pointer) const { return nullptr; } -bool HashLinkListRep::Insert(KeyHandle handle) { +void HashLinkListRep::Insert(KeyHandle handle) { Node* x = static_cast(handle); assert(!Contains(x->key)); Slice internal_key = GetLengthPrefixedSlice(x->key); @@ -586,7 +586,7 @@ bool HashLinkListRep::Insert(KeyHandle handle) { // we publish a pointer to "x" in prev[i]. x->NoBarrier_SetNext(nullptr); bucket.store(x, std::memory_order_release); - return true; + return; } BucketHeader* header = nullptr; @@ -613,7 +613,7 @@ bool HashLinkListRep::Insert(KeyHandle handle) { // incremental. skip_list_bucket_header->Counting_header.IncNumEntries(); skip_list_bucket_header->skip_list.Insert(x->key); - return true; + return; } } @@ -691,7 +691,6 @@ bool HashLinkListRep::Insert(KeyHandle handle) { header->next.store(static_cast(x), std::memory_order_release); } } - return true; } bool HashLinkListRep::Contains(const char* key) const { diff --git a/memtable/hash_skiplist_rep.cc b/memtable/hash_skiplist_rep.cc index 01eb96c45..e34743eb2 100644 --- a/memtable/hash_skiplist_rep.cc +++ b/memtable/hash_skiplist_rep.cc @@ -28,7 +28,7 @@ class HashSkipListRep : public MemTableRep { size_t bucket_size, int32_t skiplist_height, int32_t skiplist_branching_factor); - virtual bool Insert(KeyHandle handle) override; + virtual void Insert(KeyHandle handle) override; virtual bool Contains(const char* key) const override; @@ -267,13 +267,12 @@ HashSkipListRep::Bucket* HashSkipListRep::GetInitializedBucket( return bucket; } -bool HashSkipListRep::Insert(KeyHandle handle) { +void HashSkipListRep::Insert(KeyHandle handle) { auto* key = static_cast(handle); assert(!Contains(key)); auto transformed = transform_->Transform(UserKey(key)); auto bucket = GetInitializedBucket(transformed); bucket->Insert(key); - return true; } bool HashSkipListRep::Contains(const char* key) const { diff --git a/memtable/skiplistrep.cc b/memtable/skiplistrep.cc index a21d5be17..63f7a4246 100644 --- a/memtable/skiplistrep.cc +++ b/memtable/skiplistrep.cc @@ -34,15 +34,27 @@ public: // Insert key into the list. // REQUIRES: nothing that compares equal to key is currently in the list. - virtual bool Insert(KeyHandle handle) override { + virtual void Insert(KeyHandle handle) override { + skip_list_.Insert(static_cast(handle)); + } + + virtual bool InsertKey(KeyHandle handle) override { return skip_list_.Insert(static_cast(handle)); } - virtual bool InsertWithHint(KeyHandle handle, void** hint) override { + virtual void InsertWithHint(KeyHandle handle, void** hint) override { + skip_list_.InsertWithHint(static_cast(handle), hint); + } + + virtual bool InsertKeyWithHint(KeyHandle handle, void** hint) override { return skip_list_.InsertWithHint(static_cast(handle), hint); } - virtual bool InsertConcurrently(KeyHandle handle) override { + virtual void InsertConcurrently(KeyHandle handle) override { + skip_list_.InsertConcurrently(static_cast(handle)); + } + + virtual bool InsertKeyConcurrently(KeyHandle handle) override { return skip_list_.InsertConcurrently(static_cast(handle)); } diff --git a/memtable/vectorrep.cc b/memtable/vectorrep.cc index 9b6383ffe..e54025c2d 100644 --- a/memtable/vectorrep.cc +++ b/memtable/vectorrep.cc @@ -31,7 +31,7 @@ class VectorRep : public MemTableRep { // single buffer and pass that in as the parameter to Insert) // REQUIRES: nothing that compares equal to key is currently in the // collection. - virtual bool Insert(KeyHandle handle) override; + virtual void Insert(KeyHandle handle) override; // Returns true iff an entry that compares equal to key is in the collection. virtual bool Contains(const char* key) const override; @@ -108,12 +108,11 @@ class VectorRep : public MemTableRep { const KeyComparator& compare_; }; -bool VectorRep::Insert(KeyHandle handle) { +void VectorRep::Insert(KeyHandle handle) { auto* key = static_cast(handle); WriteLock l(&rwlock_); assert(!immutable_); bucket_->push_back(key); - return true; } // Returns true iff an entry that compares equal to key is in the collection.