From 64b6452e0ca6412e333cb28f9c67a708a50fc4db Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Mon, 11 Sep 2017 11:53:22 -0700 Subject: [PATCH] Make InternalKeyComparator final and directly use it in merging iterator Summary: Merging iterator invokes InternalKeyComparator.Compare() frequently to heap merge. By making InternalKeyComparator final and merging iterator to directly use InternalKeyComparator rather than through Iterator interface, we can give compiler a choice to avoid one more virtual function call if possible. I ran readseq benchmark in memory-only use case to make sure the performance at least doesn't regress. I have to disable the final key word in debug build, as a hack test class depends on overriding the class. Closes https://github.com/facebook/rocksdb/pull/2860 Differential Revision: D5800461 Pulled By: siying fbshipit-source-id: ab876f22a09bb5c560740911412336e0e25ccb53 --- db/dbformat.h | 6 +++++- table/iter_heap.h | 14 +++++++------- table/merger_test.cc | 19 +++++++++++++++---- table/merging_iterator.cc | 14 ++++++++------ table/merging_iterator.h | 12 ++++++------ util/testutil.h | 2 ++ utilities/date_tiered/date_tiered_db_impl.cc | 3 ++- utilities/date_tiered/date_tiered_db_impl.h | 2 ++ 8 files changed, 47 insertions(+), 25 deletions(-) diff --git a/db/dbformat.h b/db/dbformat.h index d9fd5f399..04d1f87f0 100644 --- a/db/dbformat.h +++ b/db/dbformat.h @@ -136,7 +136,11 @@ inline ValueType ExtractValueType(const Slice& internal_key) { // A comparator for internal keys that uses a specified comparator for // the user key portion and breaks ties by decreasing sequence number. -class InternalKeyComparator : public Comparator { +class InternalKeyComparator +#ifdef NDEBUG + final +#endif + : public Comparator { private: const Comparator* user_comparator_; std::string name_; diff --git a/table/iter_heap.h b/table/iter_heap.h index 74c06caea..f30c12272 100644 --- a/table/iter_heap.h +++ b/table/iter_heap.h @@ -6,7 +6,7 @@ #pragma once -#include "rocksdb/comparator.h" +#include "db/dbformat.h" #include "table/iterator_wrapper.h" namespace rocksdb { @@ -15,28 +15,28 @@ namespace rocksdb { // iterator with the max/largest key on top. class MaxIteratorComparator { public: - MaxIteratorComparator(const Comparator* comparator) : - comparator_(comparator) {} + MaxIteratorComparator(const InternalKeyComparator* comparator) + : comparator_(comparator) {} bool operator()(IteratorWrapper* a, IteratorWrapper* b) const { return comparator_->Compare(a->key(), b->key()) < 0; } private: - const Comparator* comparator_; + const InternalKeyComparator* comparator_; }; // When used with std::priority_queue, this comparison functor puts the // iterator with the min/smallest key on top. class MinIteratorComparator { public: - MinIteratorComparator(const Comparator* comparator) : - comparator_(comparator) {} + MinIteratorComparator(const InternalKeyComparator* comparator) + : comparator_(comparator) {} bool operator()(IteratorWrapper* a, IteratorWrapper* b) const { return comparator_->Compare(a->key(), b->key()) > 0; } private: - const Comparator* comparator_; + const InternalKeyComparator* comparator_; }; } // namespace rocksdb diff --git a/table/merger_test.cc b/table/merger_test.cc index 379a6f412..f20ed187a 100644 --- a/table/merger_test.cc +++ b/table/merger_test.cc @@ -15,12 +15,18 @@ namespace rocksdb { class MergerTest : public testing::Test { public: MergerTest() - : rnd_(3), merging_iterator_(nullptr), single_iterator_(nullptr) {} + : icomp_(BytewiseComparator()), + rnd_(3), + merging_iterator_(nullptr), + single_iterator_(nullptr) {} ~MergerTest() = default; std::vector GenerateStrings(size_t len, int string_len) { std::vector ret; + for (size_t i = 0; i < len; ++i) { - ret.push_back(test::RandomHumanReadableString(&rnd_, string_len)); + InternalKey ik(test::RandomHumanReadableString(&rnd_, string_len), 0, + ValueType::kTypeValue); + ret.push_back(ik.Encode().ToString(false)); } return ret; } @@ -37,7 +43,11 @@ class MergerTest : public testing::Test { } } - void SeekToRandom() { Seek(test::RandomHumanReadableString(&rnd_, 5)); } + void SeekToRandom() { + InternalKey ik(test::RandomHumanReadableString(&rnd_, 5), 0, + ValueType::kTypeValue); + Seek(ik.Encode().ToString(false)); + } void Seek(std::string target) { merging_iterator_->Seek(target); @@ -96,11 +106,12 @@ class MergerTest : public testing::Test { } merging_iterator_.reset( - NewMergingIterator(BytewiseComparator(), &small_iterators[0], + NewMergingIterator(&icomp_, &small_iterators[0], static_cast(small_iterators.size()))); single_iterator_.reset(new test::VectorIterator(all_keys_)); } + InternalKeyComparator icomp_; Random rnd_; std::unique_ptr merging_iterator_; std::unique_ptr single_iterator_; diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc index da30e1e63..4f1ab4ffe 100644 --- a/table/merging_iterator.cc +++ b/table/merging_iterator.cc @@ -10,6 +10,7 @@ #include "table/merging_iterator.h" #include #include +#include "db/dbformat.h" #include "db/pinned_iterators_manager.h" #include "monitoring/perf_context_imp.h" #include "rocksdb/comparator.h" @@ -35,8 +36,9 @@ const size_t kNumIterReserve = 4; class MergingIterator : public InternalIterator { public: - MergingIterator(const Comparator* comparator, InternalIterator** children, - int n, bool is_arena_mode, bool prefix_seek_mode) + MergingIterator(const InternalKeyComparator* comparator, + InternalIterator** children, int n, bool is_arena_mode, + bool prefix_seek_mode) : is_arena_mode_(is_arena_mode), comparator_(comparator), current_(nullptr), @@ -307,7 +309,7 @@ class MergingIterator : public InternalIterator { void InitMaxHeap(); bool is_arena_mode_; - const Comparator* comparator_; + const InternalKeyComparator* comparator_; autovector children_; // Cached pointer to child iterator with the current key, or nullptr if no @@ -353,7 +355,7 @@ void MergingIterator::InitMaxHeap() { } } -InternalIterator* NewMergingIterator(const Comparator* cmp, +InternalIterator* NewMergingIterator(const InternalKeyComparator* cmp, InternalIterator** list, int n, Arena* arena, bool prefix_seek_mode) { assert(n >= 0); @@ -371,8 +373,8 @@ InternalIterator* NewMergingIterator(const Comparator* cmp, } } -MergeIteratorBuilder::MergeIteratorBuilder(const Comparator* comparator, - Arena* a, bool prefix_seek_mode) +MergeIteratorBuilder::MergeIteratorBuilder( + const InternalKeyComparator* comparator, Arena* a, bool prefix_seek_mode) : first_iter(nullptr), use_merging_iter(false), arena(a) { auto mem = arena->AllocateAligned(sizeof(MergingIterator)); merge_iter = diff --git a/table/merging_iterator.h b/table/merging_iterator.h index 48a28d86f..0509d1dce 100644 --- a/table/merging_iterator.h +++ b/table/merging_iterator.h @@ -9,6 +9,7 @@ #pragma once +#include "db/dbformat.h" #include "rocksdb/types.h" namespace rocksdb { @@ -26,10 +27,9 @@ class Arena; // key is present in K child iterators, it will be yielded K times. // // REQUIRES: n >= 0 -extern InternalIterator* NewMergingIterator(const Comparator* comparator, - InternalIterator** children, int n, - Arena* arena = nullptr, - bool prefix_seek_mode = false); +extern InternalIterator* NewMergingIterator( + const InternalKeyComparator* comparator, InternalIterator** children, int n, + Arena* arena = nullptr, bool prefix_seek_mode = false); class MergingIterator; @@ -38,8 +38,8 @@ class MergeIteratorBuilder { public: // comparator: the comparator used in merging comparator // arena: where the merging iterator needs to be allocated from. - explicit MergeIteratorBuilder(const Comparator* comparator, Arena* arena, - bool prefix_seek_mode = false); + explicit MergeIteratorBuilder(const InternalKeyComparator* comparator, + Arena* arena, bool prefix_seek_mode = false); ~MergeIteratorBuilder() {} // Add iter to the merging iterator. diff --git a/util/testutil.h b/util/testutil.h index 02bfb0ff6..6683963af 100644 --- a/util/testutil.h +++ b/util/testutil.h @@ -72,6 +72,7 @@ class ErrorEnv : public EnvWrapper { } }; +#ifndef NDEBUG // An internal comparator that just forward comparing results from the // user comparator in it. Can be used to test entities that have no dependency // on internal key structure but consumes InternalKeyComparator, like @@ -94,6 +95,7 @@ class PlainInternalKeyComparator : public InternalKeyComparator { user_comparator()->FindShortSuccessor(key); } }; +#endif // A test comparator which compare two strings in this way: // (1) first compare prefix of 8 bytes in alphabet order, diff --git a/utilities/date_tiered/date_tiered_db_impl.cc b/utilities/date_tiered/date_tiered_db_impl.cc index c1b1ceb5e..83f5666e5 100644 --- a/utilities/date_tiered/date_tiered_db_impl.cc +++ b/utilities/date_tiered/date_tiered_db_impl.cc @@ -32,6 +32,7 @@ DateTieredDBImpl::DateTieredDBImpl( : db_(db), cf_options_(ColumnFamilyOptions(options)), ioptions_(ImmutableCFOptions(options)), + icomp_(cf_options_.comparator), ttl_(ttl), column_family_interval_(column_family_interval), mutex_(options.statistics.get(), db->GetEnv(), DB_MUTEX_WAIT_MICROS, @@ -382,7 +383,7 @@ Iterator* DateTieredDBImpl::NewIterator(const ReadOptions& opts) { cf_options_.max_sequential_skip_in_iterations, 0); auto arena = db_iter->GetArena(); - MergeIteratorBuilder builder(cf_options_.comparator, arena); + MergeIteratorBuilder builder(&icomp_, arena); for (auto& item : handle_map_) { auto handle = item.second; builder.AddIterator(db_impl->NewInternalIterator( diff --git a/utilities/date_tiered/date_tiered_db_impl.h b/utilities/date_tiered/date_tiered_db_impl.h index 2236cff8c..bbefabe33 100644 --- a/utilities/date_tiered/date_tiered_db_impl.h +++ b/utilities/date_tiered/date_tiered_db_impl.h @@ -56,6 +56,8 @@ class DateTieredDBImpl : public DateTieredDB { const ImmutableCFOptions ioptions_; + const InternalKeyComparator icomp_; + // Storing all column family handles for time series data. std::vector handles_;