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

@@ -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_;

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) :
+  MaxIteratorComparator(const InternalKeyComparator* comparator)
-    comparator_(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) :
+  MinIteratorComparator(const InternalKeyComparator* comparator)
-    comparator_(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

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<std::string> GenerateStrings(size_t len, int string_len) {
     std::vector<std::string> 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<int>(small_iterators.size())));
     single_iterator_.reset(new test::VectorIterator(all_keys_));
   }
 
+  InternalKeyComparator icomp_;
   Random rnd_;
   std::unique_ptr<InternalIterator> merging_iterator_;
   std::unique_ptr<InternalIterator> single_iterator_;

table/merging_iterator.cc

@@ -10,6 +10,7 @@
 #include "table/merging_iterator.h"
 #include <string>
 #include <vector>
+#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,
+  MergingIterator(const InternalKeyComparator* comparator,
-                  int n, bool is_arena_mode, bool prefix_seek_mode)
+                  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<IteratorWrapper, kNumIterReserve> 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,
+MergeIteratorBuilder::MergeIteratorBuilder(
-                                           Arena* a, bool prefix_seek_mode)
+    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 =

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,
+extern InternalIterator* NewMergingIterator(
-                                            InternalIterator** children, int n,
+    const InternalKeyComparator* comparator, InternalIterator** children, int n,
-                                            Arena* arena = nullptr,
+    Arena* arena = nullptr, bool prefix_seek_mode = false);
-                                            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,
+  explicit MergeIteratorBuilder(const InternalKeyComparator* comparator,
-                                bool prefix_seek_mode = false);
+                                Arena* arena, bool prefix_seek_mode = false);
   ~MergeIteratorBuilder() {}
 
   // Add iter to the merging iterator.

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,

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(

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<ColumnFamilyHandle*> handles_;