From 92ee3350e0ae02c0973af0fbd40fb67b0b958128 Mon Sep 17 00:00:00 2001
From: Siying Dong <siying.d@fb.com>
Date: Tue, 19 Jun 2018 09:42:40 -0700
Subject: [PATCH] BlockBasedTableIterator to keep BlockIter after out of upper
 bound (#4004)

Summary:
b555ed30a4a93b80a3ac4781c6721ab988e03b5b makes the BlockBasedTableIterator to be invalidated if the current position if over the upper bound. However, this can bring performance regression to the case of multiple Seek()s hitting the same data block but all out of upper bound.

For example, if an SST file has a data block containing following keys : {a, z}

The user sets the upper bound to be "x", and it executed following queries:
Seek("b")
Seek("c")
Seek("d")

Before the upper bound optimization, these queries always come to this same current data block of the iterator, but now inside each Seek() the data block is read from the block cache but is returned again.

To prevent this regression case, we keep the current data block iterator if it is upper bound.
Closes https://github.com/facebook/rocksdb/pull/4004

Differential Revision: D8463192

Pulled By: siying

fbshipit-source-id: 8710628b30acde7063a097c3184d6c4333a8ef81
---
 db/db_iterator_test.cc            | 54 +++++++++++++++++++++++++++++++
 table/block_based_table_reader.cc |  1 -
 table/block_based_table_reader.h  |  3 +-
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc
index a7b389cf37..7eeb12118d 100644
--- a/db/db_iterator_test.cc
+++ b/db/db_iterator_test.cc
@@ -1035,6 +1035,60 @@ TEST_P(DBIteratorTest, DBIteratorBoundTest) {
     ASSERT_EQ(static_cast<int>(get_perf_context()->internal_delete_skipped_count), 0);
   }
 }
+
+TEST_P(DBIteratorTest, DBIteratorBoundMultiSeek) {
+  Options options = CurrentOptions();
+  options.env = env_;
+  options.create_if_missing = true;
+  options.statistics = rocksdb::CreateDBStatistics();
+  options.prefix_extractor = nullptr;
+  DestroyAndReopen(options);
+  ASSERT_OK(Put("a", "0"));
+  ASSERT_OK(Put("z", "0"));
+  ASSERT_OK(Flush());
+  ASSERT_OK(Put("foo1", "bar1"));
+  ASSERT_OK(Put("foo2", "bar2"));
+  ASSERT_OK(Put("foo3", "bar3"));
+  ASSERT_OK(Put("foo4", "bar4"));
+
+  {
+    std::string up_str = "foo5";
+    Slice up(up_str);
+    ReadOptions ro;
+    ro.iterate_upper_bound = &up;
+    std::unique_ptr<Iterator> iter(NewIterator(ro));
+
+    iter->Seek("foo1");
+    ASSERT_TRUE(iter->Valid());
+    ASSERT_EQ(iter->key().compare(Slice("foo1")), 0);
+
+    uint64_t prev_block_cache_hit =
+        TestGetTickerCount(options, BLOCK_CACHE_HIT);
+    uint64_t prev_block_cache_miss =
+        TestGetTickerCount(options, BLOCK_CACHE_MISS);
+
+    ASSERT_GT(prev_block_cache_hit + prev_block_cache_miss, 0);
+
+    iter->Seek("foo4");
+    ASSERT_TRUE(iter->Valid());
+    ASSERT_EQ(iter->key().compare(Slice("foo4")), 0);
+    ASSERT_EQ(prev_block_cache_hit,
+              TestGetTickerCount(options, BLOCK_CACHE_HIT));
+    ASSERT_EQ(prev_block_cache_miss,
+              TestGetTickerCount(options, BLOCK_CACHE_MISS));
+
+    iter->Seek("foo2");
+    ASSERT_TRUE(iter->Valid());
+    ASSERT_EQ(iter->key().compare(Slice("foo2")), 0);
+    iter->Next();
+    ASSERT_TRUE(iter->Valid());
+    ASSERT_EQ(iter->key().compare(Slice("foo3")), 0);
+    ASSERT_EQ(prev_block_cache_hit,
+              TestGetTickerCount(options, BLOCK_CACHE_HIT));
+    ASSERT_EQ(prev_block_cache_miss,
+              TestGetTickerCount(options, BLOCK_CACHE_MISS));
+  }
+}
 #endif
 
 TEST_P(DBIteratorTest, DBIteratorBoundOptimizationTest) {
diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc
index 6c8a880f70..3d90940e2f 100644
--- a/table/block_based_table_reader.cc
+++ b/table/block_based_table_reader.cc
@@ -2039,7 +2039,6 @@ void BlockBasedTableIterator::FindKeyForward() {
       &reached_upper_bound);
   if (reached_upper_bound) {
     is_out_of_bound_ = true;
-    ResetDataIter();
     return;
   }
 }
diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h
index 6c39e567cf..cedab053ca 100644
--- a/table/block_based_table_reader.h
+++ b/table/block_based_table_reader.h
@@ -535,7 +535,8 @@ class BlockBasedTableIterator : public InternalIterator {
   void Next() override;
   void Prev() override;
   bool Valid() const override {
-    return block_iter_points_to_real_block_ && data_block_iter_.Valid();
+    return !is_out_of_bound_ && block_iter_points_to_real_block_ &&
+           data_block_iter_.Valid();
   }
   Slice key() const override {
     assert(Valid());