diff --git a/db/arena_wrapped_db_iter.h b/db/arena_wrapped_db_iter.h index d0eedcb357..0c135f8579 100644 --- a/db/arena_wrapped_db_iter.h +++ b/db/arena_wrapped_db_iter.h @@ -45,26 +45,26 @@ class ArenaWrappedDBIter : public Iterator { // Set the internal iterator wrapped inside the DB Iterator. Usually it is // a merging iterator. virtual void SetIterUnderDBIter(InternalIterator* iter) { - static_cast(db_iter_)->SetIter(iter); + db_iter_->SetIter(iter); } - virtual bool Valid() const override { return db_iter_->Valid(); } - virtual void SeekToFirst() override { db_iter_->SeekToFirst(); } - virtual void SeekToLast() override { db_iter_->SeekToLast(); } - virtual void Seek(const Slice& target) override { db_iter_->Seek(target); } - virtual void SeekForPrev(const Slice& target) override { + bool Valid() const override { return db_iter_->Valid(); } + void SeekToFirst() override { db_iter_->SeekToFirst(); } + void SeekToLast() override { db_iter_->SeekToLast(); } + void Seek(const Slice& target) override { db_iter_->Seek(target); } + void SeekForPrev(const Slice& target) override { db_iter_->SeekForPrev(target); } - virtual void Next() override { db_iter_->Next(); } - virtual void Prev() override { db_iter_->Prev(); } - virtual Slice key() const override { return db_iter_->key(); } - virtual Slice value() const override { return db_iter_->value(); } - virtual Status status() const override { return db_iter_->status(); } + void Next() override { db_iter_->Next(); } + void Prev() override { db_iter_->Prev(); } + Slice key() const override { return db_iter_->key(); } + Slice value() const override { return db_iter_->value(); } + Status status() const override { return db_iter_->status(); } bool IsBlob() const { return db_iter_->IsBlob(); } - virtual Status GetProperty(std::string prop_name, std::string* prop) override; + Status GetProperty(std::string prop_name, std::string* prop) override; - virtual Status Refresh() override; + Status Refresh() override; void Init(Env* env, const ReadOptions& read_options, const ImmutableCFOptions& cf_options, diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index c6fa3c16ef..cb874dbe37 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -2561,7 +2561,7 @@ ArenaWrappedDBIter* DBImpl::NewIteratorImpl(const ReadOptions& read_options, env_, read_options, *cfd->ioptions(), sv->mutable_cf_options, snapshot, sv->mutable_cf_options.max_sequential_skip_in_iterations, sv->version_number, read_callback, this, cfd, allow_blob, - ((read_options.snapshot != nullptr) ? false : allow_refresh)); + read_options.snapshot != nullptr ? false : allow_refresh); InternalIterator* internal_iter = NewInternalIterator(read_options, cfd, sv, db_iter->GetArena(), diff --git a/db/db_iter.cc b/db/db_iter.cc index 4d850abee8..e5d4029481 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -33,19 +33,6 @@ namespace ROCKSDB_NAMESPACE { -#if 0 -static void DumpInternalIter(Iterator* iter) { - for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { - ParsedInternalKey k; - if (!ParseInternalKey(iter->key(), &k)) { - fprintf(stderr, "Corrupt '%s'\n", EscapeString(iter->key()).c_str()); - } else { - fprintf(stderr, "@ '%s'\n", k.DebugString().c_str()); - } - } -} -#endif - DBIter::DBIter(Env* _env, const ReadOptions& read_options, const ImmutableCFOptions& cf_options, const MutableCFOptions& mutable_cf_options, @@ -183,7 +170,7 @@ void DBIter::Next() { // a delete marker or a sequence number higher than sequence_ // saved_key_ MUST have a proper user_key before calling this function // -// The prefix parameter, if not null, indicates that we need to iterator +// The prefix parameter, if not null, indicates that we need to iterate // within the prefix, and the iterator needs to be made invalid, if no // more entry for the prefix can be found. bool DBIter::FindNextUserEntry(bool skipping_saved_key, const Slice* prefix) { @@ -204,13 +191,14 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key, // or equal to saved_key_. We could skip these entries either because // sequence numbers were too high or because skipping_saved_key = true. // What saved_key_ contains throughout this method: - // - if skipping_saved_key : saved_key_ contains the key that we need - // to skip, - // and we haven't seen any keys greater than that, - // - if num_skipped > 0 : saved_key_ contains the key that we have skipped - // num_skipped times, and we haven't seen any keys - // greater than that, - // - none of the above : saved_key_ can contain anything, it doesn't matter. + // - if skipping_saved_key : saved_key_ contains the key that we need + // to skip, and we haven't seen any keys greater + // than that, + // - if num_skipped > 0 : saved_key_ contains the key that we have skipped + // num_skipped times, and we haven't seen any keys + // greater than that, + // - none of the above : saved_key_ can contain anything, it doesn't + // matter. uint64_t num_skipped = 0; // For write unprepared, the target sequence number in reseek could be larger // than the snapshot, and thus needs to be skipped again. This could result in @@ -292,9 +280,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key, if (start_seqnum_ > 0) { // we are taking incremental snapshot here // incremental snapshots aren't supported on DB with range deletes - assert(!( - (ikey_.type == kTypeBlobIndex) && (start_seqnum_ > 0) - )); + assert(ikey_.type != kTypeBlobIndex); if (ikey_.sequence >= start_seqnum_) { saved_key_.SetInternalKey(ikey_); valid_ = true; @@ -372,7 +358,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key, // to seek to the target sequence number. int cmp = user_comparator_.Compare(ikey_.user_key, saved_key_.GetUserKey()); - if (cmp == 0 || (skipping_saved_key && cmp <= 0)) { + if (cmp == 0 || (skipping_saved_key && cmp < 0)) { num_skipped++; } else { saved_key_.SetUserKey( @@ -391,7 +377,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key, // reseek previously. // // TODO(lth): If we reseek to sequence number greater than ikey_.sequence, - // than it does not make sense to reseek as we would actually land further + // then it does not make sense to reseek as we would actually land further // away from the desired key. There is opportunity for optimization here. if (num_skipped > max_skip_ && !reseek_done) { is_key_seqnum_zero_ = false; @@ -1130,18 +1116,16 @@ void DBIter::Seek(const Slice& target) { // Now the inner iterator is placed to the target position. From there, // we need to find out the next key that is visible to the user. - // ClearSavedValue(); if (prefix_same_as_start_) { // The case where the iterator needs to be invalidated if it has exausted // keys within the same prefix of the seek key. assert(prefix_extractor_ != nullptr); - Slice target_prefix; - target_prefix = prefix_extractor_->Transform(target); + Slice target_prefix = prefix_extractor_->Transform(target); FindNextUserEntry(false /* not skipping saved_key */, &target_prefix /* prefix */); if (valid_) { - // Remember the prefix of the seek key for the future Prev() call to + // Remember the prefix of the seek key for the future Next() call to // check. prefix_.SetUserKey(target_prefix); } @@ -1197,8 +1181,7 @@ void DBIter::SeekForPrev(const Slice& target) { // The case where the iterator needs to be invalidated if it has exausted // keys within the same prefix of the seek key. assert(prefix_extractor_ != nullptr); - Slice target_prefix; - target_prefix = prefix_extractor_->Transform(target); + Slice target_prefix = prefix_extractor_->Transform(target); PrevInternal(&target_prefix); if (valid_) { // Remember the prefix of the seek key for the future Prev() call to diff --git a/db/db_iter.h b/db/db_iter.h index 74430f0d38..32704e4d5f 100644 --- a/db/db_iter.h +++ b/db/db_iter.h @@ -25,10 +25,10 @@ namespace ROCKSDB_NAMESPACE { // This file declares the factory functions of DBIter, in its original form // or a wrapped form with class ArenaWrappedDBIter, which is defined here. // Class DBIter, which is declared and implemented inside db_iter.cc, is -// a iterator that converts internal keys (yielded by an InternalIterator) +// an iterator that converts internal keys (yielded by an InternalIterator) // that were live at the specified sequence number into appropriate user // keys. -// Each internal key is consist of a user key, a sequence number, and a value +// Each internal key consists of a user key, a sequence number, and a value // type. DBIter deals with multiple key versions, tombstones, merge operands, // etc, and exposes an Iterator. // For example, DBIter may wrap following InternalIterator: @@ -133,14 +133,12 @@ class DBIter final : public Iterator { local_stats_.BumpGlobalStatistics(statistics_); iter_.DeleteIter(arena_mode_); } - virtual void SetIter(InternalIterator* iter) { + void SetIter(InternalIterator* iter) { assert(iter_.iter() == nullptr); iter_.Set(iter); iter_.iter()->SetPinnedItersMgr(&pinned_iters_mgr_); } - virtual ReadRangeDelAggregator* GetRangeDelAggregator() { - return &range_del_agg_; - } + ReadRangeDelAggregator* GetRangeDelAggregator() { return &range_del_agg_; } bool Valid() const override { return valid_; } Slice key() const override { @@ -184,7 +182,7 @@ class DBIter final : public Iterator { void SeekForPrev(const Slice& target) final override; void SeekToFirst() final override; void SeekToLast() final override; - Env* env() { return env_; } + Env* env() const { return env_; } void set_sequence(uint64_t s) { sequence_ = s; if (read_callback_) { @@ -202,10 +200,10 @@ class DBIter final : public Iterator { bool ReverseToBackward(); // Set saved_key_ to the seek key to target, with proper sequence number set. // It might get adjusted if the seek key is smaller than iterator lower bound. - void SetSavedKeyToSeekTarget(const Slice& /*target*/); + void SetSavedKeyToSeekTarget(const Slice& target); // Set saved_key_ to the seek key to target, with proper sequence number set. // It might get adjusted if the seek key is larger than iterator upper bound. - void SetSavedKeyToSeekForPrevTarget(const Slice& /*target*/); + void SetSavedKeyToSeekForPrevTarget(const Slice& target); bool FindValueForCurrentKey(); bool FindValueForCurrentKeyUsingSeek(); bool FindUserKeyBeforeSavedKey(); @@ -221,7 +219,7 @@ class DBIter final : public Iterator { // If prefix is not null, we need to set the iterator to invalid if no more // entry can be found within the prefix. - void PrevInternal(const Slice* /*prefix*/); + void PrevInternal(const Slice* prefix); bool TooManyInternalKeysSkipped(bool increment = true); bool IsVisible(SequenceNumber sequence); @@ -330,6 +328,7 @@ class DBIter final : public Iterator { // if this value > 0 iterator will return internal keys SequenceNumber start_seqnum_; }; + // Return a new iterator that converts internal keys (yielded by // "*internal_iter") that were live at the specified `sequence` number // into appropriate user keys. diff --git a/table/block_based/block.cc b/table/block_based/block.cc index d3a57632e4..a04dd8ac29 100644 --- a/table/block_based/block.cc +++ b/table/block_based/block.cc @@ -156,12 +156,9 @@ void IndexBlockIter::Prev() { restart_index_--; } SeekToRestartPoint(restart_index_); - do { - if (!ParseNextIndexKey()) { - break; - } - // Loop until end of current entry hits the start of original entry - } while (NextEntryOffset() < original); + // Loop until end of current entry hits the start of original entry + while (ParseNextIndexKey() && NextEntryOffset() < original) { + } } // Similar to IndexBlockIter::Prev but also caches the prev entries @@ -253,12 +250,9 @@ void DataBlockIter::Seek(const Slice& target) { return; } SeekToRestartPoint(index); - // Linear search (within restart block) for first key >= target - while (true) { - if (!ParseNextDataKey() || Compare(key_, seek_key) >= 0) { - return; - } + // Linear search (within restart block) for first key >= target + while (ParseNextDataKey() && Compare(key_, seek_key) < 0) { } } @@ -415,12 +409,9 @@ void IndexBlockIter::Seek(const Slice& target) { return; } SeekToRestartPoint(index); - // Linear search (within restart block) for first key >= target - while (true) { - if (!ParseNextIndexKey() || Compare(key_, seek_key) >= 0) { - return; - } + // Linear search (within restart block) for first key >= target + while (ParseNextIndexKey() && Compare(key_, seek_key) < 0) { } } @@ -438,8 +429,8 @@ void DataBlockIter::SeekForPrev(const Slice& target) { return; } SeekToRestartPoint(index); - // Linear search (within restart block) for first key >= seek_key + // Linear search (within restart block) for first key >= seek_key while (ParseNextDataKey() && Compare(key_, seek_key) < 0) { } if (!Valid()) { diff --git a/table/block_based/block.h b/table/block_based/block.h index 95f2fac908..e82a1b2a67 100644 --- a/table/block_based/block.h +++ b/table/block_based/block.h @@ -188,7 +188,6 @@ class Block { // NOTE: for the hash based lookup, if a key prefix doesn't match any key, // the iterator will simply be set as "invalid", rather than returning // the key that is just pass the target key. - DataBlockIter* NewDataIterator(const Comparator* comparator, const Comparator* user_comparator, DataBlockIter* iter = nullptr, @@ -269,31 +268,30 @@ class BlockIter : public InternalIteratorBase { Cleanable::Reset(); } - virtual bool Valid() const override { return current_ < restarts_; } - virtual Status status() const override { return status_; } - virtual Slice key() const override { + bool Valid() const override { return current_ < restarts_; } + Status status() const override { return status_; } + Slice key() const override { assert(Valid()); return key_.GetKey(); } #ifndef NDEBUG - virtual ~BlockIter() { + ~BlockIter() override { // Assert that the BlockIter is never deleted while Pinning is Enabled. assert(!pinned_iters_mgr_ || (pinned_iters_mgr_ && !pinned_iters_mgr_->PinningEnabled())); } - virtual void SetPinnedItersMgr( - PinnedIteratorsManager* pinned_iters_mgr) override { + void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override { pinned_iters_mgr_ = pinned_iters_mgr; } PinnedIteratorsManager* pinned_iters_mgr_ = nullptr; #endif - virtual bool IsKeyPinned() const override { + bool IsKeyPinned() const override { return block_contents_pinned_ && key_pinned_; } - virtual bool IsValuePinned() const override { return block_contents_pinned_; } + bool IsValuePinned() const override { return block_contents_pinned_; } size_t TEST_CurrentEntrySize() { return NextEntryOffset() - current_; } @@ -394,7 +392,7 @@ class DataBlockIter final : public BlockIter { data_block_hash_index_ = data_block_hash_index; } - virtual Slice value() const override { + Slice value() const override { assert(Valid()); if (read_amp_bitmap_ && current_ < restarts_ && current_ != last_bitmap_offset_) { @@ -405,7 +403,7 @@ class DataBlockIter final : public BlockIter { return value_; } - virtual void Seek(const Slice& target) override; + void Seek(const Slice& target) override; inline bool SeekForGet(const Slice& target) { if (!data_block_hash_index_) { @@ -416,25 +414,25 @@ class DataBlockIter final : public BlockIter { return SeekForGetImpl(target); } - virtual void SeekForPrev(const Slice& target) override; + void SeekForPrev(const Slice& target) override; - virtual void Prev() override; + void Prev() override; - virtual void Next() final override; + void Next() final override; // Try to advance to the next entry in the block. If there is data corruption // or error, report it to the caller instead of aborting the process. May // incur higher CPU overhead because we need to perform check on every entry. void NextOrReport(); - virtual void SeekToFirst() override; + void SeekToFirst() override; // Try to seek to the first entry in the block. If there is data corruption // or error, report it to caller instead of aborting the process. May incur // higher CPU overhead because we need to perform check on every entry. void SeekToFirstOrReport(); - virtual void SeekToLast() override; + void SeekToLast() override; void Invalidate(Status s) { InvalidateBase(s); @@ -490,7 +488,7 @@ class IndexBlockIter final : public BlockIter { public: IndexBlockIter() : BlockIter(), prefix_index_(nullptr) {} - virtual Slice key() const override { + Slice key() const override { assert(Valid()); return key_.GetKey(); } @@ -526,7 +524,7 @@ class IndexBlockIter final : public BlockIter { return key(); } - virtual IndexValue value() const override { + IndexValue value() const override { assert(Valid()); if (value_delta_encoded_ || global_seqno_state_ != nullptr) { return decoded_value_; @@ -547,9 +545,9 @@ class IndexBlockIter final : public BlockIter { // If the prefix of `target` doesn't exist in the file, it can either // return the result of total order seek, or set both of Valid() = false // and status() = NotFound(). - virtual void Seek(const Slice& target) override; + void Seek(const Slice& target) override; - virtual void SeekForPrev(const Slice&) override { + void SeekForPrev(const Slice&) override { assert(false); current_ = restarts_; restart_index_ = num_restarts_; @@ -560,13 +558,13 @@ class IndexBlockIter final : public BlockIter { value_.clear(); } - virtual void Prev() override; + void Prev() override; - virtual void Next() override; + void Next() override; - virtual void SeekToFirst() override; + void SeekToFirst() override; - virtual void SeekToLast() override; + void SeekToLast() override; void Invalidate(Status s) { InvalidateBase(s); } diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index ffa4c68926..9b37b431f0 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -54,6 +54,7 @@ #include "util/crc32c.h" #include "util/stop_watch.h" #include "util/string_util.h" +#include "util/util.h" #include "util/xxhash.h" namespace ROCKSDB_NAMESPACE { @@ -4047,6 +4048,7 @@ Status BlockBasedTable::CreateIndexReader( index_reader); } case BlockBasedTableOptions::kBinarySearch: + FALLTHROUGH_INTENDED; case BlockBasedTableOptions::kBinarySearchWithFirstKey: { return BinarySearchIndexReader::Create(this, prefetch_buffer, use_cache, prefetch, pin, lookup_context, diff --git a/table/iterator_wrapper.h b/table/iterator_wrapper.h index ac37f69199..c13359e991 100644 --- a/table/iterator_wrapper.h +++ b/table/iterator_wrapper.h @@ -56,7 +56,7 @@ class IteratorWrapperBase { } // Iterator interface methods - bool Valid() const { return valid_; } + bool Valid() const { return valid_; } Slice key() const { assert(Valid()); return result_.key; @@ -66,13 +66,20 @@ class IteratorWrapperBase { return iter_->value(); } // Methods below require iter() != nullptr - Status status() const { assert(iter_); return iter_->status(); } + Status status() const { + assert(iter_); + return iter_->status(); + } void Next() { assert(iter_); valid_ = iter_->NextAndGetResult(&result_); assert(!valid_ || iter_->status().ok()); } - void Prev() { assert(iter_); iter_->Prev(); Update(); } + void Prev() { + assert(iter_); + iter_->Prev(); + Update(); + } void Seek(const Slice& k) { assert(iter_); iter_->Seek(k); @@ -83,8 +90,16 @@ class IteratorWrapperBase { iter_->SeekForPrev(k); Update(); } - void SeekToFirst() { assert(iter_); iter_->SeekToFirst(); Update(); } - void SeekToLast() { assert(iter_); iter_->SeekToLast(); Update(); } + void SeekToFirst() { + assert(iter_); + iter_->SeekToFirst(); + Update(); + } + void SeekToLast() { + assert(iter_); + iter_->SeekToLast(); + Update(); + } bool MayBeOutOfLowerBound() { assert(Valid()); diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc index 205fb8d6d5..47fa048f39 100644 --- a/table/merging_iterator.cc +++ b/table/merging_iterator.cc @@ -167,7 +167,6 @@ class MergingIterator : public InternalIterator { SwitchToForward(); // The loop advanced all non-current children to be > key() so current_ // should still be strictly the smallest key. - assert(current_ == CurrentForward()); } // For the heap modifications below to be correct, current_ must be the