From f29dc1b90641e7f44b14f932e3866c5840391cd5 Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Fri, 29 Mar 2019 13:07:44 -0700 Subject: [PATCH] Avoid per-key upper bound check in BlockBasedTableIterator (#5101) Summary: `BlockBasedTableIterator` avoid reading next block on `Next()` if it detects the iterator will be out of bound, by checking against index key. The optimization was added in #2239, and by the time it only check the bound per block. It seems later change make it a per-key check, which introduce unnecessary key comparisons. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5101 Differential Revision: D14678707 Pulled By: siying fbshipit-source-id: 2372446116753c7892ea4cec7b4b49ef87ba463e --- 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, 56 insertions(+), 25 deletions(-) diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index e24b7c3b7..fb9c04b9f 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -1026,11 +1026,8 @@ TEST_P(DBIteratorTest, DBIteratorBoundOptimizationTest) { int upper_bound_hits = 0; Options options = CurrentOptions(); rocksdb::SyncPoint::GetInstance()->SetCallBack( - "BlockBasedTable::BlockEntryIteratorState::KeyReachedUpperBound", - [&upper_bound_hits](void* arg) { - assert(arg != nullptr); - upper_bound_hits += (*static_cast(arg) ? 1 : 0); - }); + "BlockBasedTableIterator:out_of_bound", + [&upper_bound_hits](void*) { upper_bound_hits++; }); 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 0ca0a6655..ae4e636f5 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -2346,6 +2346,7 @@ 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) || @@ -2409,6 +2410,7 @@ void BlockBasedTableIterator::SeekToFirst() { InitDataBlock(); block_iter_.SeekToFirst(); FindKeyForward(); + CheckOutOfBound(); } template @@ -2491,18 +2493,24 @@ 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(); - // 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. + 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; + } index_iter_->Next(); if (index_iter_->Valid()) { @@ -2512,25 +2520,10 @@ 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; @@ -2551,6 +2544,16 @@ 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 f0b5cdb1b..50849fcc5 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -623,6 +623,7 @@ 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 { @@ -673,6 +674,7 @@ 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 666628f44..8ccb2cbb0 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -3842,6 +3842,35 @@ 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) {