From ebcc8ae1d3875aa9529b05b31c3da396cdf0c7bc Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Tue, 2 Apr 2019 09:57:35 -0700 Subject: [PATCH] Revert "Avoid per-key upper bound check in BlockBasedTableIterator (#5101)" (#5132) Summary: This reverts commit f29dc1b90641e7f44b14f932e3866c5840391cd5. In BlockBasedTableIterator, index_iter_->key() is sometimes a user key, so it is wrong to call ExtractUserKey() against it. This is a bug introduced by #5101. Temporarily revert the diff to keep the branch clean. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5132 Differential Revision: D14718584 Pulled By: siying fbshipit-source-id: 0ac55dc9b5dbc18c7809092146bdf7eb9364b9ad --- db/db_iterator_test.cc | 7 +++-- table/block_based_table_reader.cc | 43 ++++++++++++++----------------- table/block_based_table_reader.h | 2 -- table/table_test.cc | 29 --------------------- 4 files changed, 25 insertions(+), 56 deletions(-) diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index fb9c04b9f..e24b7c3b7 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -1026,8 +1026,11 @@ TEST_P(DBIteratorTest, DBIteratorBoundOptimizationTest) { int upper_bound_hits = 0; Options options = CurrentOptions(); rocksdb::SyncPoint::GetInstance()->SetCallBack( - "BlockBasedTableIterator:out_of_bound", - [&upper_bound_hits](void*) { upper_bound_hits++; }); + "BlockBasedTable::BlockEntryIteratorState::KeyReachedUpperBound", + [&upper_bound_hits](void* arg) { + assert(arg != nullptr); + upper_bound_hits += (*static_cast(arg) ? 1 : 0); + }); rocksdb::SyncPoint::GetInstance()->EnableProcessing(); options.env = env_; options.create_if_missing = true; diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index ae4e636f5..0ca0a6655 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -2346,7 +2346,6 @@ void BlockBasedTableIterator::Seek(const Slice& target) { block_iter_.Seek(target); FindKeyForward(); - CheckOutOfBound(); assert( !block_iter_.Valid() || (key_includes_seq_ && icomp_.Compare(target, block_iter_.key()) <= 0) || @@ -2410,7 +2409,6 @@ void BlockBasedTableIterator::SeekToFirst() { InitDataBlock(); block_iter_.SeekToFirst(); FindKeyForward(); - CheckOutOfBound(); } template @@ -2493,24 +2491,18 @@ void BlockBasedTableIterator::InitDataBlock() { template void BlockBasedTableIterator::FindKeyForward() { + assert(!is_out_of_bound_); // TODO the while loop inherits from two-level-iterator. We don't know // whether a block can be empty so it can be replaced by an "if". while (!block_iter_.Valid()) { if (!block_iter_.status().ok()) { return; } - if (read_options_.iterate_upper_bound != nullptr && - block_iter_points_to_real_block_) { - is_out_of_bound_ = - (user_comparator_.Compare(*read_options_.iterate_upper_bound, - ExtractUserKey(index_iter_->key())) <= 0); - } ResetDataIter(); - if (is_out_of_bound_) { - // The next block is out of bound. No need to read it. - TEST_SYNC_POINT_CALLBACK("BlockBasedTableIterator:out_of_bound", nullptr); - return; - } + // We used to check the current index key for upperbound. + // It will only save a data reading for a small percentage of use cases, + // so for code simplicity, we removed it. We can add it back if there is a + // significnat performance regression. index_iter_->Next(); if (index_iter_->Valid()) { @@ -2520,10 +2512,25 @@ void BlockBasedTableIterator::FindKeyForward() { return; } } + + // Check upper bound on the current key + bool reached_upper_bound = + (read_options_.iterate_upper_bound != nullptr && + block_iter_points_to_real_block_ && block_iter_.Valid() && + user_comparator_.Compare(ExtractUserKey(block_iter_.key()), + *read_options_.iterate_upper_bound) >= 0); + TEST_SYNC_POINT_CALLBACK( + "BlockBasedTable::BlockEntryIteratorState::KeyReachedUpperBound", + &reached_upper_bound); + if (reached_upper_bound) { + is_out_of_bound_ = true; + return; + } } template void BlockBasedTableIterator::FindKeyBackward() { + assert(!is_out_of_bound_); while (!block_iter_.Valid()) { if (!block_iter_.status().ok()) { return; @@ -2544,16 +2551,6 @@ void BlockBasedTableIterator::FindKeyBackward() { // code simplicity. } -template -void BlockBasedTableIterator::CheckOutOfBound() { - if (read_options_.iterate_upper_bound != nullptr && - block_iter_points_to_real_block_ && block_iter_.Valid()) { - is_out_of_bound_ = - user_comparator_.Compare(*read_options_.iterate_upper_bound, - ExtractUserKey(block_iter_.key())) <= 0; - } -} - InternalIterator* BlockBasedTable::NewIterator( const ReadOptions& read_options, const SliceTransform* prefix_extractor, Arena* arena, bool skip_filters, bool for_compaction) { diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index 50849fcc5..f0b5cdb1b 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -623,7 +623,6 @@ class BlockBasedTableIterator : public InternalIteratorBase { } } - // Whether iterator invalidated for being out of bound. bool IsOutOfBound() override { return is_out_of_bound_; } void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override { @@ -674,7 +673,6 @@ class BlockBasedTableIterator : public InternalIteratorBase { void InitDataBlock(); void FindKeyForward(); void FindKeyBackward(); - void CheckOutOfBound(); private: BlockBasedTable* table_; diff --git a/table/table_test.cc b/table/table_test.cc index 8ccb2cbb0..666628f44 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -3842,35 +3842,6 @@ TEST_P(BlockBasedTableTest, DataBlockHashIndex) { } } -// BlockBasedTableIterator should invalidate itself and return -// OutOfBound()=true immediately after Seek(), to allow LevelIterator -// filter out corresponding level. -TEST_P(BlockBasedTableTest, OutOfBoundOnSeek) { - TableConstructor c(BytewiseComparator(), true /*convert_to_internal_key*/); - c.Add("foo", "v1"); - std::vector keys; - stl_wrappers::KVMap kvmap; - Options options; - const ImmutableCFOptions ioptions(options); - const MutableCFOptions moptions(options); - c.Finish(options, ioptions, moptions, BlockBasedTableOptions(), - GetPlainInternalComparator(BytewiseComparator()), &keys, &kvmap); - auto* reader = c.GetTableReader(); - ReadOptions read_opt; - std::string upper_bound = "bar"; - Slice upper_bound_slice(upper_bound); - read_opt.iterate_upper_bound = &upper_bound_slice; - std::unique_ptr iter; - iter.reset(reader->NewIterator(read_opt, nullptr /*prefix_extractor*/)); - iter->SeekToFirst(); - ASSERT_FALSE(iter->Valid()); - ASSERT_TRUE(iter->IsOutOfBound()); - iter.reset(reader->NewIterator(read_opt, nullptr /*prefix_extractor*/)); - iter->Seek("foo"); - ASSERT_FALSE(iter->Valid()); - ASSERT_TRUE(iter->IsOutOfBound()); -} - } // namespace rocksdb int main(int argc, char** argv) {