De-template block based table iterator #6531
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Right now block based table iterator is used as both of iterating data for block based table, and for the index iterator for partitioend index. This was initially convenient for introducing a new iterator and block type for new index format, while reducing code change. However, these two usage doesn't go with each other very well. For example, Prev() is never called for partitioned index iterator, and some other complexity is maintained in block based iterators, which is not needed for index iterator but maintainers will always need to reason about it. Furthermore, the template usage is not following Google C++ Style which we are following, and makes a large chunk of code tangled together. This commit separate the two iterators. Right now, here is what it is done: 1. Copy the block based iterator code into partitioned index iterator, and de-template them. 2. Remove some code not needed for partitioned index. The upper bound check and tricks are removed. We never tested performance for those tricks when partitioned index is enabled in the first place. It's unlikelyl to generate performance regression, as creating new partitioned index block is much rarer than data blocks. 3. Separate out the prefetch logic to a helper class and both classes call them. This commit will enable future follow-ups. One direction is that we might separate index iterator interface for data blocks and index blocks, as they are quite different. Test Plan: build using make and cmake
ltamasi
reviewed
Mar 13, 2020
| CheckOutOfBound(); | ||
|
|
||
| if (target) { | ||
| assert(!Valid() || ((block_type_ == BlockType::kIndex && |
There was a problem hiding this comment.
block_type_ can no longer be kIndex in this class, right?
table/block_based/block_prefetcher.h
Outdated
| namespace ROCKSDB_NAMESPACE { | ||
| class BlockPrefetcher { | ||
| public: | ||
| BlockPrefetcher(size_t compaction_readahead_size) |
There was a problem hiding this comment.
We probably should make this ctor explicit.
| user_comparator_(icomp.user_comparator()), | ||
| index_iter_(index_iter), | ||
| block_iter_points_to_real_block_(false), | ||
| block_type_(block_type), |
There was a problem hiding this comment.
... on the contrary, block_type_ can only be kIndex in this class, right? If so, we could remove this member altogether.
| @@ -124,8 +124,10 @@ LIB_SOURCES = \ | |||
| table/block_based/block_based_filter_block.cc \ | |||
| table/block_based/block_based_table_builder.cc \ | |||
| table/block_based/block_based_table_factory.cc \ | |||
| table/block_based/block_based_table_iterator.cc \ | |||
There was a problem hiding this comment.
Could you please run the buckifier to update TARGETS as well?
| bool is_for_compaction = | ||
| lookup_context_.caller == TableReaderCaller::kCompaction; | ||
| // Prefetch additional data for range scans (iterators). Enabled only for | ||
| // user reads. |
There was a problem hiding this comment.
I don't think this is true anymore?
| @@ -39,7 +39,7 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> { | |||
| prefix_extractor_(prefix_extractor), | |||
| block_type_(block_type), | |||
| lookup_context_(caller), | |||
| compaction_readahead_size_(compaction_readahead_size) {} | |||
| block_prefetcher_(compaction_readahead_size) {} | |||
|
|
|||
| ~BlockBasedTableIterator() { delete index_iter_; } | |||
There was a problem hiding this comment.
We could use unique_ptr instead of explicitly calling delete.
| lookup_context_(caller), | ||
| block_prefetcher_(compaction_readahead_size) {} | ||
|
|
||
| ~ParititionedIndexIterator() { delete index_iter_; } |
There was a problem hiding this comment.
We could make index_iter_ a unique_ptr here as well.
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in d669080. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 17, 2020
Summary: #6531 removed some code in partitioned index seek logic. By mistake the logic of storing previous index offset is removed, while the logic of using it is preserved, so that the code might use wrong value to determine reseeking condition. This will trigger a bug, if following a Seek() not going to the last block, SeekToLast() is called, and then Seek() is called which should position the cursor to the block before SeekToLast(). Pull Request resolved: #6551 Test Plan: Add a unit test that reproduces the bug. In the same unit test, also some reseek cases are covered to avoid regression. Reviewed By: pdillinger Differential Revision: D20493990 fbshipit-source-id: 3919aa4861c0481ec96844e053048da1a934b91d
sthagen
added a commit
to sthagen/facebook-rocksdb
that referenced
this pull request
Mar 18, 2020
Fix regression bug in partitioned index reseek caused by facebook#6531 (facebook#6551)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Right now block based table iterator is used as both of iterating data for block based table, and for the index iterator for partitioend index. This was initially convenient for introducing a new iterator and block type for new index format, while reducing code change. However, these two usage doesn't go with each other very well. For example, Prev() is never called for partitioned index iterator, and some other complexity is maintained in block based iterators, which is not needed for index iterator but maintainers will always need to reason about it. Furthermore, the template usage is not following Google C++ Style which we are following, and makes a large chunk of code tangled together. This commit separate the two iterators. Right now, here is what it is done:
This commit will enable future follow-ups. One direction is that we might separate index iterator interface for data blocks and index blocks, as they are quite different.
Test Plan: build using make and cmake. And build release