Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce iterator key comparison for upper/lower bound check #5111

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Reduce iterator key comparison for upper/lower bound check
Signed-off-by: Yi Wu <yiwu@pingcap.com>
  • Loading branch information
Yi Wu committed Apr 5, 2019
commit dc3728d2a4424095483d220757845539f3aa3769
4 changes: 2 additions & 2 deletions db/db_iter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping, bool prefix_check) {
return false;
}

if (iterate_upper_bound_ != nullptr &&
if (iterate_upper_bound_ != nullptr && !iter_->HintWithinUpperBound() &&
yiwu-arbug marked this conversation as resolved.
Show resolved Hide resolved
user_comparator_.Compare(ikey_.user_key, *iterate_upper_bound_) >= 0) {
break;
}
Expand Down Expand Up @@ -838,7 +838,7 @@ void DBIter::PrevInternal() {
return;
}

if (iterate_lower_bound_ != nullptr &&
if (iterate_lower_bound_ != nullptr && !iter_->HintWithinLowerBound() &&
user_comparator_.Compare(saved_key_.GetUserKey(),
*iterate_lower_bound_) < 0) {
// We've iterated earlier than the user-specified lower bound.
Expand Down
8 changes: 5 additions & 3 deletions db/table_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ InternalIterator* TableCache::NewIterator(
TableReader** table_reader_ptr, HistogramImpl* file_read_hist,
bool for_compaction, Arena* arena, bool skip_filters, int level,
const InternalKey* smallest_compaction_key,
const InternalKey* largest_compaction_key) {
const InternalKey* largest_compaction_key, bool hint_within_lower_bound,
bool hint_within_upper_bound) {
PERF_TIMER_GUARD(new_table_iterator_nanos);

Status s;
Expand Down Expand Up @@ -246,8 +247,9 @@ InternalIterator* TableCache::NewIterator(
!options.table_filter(*table_reader->GetTableProperties())) {
result = NewEmptyInternalIterator<Slice>(arena);
} else {
result = table_reader->NewIterator(options, prefix_extractor, arena,
skip_filters, for_compaction);
result = table_reader->NewIterator(
options, prefix_extractor, arena, skip_filters, for_compaction,
hint_within_lower_bound, hint_within_upper_bound);
}
if (create_new_table_reader) {
assert(handle == nullptr);
Expand Down
8 changes: 7 additions & 1 deletion db/table_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ class TableCache {
// aggregator. If an error occurs, returns it in a NewErrorInternalIterator
// @param skip_filters Disables loading/accessing the filter block
// @param level The level this table is at, -1 for "not set / don't know"
// @param hint_within_lower_bound hint whether the table is fully within
// iterate_lower_bound.
// @param hint_within_upper_bound hint whether the table is fully within
// iterate_upper_bound.
InternalIterator* NewIterator(
const ReadOptions& options, const EnvOptions& toptions,
const InternalKeyComparator& internal_comparator,
Expand All @@ -58,7 +62,9 @@ class TableCache {
HistogramImpl* file_read_hist = nullptr, bool for_compaction = false,
Arena* arena = nullptr, bool skip_filters = false, int level = -1,
const InternalKey* smallest_compaction_key = nullptr,
const InternalKey* largest_compaction_key = nullptr);
const InternalKey* largest_compaction_key = nullptr,
bool hint_within_lower_bound = false,
bool hint_within_upper_bound = false);

// If a seek to internal key "k" in specified file finds an entry,
// call (*handle_result)(arg, found_key, found_value) repeatedly until
Expand Down
35 changes: 32 additions & 3 deletions db/version_set.cc
Original file line number Diff line number Diff line change
Expand Up @@ -508,23 +508,38 @@ class LevelIterator final : public InternalIterator {
assert(Valid());
return file_iter_.key();
}

Slice value() const override {
assert(Valid());
return file_iter_.value();
}

Status status() const override {
return file_iter_.iter() ? file_iter_.status() : Status::OK();
}

bool HintWithinLowerBound() override {
assert(Valid());
return file_iter_.HintWithinLowerBound();
}

bool HintWithinUpperBound() override {
assert(Valid());
return file_iter_.HintWithinUpperBound();
yiwu-arbug marked this conversation as resolved.
Show resolved Hide resolved
}

void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override {
pinned_iters_mgr_ = pinned_iters_mgr;
if (file_iter_.iter()) {
file_iter_.SetPinnedItersMgr(pinned_iters_mgr);
}
}

bool IsKeyPinned() const override {
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
file_iter_.iter() && file_iter_.IsKeyPinned();
}

bool IsValuePinned() const override {
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
file_iter_.iter() && file_iter_.IsValuePinned();
Expand All @@ -541,6 +556,11 @@ class LevelIterator final : public InternalIterator {
return flevel_->files[file_index].smallest_key;
}

const Slice& file_largest_key(size_t file_index) {
assert(file_index < flevel_->num_files);
return flevel_->files[file_index].largest_key;
}

bool KeyReachedUpperBound(const Slice& internal_key) {
return read_options_.iterate_upper_bound != nullptr &&
user_comparator_.Compare(ExtractUserKey(internal_key),
Expand All @@ -560,12 +580,21 @@ class LevelIterator final : public InternalIterator {
smallest_compaction_key = (*compaction_boundaries_)[file_index_].smallest;
largest_compaction_key = (*compaction_boundaries_)[file_index_].largest;
}
bool hint_within_lower_bound =
read_options_.iterate_lower_bound != nullptr &&
user_comparator_.Compare(ExtractUserKey(file_smallest_key(file_index_)),
*read_options_.iterate_lower_bound) >= 0;
bool hint_within_upper_bound =
read_options_.iterate_upper_bound != nullptr &&
user_comparator_.Compare(ExtractUserKey(file_largest_key(file_index_)),
*read_options_.iterate_upper_bound) < 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do allow users to change upper bound for a new Seek(). So maybe we should do it in InitFileIterator and persistent the result in this iterator, even if we don't create a new file iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, the upper bound check result won't change even in InitFileIterator we don't create new file iterator, so it is safe to not updating it in that case. Do I miss anything?

return table_cache_->NewIterator(
read_options_, env_options_, icomparator_, *file_meta.file_metadata,
range_del_agg_, prefix_extractor_,
nullptr /* don't need reference to table */,
file_read_hist_, for_compaction_, nullptr /* arena */, skip_filters_,
level_, smallest_compaction_key, largest_compaction_key);
nullptr /* don't need reference to table */, file_read_hist_,
for_compaction_, nullptr /* arena */, skip_filters_, level_,
smallest_compaction_key, largest_compaction_key,
hint_within_lower_bound, hint_within_upper_bound);
}

TableCache* table_cache_;
Expand Down
22 changes: 15 additions & 7 deletions table/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2488,6 +2488,12 @@ void BlockBasedTableIterator<TBlockIter, TValue>::InitDataBlock() {
key_includes_seq_, index_key_is_full_,
/* get_context */ nullptr, s, prefetch_buffer_.get());
block_iter_points_to_real_block_ = true;
if (read_options_.iterate_upper_bound != nullptr) {
data_block_within_upper_bound_ =
hint_within_upper_bound_ ||
(user_comparator_.Compare(*read_options_.iterate_upper_bound,
index_iter_->user_key()) > 0);
}
}
}

Expand All @@ -2505,9 +2511,7 @@ void BlockBasedTableIterator<TBlockIter, TValue>::FindKeyForward() {
// Do not set is_out_of_bound_ here. Even when index key is out of bound,
// it can be larger than smallest key of the next file on the level, and
// that file may still be within bound.
block_is_out_of_bound =
(user_comparator_.Compare(*read_options_.iterate_upper_bound,
index_iter_->user_key()) <= 0);
block_is_out_of_bound = !data_block_within_upper_bound_;
}
ResetDataIter();
if (block_is_out_of_bound) {
Expand Down Expand Up @@ -2552,14 +2556,16 @@ template <class TBlockIter, typename TValue>
void BlockBasedTableIterator<TBlockIter, TValue>::CheckOutOfBound() {
if (read_options_.iterate_upper_bound != nullptr &&
block_iter_points_to_real_block_ && block_iter_.Valid()) {
is_out_of_bound_ = user_comparator_.Compare(
is_out_of_bound_ = !hint_within_upper_bound_ &&
user_comparator_.Compare(
*read_options_.iterate_upper_bound, user_key()) <= 0;
}
}

InternalIterator* BlockBasedTable::NewIterator(
const ReadOptions& read_options, const SliceTransform* prefix_extractor,
Arena* arena, bool skip_filters, bool for_compaction) {
Arena* arena, bool skip_filters, bool for_compaction,
bool hint_within_lower_bound, bool hint_within_upper_bound) {
bool need_upper_bound_check =
PrefixExtractorChanged(rep_->table_properties.get(), prefix_extractor);
const bool kIsNotIndex = false;
Expand All @@ -2573,7 +2579,8 @@ InternalIterator* BlockBasedTable::NewIterator(
!skip_filters && !read_options.total_order_seek &&
prefix_extractor != nullptr,
need_upper_bound_check, prefix_extractor, kIsNotIndex,
true /*key_includes_seq*/, true /*index_key_is_full*/, for_compaction);
true /*key_includes_seq*/, true /*index_key_is_full*/, for_compaction,
hint_within_lower_bound, hint_within_upper_bound);
} else {
auto* mem =
arena->AllocateAligned(sizeof(BlockBasedTableIterator<DataBlockIter>));
Expand All @@ -2583,7 +2590,8 @@ InternalIterator* BlockBasedTable::NewIterator(
!skip_filters && !read_options.total_order_seek &&
prefix_extractor != nullptr,
need_upper_bound_check, prefix_extractor, kIsNotIndex,
true /*key_includes_seq*/, true /*index_key_is_full*/, for_compaction);
true /*key_includes_seq*/, true /*index_key_is_full*/, for_compaction,
hint_within_lower_bound, hint_within_upper_bound);
}
}

Expand Down
30 changes: 27 additions & 3 deletions table/block_based_table_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ class BlockBasedTable : public TableReader {
const SliceTransform* prefix_extractor,
Arena* arena = nullptr,
bool skip_filters = false,
bool for_compaction = false) override;
bool for_compaction = false,
bool within_lower_bound = false,
bool within_upper_bound = false) override;

FragmentedRangeTombstoneIterator* NewRangeTombstoneIterator(
const ReadOptions& read_options) override;
Expand Down Expand Up @@ -577,7 +579,9 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
const SliceTransform* prefix_extractor, bool is_index,
bool key_includes_seq = true,
bool index_key_is_full = true,
bool for_compaction = false)
bool for_compaction = false,
bool hint_within_lower_bound = false,
bool hint_within_upper_bound = false)
: table_(table),
read_options_(read_options),
icomp_(icomp),
Expand All @@ -591,7 +595,9 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
is_index_(is_index),
key_includes_seq_(key_includes_seq),
index_key_is_full_(index_key_is_full),
for_compaction_(for_compaction) {}
for_compaction_(for_compaction),
hint_within_lower_bound_(hint_within_lower_bound),
hint_within_upper_bound_(hint_within_upper_bound) {}

~BlockBasedTableIterator() { delete index_iter_; }

Expand Down Expand Up @@ -632,6 +638,20 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
// Whether iterator invalidated for being out of bound.
bool IsOutOfBound() override { return is_out_of_bound_; }

bool HintWithinLowerBound() override {
assert(Valid());
assert(read_options_.iterate_lower_bound != nullptr);
// Potentially we can use index key for the previous block as lower bound
// of current block, and check if we are wihtin iterate_lower_bound.
return hint_within_lower_bound_;
}

bool HintWithinUpperBound() override {
assert(Valid());
assert(read_options_.iterate_upper_bound != nullptr);
return data_block_within_upper_bound_;
}

void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override {
pinned_iters_mgr_ = pinned_iters_mgr;
}
Expand Down Expand Up @@ -692,6 +712,8 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
TBlockIter block_iter_;
bool block_iter_points_to_real_block_;
bool is_out_of_bound_ = false;
// Whether current data block being fully within iterate upper bound.
bool data_block_within_upper_bound_;
bool check_filter_;
// TODO(Zhongyi): pick a better name
bool need_upper_bound_check_;
Expand All @@ -703,6 +725,8 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
bool index_key_is_full_;
// If this iterator is created for compaction
bool for_compaction_;
const bool hint_within_lower_bound_;
const bool hint_within_upper_bound_;
BlockHandle prev_index_value_;

static const size_t kInitReadaheadSize = 8 * 1024;
Expand Down
3 changes: 2 additions & 1 deletion table/cuckoo_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,8 @@ Slice CuckooTableIterator::value() const {
InternalIterator* CuckooTableReader::NewIterator(
const ReadOptions& /*read_options*/,
const SliceTransform* /* prefix_extractor */, Arena* arena,
bool /*skip_filters*/, bool /*for_compaction*/) {
bool /*skip_filters*/, bool /*for_compaction*/,
bool /*hint_within_lower_bound*/, bool /*hint_within_upper_bound*/) {
if (!status().ok()) {
return NewErrorInternalIterator<Slice>(
Status::Corruption("CuckooTableReader status is not okay."), arena);
Expand Down
4 changes: 3 additions & 1 deletion table/cuckoo_table_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ class CuckooTableReader: public TableReader {
const SliceTransform* prefix_extractor,
Arena* arena = nullptr,
bool skip_filters = false,
bool for_compaction = false) override;
bool for_compaction = false,
bool hint_within_lower_bound = false,
bool hint_within_upper_bound = false) override;
void Prepare(const Slice& target) override;

// Report an approximation of how much memory has been used.
Expand Down
8 changes: 8 additions & 0 deletions table/internal_iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ class InternalIteratorBase : public Cleanable {
// upper bound
virtual bool IsOutOfBound() { return false; }

// Hint iterate being within iterate_lower_bound. DBIter use it to avoid
// double checking for lower bound.
virtual bool HintWithinLowerBound() { return false; }

// Hint iterate being within iterate_upper_bound. DBIter use it to avoid
// double checking for upper bound.
virtual bool HintWithinUpperBound() { return false; }

// Pass the PinnedIteratorsManager to the Iterator, most Iterators dont
// communicate with PinnedIteratorsManager so default implementation is no-op
// but for Iterators that need to communicate with PinnedIteratorsManager
Expand Down
10 changes: 10 additions & 0 deletions table/iterator_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ class IteratorWrapperBase {
void SeekToFirst() { assert(iter_); iter_->SeekToFirst(); Update(); }
void SeekToLast() { assert(iter_); iter_->SeekToLast(); Update(); }

bool HintWithinLowerBound() {
assert(Valid());
return iter_->HintWithinLowerBound();
}

bool HintWithinUpperBound() {
assert(Valid());
return iter_->HintWithinUpperBound();
}

void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) {
assert(iter_);
iter_->SetPinnedItersMgr(pinned_iters_mgr);
Expand Down
14 changes: 14 additions & 0 deletions table/merging_iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,20 @@ class MergingIterator : public InternalIterator {
return current_->value();
}

// Here we simply relay WithinLowerBound/WithinUpperBound result from current
// child iterator. Potentially as long as one of child iterator report being
// within bound, we can report current key is within bound.

bool HintWithinLowerBound() override {
assert(Valid());
return current_->HintWithinLowerBound();
}

bool HintWithinUpperBound() override {
assert(Valid());
return current_->HintWithinUpperBound();
}

void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override {
pinned_iters_mgr_ = pinned_iters_mgr;
for (auto& child : children_) {
Expand Down
3 changes: 2 additions & 1 deletion table/mock_table.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ stl_wrappers::KVMap MakeMockFile(

InternalIterator* MockTableReader::NewIterator(
const ReadOptions&, const SliceTransform* /* prefix_extractor */,
Arena* /*arena*/, bool /*skip_filters*/, bool /*for_compaction*/) {
Arena* /*arena*/, bool /*skip_filters*/, bool /*for_compaction*/,
bool /*hint_within_lower_bound*/, bool /*hint_within_upper_bound*/) {
return new MockTableIterator(table_);
}

Expand Down
4 changes: 3 additions & 1 deletion table/mock_table.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ class MockTableReader : public TableReader {
const SliceTransform* prefix_extractor,
Arena* arena = nullptr,
bool skip_filters = false,
bool for_compaction = false) override;
bool for_compaction = false,
bool hint_within_lower_bound = false,
bool hint_within_upper_bound = false) override;

Status Get(const ReadOptions& readOptions, const Slice& key,
GetContext* get_context, const SliceTransform* prefix_extractor,
Expand Down
3 changes: 2 additions & 1 deletion table/plain_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,8 @@ void PlainTableReader::SetupForCompaction() {

InternalIterator* PlainTableReader::NewIterator(
const ReadOptions& options, const SliceTransform* /* prefix_extractor */,
Arena* arena, bool /*skip_filters*/, bool /*for_compaction*/) {
Arena* arena, bool /*skip_filters*/, bool /*for_compaction*/,
bool /*hint_within_lower_bound*/, bool /*hint_within_upper_bound*/) {
bool use_prefix_seek = !IsTotalOrderMode() && !options.total_order_seek;
if (arena == nullptr) {
return new PlainTableIterator(this, use_prefix_seek);
Expand Down
4 changes: 3 additions & 1 deletion table/plain_table_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ class PlainTableReader: public TableReader {
const SliceTransform* prefix_extractor,
Arena* arena = nullptr,
bool skip_filters = false,
bool for_compaction = false) override;
bool for_compaction = false,
bool hint_within_lower_bound = false,
bool hint_within_upper_bound = false) override;

void Prepare(const Slice& target) override;

Expand Down
Loading