From a3609b7dde4b8a37602c74d5cf08a502a067198e Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Fri, 31 May 2019 11:37:21 -0700 Subject: [PATCH] Improve const correctness in BlockBasedTableReader (#5383) Summary: Many methods are passing around pointers to non-const objects when in fact they do not/should not modify said objects. The patch makes the semantics clearer and also helps from a thread safety point-of-view by changing some pointers to pointers-to-const and marking some instance methods as const. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5383 Differential Revision: D15562770 Pulled By: ltamasi fbshipit-source-id: 89361dadbb8b25bbe54d17e8da28fee24a2419af --- table/block_based/block_based_table_reader.cc | 36 ++++++++++--------- table/block_based/block_based_table_reader.h | 26 ++++++-------- 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 944a1fde4..b7fba779f 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -203,19 +203,20 @@ bool PrefixExtractorChanged(const TableProperties* table_properties, // in the cache or not. class BlockBasedTable::IndexReaderCommon : public BlockBasedTable::IndexReader { public: - IndexReaderCommon(BlockBasedTable* t, CachableEntry&& index_block) + IndexReaderCommon(const BlockBasedTable* t, + CachableEntry&& index_block) : table_(t), index_block_(std::move(index_block)) { assert(table_ != nullptr); } protected: - static Status ReadIndexBlock(BlockBasedTable* table, + static Status ReadIndexBlock(const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, const ReadOptions& read_options, GetContext* get_context, CachableEntry* index_block); - BlockBasedTable* table() const { return table_; } + const BlockBasedTable* table() const { return table_; } const InternalKeyComparator* internal_comparator() const { assert(table_ != nullptr); @@ -256,12 +257,12 @@ class BlockBasedTable::IndexReaderCommon : public BlockBasedTable::IndexReader { } private: - BlockBasedTable* table_; + const BlockBasedTable* table_; CachableEntry index_block_; }; Status BlockBasedTable::IndexReaderCommon::ReadIndexBlock( - BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, + const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, const ReadOptions& read_options, GetContext* get_context, CachableEntry* index_block) { PERF_TIMER_GUARD(read_index_block_nanos); @@ -304,7 +305,7 @@ class PartitionIndexReader : public BlockBasedTable::IndexReaderCommon { // `PartitionIndexReader`. // On success, index_reader will be populated; otherwise it will remain // unmodified. - static Status Create(BlockBasedTable* table, + static Status Create(const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, bool use_cache, bool prefetch, bool pin, IndexReader** index_reader) { assert(table != nullptr); @@ -473,7 +474,8 @@ class PartitionIndexReader : public BlockBasedTable::IndexReaderCommon { } private: - PartitionIndexReader(BlockBasedTable* t, CachableEntry&& index_block) + PartitionIndexReader(const BlockBasedTable* t, + CachableEntry&& index_block) : IndexReaderCommon(t, std::move(index_block)) {} std::unordered_map> partition_map_; @@ -488,7 +490,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon { // `BinarySearchIndexReader`. // On success, index_reader will be populated; otherwise it will remain // unmodified. - static Status Create(BlockBasedTable* table, + static Status Create(const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, bool use_cache, bool prefetch, bool pin, IndexReader** index_reader) { assert(table != nullptr); @@ -553,7 +555,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon { } private: - BinarySearchIndexReader(BlockBasedTable* t, + BinarySearchIndexReader(const BlockBasedTable* t, CachableEntry&& index_block) : IndexReaderCommon(t, std::move(index_block)) {} }; @@ -562,7 +564,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon { // key. class HashIndexReader : public BlockBasedTable::IndexReaderCommon { public: - static Status Create(BlockBasedTable* table, + static Status Create(const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer, InternalIterator* meta_index_iter, bool use_cache, bool prefetch, bool pin, IndexReader** index_reader) { @@ -699,7 +701,7 @@ class HashIndexReader : public BlockBasedTable::IndexReaderCommon { } private: - HashIndexReader(BlockBasedTable* t, CachableEntry&& index_block) + HashIndexReader(const BlockBasedTable* t, CachableEntry&& index_block) : IndexReaderCommon(t, std::move(index_block)) {} std::unique_ptr prefix_index_; @@ -1188,7 +1190,7 @@ Status BlockBasedTable::ReadRangeDelBlock( } Status BlockBasedTable::ReadCompressionDictBlock( - Rep* rep, FilePrefetchBuffer* prefetch_buffer, + const Rep* rep, FilePrefetchBuffer* prefetch_buffer, std::unique_ptr* compression_dict_block) { assert(compression_dict_block != nullptr); Status s; @@ -1842,7 +1844,7 @@ CachableEntry BlockBasedTable::GetFilter( } CachableEntry -BlockBasedTable::GetUncompressionDict(Rep* rep, +BlockBasedTable::GetUncompressionDict(const Rep* rep, FilePrefetchBuffer* prefetch_buffer, bool no_io, GetContext* get_context) { if (!rep->table_options.cache_index_and_filter_blocks) { @@ -1925,7 +1927,7 @@ BlockBasedTable::GetUncompressionDict(Rep* rep, // differs from the one in mutable_cf_options and index type is HashBasedIndex InternalIteratorBase* BlockBasedTable::NewIndexIterator( const ReadOptions& read_options, bool disable_prefix_seek, - IndexBlockIter* input_iter, GetContext* get_context) { + IndexBlockIter* input_iter, GetContext* get_context) const { assert(rep_ != nullptr); assert(rep_->index_reader != nullptr); @@ -1941,7 +1943,7 @@ InternalIteratorBase* BlockBasedTable::NewIndexIterator( // If input_iter is not null, update this iter and return it template TBlockIter* BlockBasedTable::NewDataBlockIterator( - Rep* rep, const ReadOptions& ro, const BlockHandle& handle, + const Rep* rep, const ReadOptions& ro, const BlockHandle& handle, TBlockIter* input_iter, bool is_index, bool key_includes_seq, bool index_key_is_full, GetContext* get_context, Status s, FilePrefetchBuffer* prefetch_buffer) { @@ -2164,7 +2166,7 @@ Status BlockBasedTable::RetrieveBlock( } BlockBasedTable::PartitionedIndexIteratorState::PartitionedIndexIteratorState( - BlockBasedTable* table, + const BlockBasedTable* table, std::unordered_map>* block_map, bool index_key_includes_seq, bool index_key_is_full) : table_(table), @@ -2214,7 +2216,7 @@ BlockBasedTable::PartitionedIndexIteratorState::NewSecondaryIterator( bool BlockBasedTable::PrefixMayMatch( const Slice& internal_key, const ReadOptions& read_options, const SliceTransform* options_prefix_extractor, - const bool need_upper_bound_check) { + const bool need_upper_bound_check) const { if (!rep_->filter_policy) { return true; } diff --git a/table/block_based/block_based_table_reader.h b/table/block_based/block_based_table_reader.h index 3af617fec..f6f610ca2 100644 --- a/table/block_based/block_based_table_reader.h +++ b/table/block_based/block_based_table_reader.h @@ -112,7 +112,7 @@ class BlockBasedTable : public TableReader { bool PrefixMayMatch(const Slice& internal_key, const ReadOptions& read_options, const SliceTransform* options_prefix_extractor, - const bool need_upper_bound_check); + const bool need_upper_bound_check) const; // Returns a new iterator over the table contents. // The result of NewIterator() is initially invalid (caller must @@ -215,18 +215,12 @@ class BlockBasedTable : public TableReader { struct Rep; Rep* get_rep() { return rep_; } + const Rep* get_rep() const { return rep_; } // input_iter: if it is not null, update this one and return it as Iterator template static TBlockIter* NewDataBlockIterator( - Rep* rep, const ReadOptions& ro, const Slice& index_value, - TBlockIter* input_iter = nullptr, bool is_index = false, - bool key_includes_seq = true, bool index_key_is_full = true, - GetContext* get_context = nullptr, - FilePrefetchBuffer* prefetch_buffer = nullptr); - template - static TBlockIter* NewDataBlockIterator( - Rep* rep, const ReadOptions& ro, const BlockHandle& block_hanlde, + const Rep* rep, const ReadOptions& ro, const BlockHandle& block_hanlde, TBlockIter* input_iter = nullptr, bool is_index = false, bool key_includes_seq = true, bool index_key_is_full = true, GetContext* get_context = nullptr, Status s = Status(), @@ -283,7 +277,7 @@ class BlockBasedTable : public TableReader { const SliceTransform* prefix_extractor = nullptr) const; static CachableEntry GetUncompressionDict( - Rep* rep, FilePrefetchBuffer* prefetch_buffer, bool no_io, + const Rep* rep, FilePrefetchBuffer* prefetch_buffer, bool no_io, GetContext* get_context); // Get the iterator from the index reader. @@ -299,7 +293,7 @@ class BlockBasedTable : public TableReader { InternalIteratorBase* NewIndexIterator( const ReadOptions& read_options, bool need_upper_bound_check = false, IndexBlockIter* input_iter = nullptr, - GetContext* get_context = nullptr); + GetContext* get_context = nullptr) const; // Read block cache from block caches (if set): block_cache and // block_cache_compressed. @@ -386,7 +380,7 @@ class BlockBasedTable : public TableReader { InternalIterator* meta_iter, const InternalKeyComparator& internal_comparator); static Status ReadCompressionDictBlock( - Rep* rep, FilePrefetchBuffer* prefetch_buffer, + const Rep* rep, FilePrefetchBuffer* prefetch_buffer, std::unique_ptr* compression_dict_block); static Status PrefetchIndexAndFilterBlocks( Rep* rep, FilePrefetchBuffer* prefetch_buffer, @@ -430,7 +424,7 @@ class BlockBasedTable::PartitionedIndexIteratorState : public TwoLevelIteratorState { public: PartitionedIndexIteratorState( - BlockBasedTable* table, + const BlockBasedTable* table, std::unordered_map>* block_map, const bool index_key_includes_seq, const bool index_key_is_full); InternalIteratorBase* NewSecondaryIterator( @@ -438,7 +432,7 @@ class BlockBasedTable::PartitionedIndexIteratorState private: // Don't own table_ - BlockBasedTable* table_; + const BlockBasedTable* table_; std::unordered_map>* block_map_; bool index_key_includes_seq_; bool index_key_is_full_; @@ -561,7 +555,7 @@ struct BlockBasedTable::Rep { template class BlockBasedTableIterator : public InternalIteratorBase { public: - BlockBasedTableIterator(BlockBasedTable* table, + BlockBasedTableIterator(const BlockBasedTable* table, const ReadOptions& read_options, const InternalKeyComparator& icomp, InternalIteratorBase* index_iter, @@ -681,7 +675,7 @@ class BlockBasedTableIterator : public InternalIteratorBase { void CheckOutOfBound(); private: - BlockBasedTable* table_; + const BlockBasedTable* table_; const ReadOptions read_options_; const InternalKeyComparator& icomp_; UserComparatorWrapper user_comparator_;