From 992dfc78110782daf28aff0c8ba543aad4ce440d Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Thu, 18 Apr 2019 11:08:33 -0700 Subject: [PATCH] Introduce InternalIteratorBase::NextAndGetResult() (#5197) Summary: In long scans, virtual function calls of Next(), Valid(), key() and value() are not trivial. By introducing NextAndGetResult(), Some of the Next(), Valid() and key() calls are consolidated into one virtual function call to reduce CPU. Also did some inline tricks and add some "final" randomly in some functions. Even without the "final" annotation, most Next() calls are inlined with -O3, but sometimes with a final it is inlined by O2 too. It doesn't hurt to add those final annotations. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5197 Differential Revision: D14945977 Pulled By: siying fbshipit-source-id: 7003969f9a5f1d5717f0bda503b91d19ba75ed88 --- db/db_iter.cc | 2 +- db/version_set.cc | 23 ++++++++++++++++++----- table/block.h | 2 +- table/block_based_table_reader.cc | 24 ++++++++++++++++++++++-- table/block_based_table_reader.h | 6 ++++-- table/internal_iterator.h | 9 +++++++++ table/iterator_wrapper.h | 6 +++++- 7 files changed, 60 insertions(+), 12 deletions(-) diff --git a/db/db_iter.cc b/db/db_iter.cc index 541a5fbed..df2be1b7c 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -231,7 +231,7 @@ class DBIter final: public Iterator { return Status::InvalidArgument("Unidentified property."); } - void Next() override; + void Next() final override; void Prev() override; void Seek(const Slice& target) override; void SeekForPrev(const Slice& target) override; diff --git a/db/version_set.cc b/db/version_set.cc index 1295baf7e..e916fed6e 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -906,7 +906,8 @@ class LevelIterator final : public InternalIterator { void SeekForPrev(const Slice& target) override; void SeekToFirst() override; void SeekToLast() override; - void Next() override; + void Next() final override; + bool NextAndGetResult(Slice* ret_key) override; void Prev() override; bool Valid() const override { return file_iter_.Valid(); } @@ -942,6 +943,13 @@ class LevelIterator final : public InternalIterator { void SetFileIterator(InternalIterator* iter); void InitFileIterator(size_t new_file_index); + // Called by both of Next() and NextAndGetResult(). Force inline. + void NextImpl() { + assert(Valid()); + file_iter_.Next(); + SkipEmptyFileForward(); + } + const Slice& file_smallest_key(size_t file_index) { assert(file_index < flevel_->num_files); return flevel_->files[file_index].smallest_key; @@ -1037,10 +1045,15 @@ void LevelIterator::SeekToLast() { SkipEmptyFileBackward(); } -void LevelIterator::Next() { - assert(Valid()); - file_iter_.Next(); - SkipEmptyFileForward(); +void LevelIterator::Next() { NextImpl(); } + +bool LevelIterator::NextAndGetResult(Slice* ret_key) { + NextImpl(); + bool is_valid = Valid(); + if (is_valid) { + *ret_key = key(); + } + return is_valid; } void LevelIterator::Prev() { diff --git a/table/block.h b/table/block.h index 7ef001d58..df4d4eb82 100644 --- a/table/block.h +++ b/table/block.h @@ -392,7 +392,7 @@ class DataBlockIter final : public BlockIter { virtual void Prev() override; - virtual void Next() override; + virtual 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 diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index c6ceacdcb..9db053842 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -2435,6 +2435,17 @@ void BlockBasedTableIterator::Next() { FindKeyForward(); } +template +bool BlockBasedTableIterator::NextAndGetResult( + Slice* ret_key) { + Next(); + bool is_valid = Valid(); + if (is_valid) { + *ret_key = key(); + } + return is_valid; +} + template void BlockBasedTableIterator::Prev() { assert(block_iter_points_to_real_block_); @@ -2493,10 +2504,10 @@ void BlockBasedTableIterator::InitDataBlock() { } template -void BlockBasedTableIterator::FindKeyForward() { +void BlockBasedTableIterator::FindBlockForward() { // 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()) { + do { if (!block_iter_.status().ok()) { return; } @@ -2528,6 +2539,15 @@ void BlockBasedTableIterator::FindKeyForward() { } else { return; } + } while (!block_iter_.Valid()); +} + +template +void BlockBasedTableIterator::FindKeyForward() { + assert(!is_out_of_bound_); + + if (!block_iter_.Valid()) { + FindBlockForward(); } } diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index 82fe3c888..ea8ba62c5 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -611,7 +611,8 @@ class BlockBasedTableIterator : public InternalIteratorBase { void SeekForPrev(const Slice& target) override; void SeekToFirst() override; void SeekToLast() override; - void Next() override; + void Next() final override; + bool NextAndGetResult(Slice* ret_key) override; void Prev() override; bool Valid() const override { return !is_out_of_bound_ && block_iter_points_to_real_block_ && @@ -688,7 +689,8 @@ class BlockBasedTableIterator : public InternalIteratorBase { } void InitDataBlock(); - void FindKeyForward(); + inline void FindKeyForward(); + void FindBlockForward(); void FindKeyBackward(); void CheckOutOfBound(); diff --git a/table/internal_iterator.h b/table/internal_iterator.h index d08fcae25..6b713e7b9 100644 --- a/table/internal_iterator.h +++ b/table/internal_iterator.h @@ -54,6 +54,15 @@ class InternalIteratorBase : public Cleanable { // REQUIRES: Valid() virtual void Next() = 0; + virtual bool NextAndGetResult(Slice* ret_key) { + Next(); + bool is_valid = Valid(); + if (is_valid) { + *ret_key = key(); + } + return is_valid; + } + // Moves to the previous entry in the source. After this call, Valid() is // true iff the iterator was not positioned at the first entry in source. // REQUIRES: Valid() diff --git a/table/iterator_wrapper.h b/table/iterator_wrapper.h index 5941b846a..fc5eb2613 100644 --- a/table/iterator_wrapper.h +++ b/table/iterator_wrapper.h @@ -63,7 +63,11 @@ class IteratorWrapperBase { } // Methods below require iter() != nullptr Status status() const { assert(iter_); return iter_->status(); } - void Next() { assert(iter_); iter_->Next(); Update(); } + void Next() { + assert(iter_); + valid_ = iter_->NextAndGetResult(&key_); + assert(!valid_ || iter_->status().ok()); + } void Prev() { assert(iter_); iter_->Prev(); Update(); } void Seek(const Slice& k) { assert(iter_); iter_->Seek(k); Update(); } void SeekForPrev(const Slice& k) {