From eae1804f29585cc643dee798a52d71569d1d90de Mon Sep 17 00:00:00 2001 From: kailiu Date: Wed, 15 Jan 2014 18:17:58 -0800 Subject: [PATCH] Remove the unnecessary use of shared_ptr Summary: shared_ptr is slower than unique_ptr (which literally comes with no performance cost compare with raw pointers). In memtable and memtable rep, we use shared_ptr when we'd actually should use unique_ptr. According to igor's previous work, we are likely to make quite some performance gain from this diff. Test Plan: make check Reviewers: dhruba, igor, sdong, haobo CC: leveldb Differential Revision: https://reviews.facebook.net/D15213 --- db/memtable.cc | 20 ++++++------- db/memtable.h | 2 +- db/version_set.cc | 2 +- include/rocksdb/memtablerep.h | 24 +++++++--------- util/hash_skiplist_rep.cc | 54 ++++++++++++++--------------------- util/hash_skiplist_rep.h | 4 +-- util/skiplistrep.cc | 10 +++---- util/vectorrep.cc | 14 ++++----- 8 files changed, 58 insertions(+), 72 deletions(-) diff --git a/db/memtable.cc b/db/memtable.cc index baff4fb340..7eb4eb165a 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -85,11 +85,11 @@ class MemTableIterator: public Iterator { MemTableIterator(MemTableRep* table, const ReadOptions& options) : iter_() { if (options.prefix) { - iter_ = table->GetPrefixIterator(*options.prefix); + iter_.reset(table->GetPrefixIterator(*options.prefix)); } else if (options.prefix_seek) { - iter_ = table->GetDynamicPrefixIterator(); + iter_.reset(table->GetDynamicPrefixIterator()); } else { - iter_ = table->GetIterator(); + iter_.reset(table->GetIterator()); } } @@ -110,7 +110,7 @@ class MemTableIterator: public Iterator { virtual Status status() const { return Status::OK(); } private: - std::shared_ptr iter_; + std::unique_ptr iter_; std::string tmp_; // For passing to EncodeKey // No copying allowed @@ -161,8 +161,8 @@ void MemTable::Add(SequenceNumber s, ValueType type, bool MemTable::Get(const LookupKey& key, std::string* value, Status* s, MergeContext& merge_context, const Options& options) { Slice memkey = key.memtable_key(); - std::shared_ptr iter( - table_->GetIterator(key.user_key())); + std::unique_ptr iter( + table_->GetIterator(key.user_key())); iter->Seek(memkey.data()); bool merge_in_progress = s->IsMergeInProgress(); @@ -267,8 +267,8 @@ bool MemTable::Update(SequenceNumber seq, ValueType type, LookupKey lkey(key, seq); Slice memkey = lkey.memtable_key(); - std::shared_ptr iter( - table_->GetIterator(lkey.user_key())); + std::unique_ptr iter( + table_->GetIterator(lkey.user_key())); iter->Seek(memkey.data()); if (iter->Valid()) { @@ -329,8 +329,8 @@ size_t MemTable::CountSuccessiveMergeEntries(const LookupKey& key) { // A total ordered iterator is costly for some memtablerep (prefix aware // reps). By passing in the user key, we allow efficient iterator creation. // The iterator only needs to be ordered within the same user key. - std::shared_ptr iter( - table_->GetIterator(key.user_key())); + std::unique_ptr iter( + table_->GetIterator(key.user_key())); iter->Seek(memkey.data()); size_t num_successive_merges = 0; diff --git a/db/memtable.h b/db/memtable.h index 24a2c852bd..1b9005800e 100644 --- a/db/memtable.h +++ b/db/memtable.h @@ -143,7 +143,7 @@ class MemTable { KeyComparator comparator_; int refs_; ArenaImpl arena_impl_; - shared_ptr table_; + unique_ptr table_; // These are used to manage memtable flushes to storage bool flush_in_progress_; // started the flush diff --git a/db/version_set.cc b/db/version_set.cc index 64ebb14275..22135b947f 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -698,7 +698,7 @@ bool CompareSeqnoDescending(const Version::Fsize& first, return false; } -} // anonymous namespace +} // anonymous namespace void Version::UpdateFilesBySize() { // No need to sort the highest level because it is never compacted. diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index fcb782d415..2fca8d1610 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -111,27 +111,23 @@ class MemTableRep { }; // Return an iterator over the keys in this representation. - virtual std::shared_ptr GetIterator() = 0; + virtual Iterator* GetIterator() = 0; // Return an iterator over at least the keys with the specified user key. The // iterator may also allow access to other keys, but doesn't have to. Default: // GetIterator(). - virtual std::shared_ptr GetIterator(const Slice& user_key) { - return GetIterator(); - } + virtual Iterator* GetIterator(const Slice& user_key) { return GetIterator(); } // Return an iterator over at least the keys with the specified prefix. The // iterator may also allow access to other keys, but doesn't have to. Default: // GetIterator(). - virtual std::shared_ptr GetPrefixIterator(const Slice& prefix) { + virtual Iterator* GetPrefixIterator(const Slice& prefix) { return GetIterator(); } // Return an iterator that has a special Seek semantics. The result of // a Seek might only include keys with the same prefix as the target key. - virtual std::shared_ptr GetDynamicPrefixIterator() { - return GetIterator(); - } + virtual Iterator* GetDynamicPrefixIterator() { return GetIterator(); } protected: // When *key is an internal key concatenated with the value, returns the @@ -144,8 +140,8 @@ class MemTableRep { class MemTableRepFactory { public: virtual ~MemTableRepFactory() { }; - virtual std::shared_ptr CreateMemTableRep( - MemTableRep::KeyComparator&, Arena*) = 0; + virtual MemTableRep* CreateMemTableRep(MemTableRep::KeyComparator&, + Arena*) = 0; virtual const char* Name() const = 0; }; @@ -161,8 +157,8 @@ class VectorRepFactory : public MemTableRepFactory { const size_t count_; public: explicit VectorRepFactory(size_t count = 0) : count_(count) { } - virtual std::shared_ptr CreateMemTableRep( - MemTableRep::KeyComparator&, Arena*) override; + virtual MemTableRep* CreateMemTableRep(MemTableRep::KeyComparator&, + Arena*) override; virtual const char* Name() const override { return "VectorRepFactory"; } @@ -171,8 +167,8 @@ public: // This uses a skip list to store keys. It is the default. class SkipListFactory : public MemTableRepFactory { public: - virtual std::shared_ptr CreateMemTableRep( - MemTableRep::KeyComparator&, Arena*) override; + virtual MemTableRep* CreateMemTableRep(MemTableRep::KeyComparator&, + Arena*) override; virtual const char* Name() const override { return "SkipListFactory"; } diff --git a/util/hash_skiplist_rep.cc b/util/hash_skiplist_rep.cc index c669769e09..e9fe1573aa 100644 --- a/util/hash_skiplist_rep.cc +++ b/util/hash_skiplist_rep.cc @@ -31,17 +31,15 @@ class HashSkipListRep : public MemTableRep { virtual ~HashSkipListRep(); - virtual std::shared_ptr GetIterator() override; + virtual MemTableRep::Iterator* GetIterator() override; - virtual std::shared_ptr GetIterator( - const Slice& slice) override; + virtual MemTableRep::Iterator* GetIterator(const Slice& slice) override; - virtual std::shared_ptr GetPrefixIterator( - const Slice& prefix) override; - - virtual std::shared_ptr GetDynamicPrefixIterator() + virtual MemTableRep::Iterator* GetPrefixIterator(const Slice& prefix) override; + virtual MemTableRep::Iterator* GetDynamicPrefixIterator() override; + private: friend class DynamicIterator; typedef SkipList Bucket; @@ -208,18 +206,15 @@ class HashSkipListRep : public MemTableRep { virtual void SeekToLast() { } private: }; - - std::shared_ptr empty_iterator_; }; HashSkipListRep::HashSkipListRep(MemTableRep::KeyComparator& compare, - Arena* arena, const SliceTransform* transform, size_t bucket_size) - : bucket_size_(bucket_size), - transform_(transform), - compare_(compare), - arena_(arena), - empty_iterator_(std::make_shared()) { - + Arena* arena, const SliceTransform* transform, + size_t bucket_size) + : bucket_size_(bucket_size), + transform_(transform), + compare_(compare), + arena_(arena) { buckets_ = new port::AtomicPointer[bucket_size]; for (size_t i = 0; i < bucket_size_; ++i) { @@ -263,7 +258,7 @@ size_t HashSkipListRep::ApproximateMemoryUsage() { return sizeof(buckets_); } -std::shared_ptr HashSkipListRep::GetIterator() { +MemTableRep::Iterator* HashSkipListRep::GetIterator() { auto list = new Bucket(compare_, arena_); for (size_t i = 0; i < bucket_size_; ++i) { auto bucket = GetBucket(i); @@ -274,35 +269,30 @@ std::shared_ptr HashSkipListRep::GetIterator() { } } } - return std::make_shared(list); + return new Iterator(list); } -std::shared_ptr HashSkipListRep::GetPrefixIterator( - const Slice& prefix) { +MemTableRep::Iterator* HashSkipListRep::GetPrefixIterator(const Slice& prefix) { auto bucket = GetBucket(prefix); if (bucket == nullptr) { - return empty_iterator_; + return new EmptyIterator(); } - return std::make_shared(bucket, false); + return new Iterator(bucket, false); } -std::shared_ptr HashSkipListRep::GetIterator( - const Slice& slice) { +MemTableRep::Iterator* HashSkipListRep::GetIterator(const Slice& slice) { return GetPrefixIterator(transform_->Transform(slice)); } -std::shared_ptr - HashSkipListRep::GetDynamicPrefixIterator() { - return std::make_shared(*this); +MemTableRep::Iterator* HashSkipListRep::GetDynamicPrefixIterator() { + return new DynamicIterator(*this); } } // anon namespace -std::shared_ptr -HashSkipListRepFactory::CreateMemTableRep(MemTableRep::KeyComparator &compare, - Arena *arena) { - return std::make_shared(compare, arena, transform_, - bucket_count_); +MemTableRep* HashSkipListRepFactory::CreateMemTableRep( + MemTableRep::KeyComparator& compare, Arena* arena) { + return new HashSkipListRep(compare, arena, transform_, bucket_count_); } MemTableRepFactory* NewHashSkipListRepFactory( diff --git a/util/hash_skiplist_rep.h b/util/hash_skiplist_rep.h index b946cf05ef..7b8414c887 100644 --- a/util/hash_skiplist_rep.h +++ b/util/hash_skiplist_rep.h @@ -21,8 +21,8 @@ class HashSkipListRepFactory : public MemTableRepFactory { virtual ~HashSkipListRepFactory() { delete transform_; } - virtual std::shared_ptr CreateMemTableRep( - MemTableRep::KeyComparator& compare, Arena* arena) override; + virtual MemTableRep* CreateMemTableRep(MemTableRep::KeyComparator& compare, + Arena* arena) override; virtual const char* Name() const override { return "HashSkipListRepFactory"; diff --git a/util/skiplistrep.cc b/util/skiplistrep.cc index 955d754b15..a5b072ad16 100644 --- a/util/skiplistrep.cc +++ b/util/skiplistrep.cc @@ -90,15 +90,15 @@ public: // Unhide default implementations of GetIterator using MemTableRep::GetIterator; - virtual std::shared_ptr GetIterator() override { - return std::make_shared(&skip_list_); + virtual MemTableRep::Iterator* GetIterator() override { + return new SkipListRep::Iterator(&skip_list_); } }; } -std::shared_ptr SkipListFactory::CreateMemTableRep ( - MemTableRep::KeyComparator& compare, Arena* arena) { - return std::shared_ptr(new SkipListRep(compare, arena)); +MemTableRep* SkipListFactory::CreateMemTableRep( + MemTableRep::KeyComparator& compare, Arena* arena) { + return new SkipListRep(compare, arena); } } // namespace rocksdb diff --git a/util/vectorrep.cc b/util/vectorrep.cc index 8d3ccc9dfb..87fae4bc72 100644 --- a/util/vectorrep.cc +++ b/util/vectorrep.cc @@ -88,7 +88,7 @@ class VectorRep : public MemTableRep { using MemTableRep::GetIterator; // Return an iterator over the keys in this representation. - virtual std::shared_ptr GetIterator() override; + virtual MemTableRep::Iterator* GetIterator() override; private: friend class Iterator; @@ -228,22 +228,22 @@ void VectorRep::Iterator::SeekToLast() { } } -std::shared_ptr VectorRep::GetIterator() { +MemTableRep::Iterator* VectorRep::GetIterator() { ReadLock l(&rwlock_); // Do not sort here. The sorting would be done the first time // a Seek is performed on the iterator. if (immutable_) { - return std::make_shared(this, bucket_, compare_); + return new Iterator(this, bucket_, compare_); } else { std::shared_ptr tmp; tmp.reset(new Bucket(*bucket_)); // make a copy - return std::make_shared(nullptr, tmp, compare_); + return new Iterator(nullptr, tmp, compare_); } } } // anon namespace -std::shared_ptr VectorRepFactory::CreateMemTableRep( - MemTableRep::KeyComparator& compare, Arena* arena) { - return std::make_shared(compare, arena, count_); +MemTableRep* VectorRepFactory::CreateMemTableRep( + MemTableRep::KeyComparator& compare, Arena* arena) { + return new VectorRep(compare, arena, count_); } } // namespace rocksdb