From 41c328fe57bea83b819e4c184d95a81bfbdd10bb Mon Sep 17 00:00:00 2001 From: sdong Date: Tue, 4 Aug 2020 11:28:02 -0700 Subject: [PATCH] Fix a perf regression that caused every key to go through upper bound check (#7209) Summary: https://github.com/facebook/rocksdb/pull/5289 introduces a performance regression that caused an upper bound check within every BlockBasedTableIterator::Next(). This is unnecessary if we've checked the boundary key for current block and it is within upper bound. Fix the bug. Also rename the boolean to a enum so that the code is slightly better readable. The original regression was probably to fix a bug that the block upper bound check status is not reset after a new block is created. Fix it bug so that the regression can be avoided without hitting the bug. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7209 Test Plan: Run all existing tests. Will run atomic black box crash test for a while. Reviewed By: anand1976 Differential Revision: D22859246 fbshipit-source-id: cbdad1f5e656c55fd8b71726d5a4f6cb53ff9140 --- HISTORY.md | 3 ++ .../block_based/block_based_table_iterator.cc | 18 +++++---- .../block_based/block_based_table_iterator.h | 37 +++++++++++++++++-- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 2030c4cf6..7e689a855 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,8 @@ # Rocksdb Change Log ## Unreleased +### Bug fixes +* Fix a performance regression introduced in 6.4 that makes a upper bound check for every Next() even if keys are within a data block that is within the upper bound. + ## 6.12 (2020-07-28) ### Public API Change diff --git a/table/block_based/block_based_table_iterator.cc b/table/block_based/block_based_table_iterator.cc index aa398766b..af81799a0 100644 --- a/table/block_based/block_based_table_iterator.cc +++ b/table/block_based/block_based_table_iterator.cc @@ -300,7 +300,8 @@ void BlockBasedTableIterator::FindBlockForward() { // Whether next data block is out of upper bound, if there is one. const bool next_block_is_out_of_bound = read_options_.iterate_upper_bound != nullptr && - block_iter_points_to_real_block_ && !data_block_within_upper_bound_; + block_iter_points_to_real_block_ && + block_upper_bound_check_ == BlockUpperBound::kUpperBoundInCurBlock; assert(!next_block_is_out_of_bound || user_comparator_.CompareWithoutTimestamp( *read_options_.iterate_upper_bound, /*a_has_ts=*/false, @@ -358,7 +359,9 @@ void BlockBasedTableIterator::FindKeyBackward() { } void BlockBasedTableIterator::CheckOutOfBound() { - if (read_options_.iterate_upper_bound != nullptr && Valid()) { + if (read_options_.iterate_upper_bound != nullptr && + block_upper_bound_check_ != BlockUpperBound::kUpperBoundBeyondCurBlock && + Valid()) { is_out_of_bound_ = user_comparator_.CompareWithoutTimestamp( *read_options_.iterate_upper_bound, /*a_has_ts=*/false, user_key(), @@ -369,11 +372,12 @@ void BlockBasedTableIterator::CheckOutOfBound() { void BlockBasedTableIterator::CheckDataBlockWithinUpperBound() { if (read_options_.iterate_upper_bound != nullptr && block_iter_points_to_real_block_) { - data_block_within_upper_bound_ = - (user_comparator_.CompareWithoutTimestamp( - *read_options_.iterate_upper_bound, /*a_has_ts=*/false, - index_iter_->user_key(), - /*b_has_ts=*/true) > 0); + block_upper_bound_check_ = (user_comparator_.CompareWithoutTimestamp( + *read_options_.iterate_upper_bound, + /*a_has_ts=*/false, index_iter_->user_key(), + /*b_has_ts=*/true) > 0) + ? BlockUpperBound::kUpperBoundBeyondCurBlock + : BlockUpperBound::kUpperBoundInCurBlock; } } } // namespace ROCKSDB_NAMESPACE diff --git a/table/block_based/block_based_table_iterator.h b/table/block_based/block_based_table_iterator.h index 026ac9ee7..ac3f47b68 100644 --- a/table/block_based/block_based_table_iterator.h +++ b/table/block_based/block_based_table_iterator.h @@ -104,7 +104,8 @@ class BlockBasedTableIterator : public InternalIteratorBase { inline bool MayBeOutOfUpperBound() override { assert(Valid()); - return !data_block_within_upper_bound_; + return block_upper_bound_check_ != + BlockUpperBound::kUpperBoundBeyondCurBlock; } void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override { @@ -134,6 +135,7 @@ class BlockBasedTableIterator : public InternalIteratorBase { block_iter_.Invalidate(Status::OK()); block_iter_points_to_real_block_ = false; } + block_upper_bound_check_ = BlockUpperBound::kUnknown; } void SavePrevIndexValue() { @@ -149,6 +151,34 @@ class BlockBasedTableIterator : public InternalIteratorBase { kForward, kBackward, }; + // This enum indicates whether the upper bound falls into current block + // or beyond. + // +-------------+ + // | cur block | <-- (1) + // +-------------+ + // <-- (2) + // --- --- + // <-- (3) + // +-------------+ + // | next block | <-- (4) + // ...... + // + // When the block is smaller than , kUpperBoundInCurBlock + // is the value to use. The examples are (1) or (2) in the graph. It means + // all keys in the next block or beyond will be out of bound. Keys within + // the current block may or may not be out of bound. + // When the block is larger or equal to , + // kUpperBoundBeyondCurBlock is to be used. The examples are (3) and (4) + // in the graph. It means that all keys in the current block is within the + // upper bound and keys in the next block may or may not be within the uppder + // bound. + // If the boundary key hasn't been checked against the upper bound, + // kUnknown can be used. + enum class BlockUpperBound { + kUpperBoundInCurBlock, + kUpperBoundBeyondCurBlock, + kUnknown, + }; const BlockBasedTable* table_; const ReadOptions& read_options_; @@ -169,8 +199,9 @@ class BlockBasedTableIterator : public InternalIteratorBase { bool block_iter_points_to_real_block_; // See InternalIteratorBase::IsOutOfBound(). bool is_out_of_bound_ = false; - // Whether current data block being fully within iterate upper bound. - bool data_block_within_upper_bound_ = false; + // How current data block's boundary key with the next block is compared with + // iterate upper bound. + BlockUpperBound block_upper_bound_check_ = BlockUpperBound::kUnknown; // True if we're standing at the first key of a block, and we haven't loaded // that block yet. A call to PrepareValue() will trigger loading the block. bool is_at_first_key_from_index_ = false;